Re: svn commit: r197790 - head/etc

[Doug Barton]

I think this is the wrong solution to the problem. In at least two
cases (routed and route6d) where $command is not defined in the rc.d
scripts this change is resulting in $command not being defined at all.

If you look at the definition of the + parameter substitution this
makes sense:

     ${parameter:+word}
             Use Alternate Value.  If parameter is unset or null, null
             is substituted; otherwise, the expansion of word is
             substituted.

I think that what you really wanted to do was:

Index: rc.subr
===================================================================
--- rc.subr (revision 198000)
+++ rc.subr (working copy)
@@ -616,7 +616,7 @@
  esac

  eval _override_command=\$${name}_program
- command=${command:+${_override_command:-$command}}
+ command=${_override_command:-$command}

  _keywords="start stop restart rcvar $extra_commands"
  rc_pid=

That actually makes more sense anyway, at least to me since it allows
the user to override the definition of command in rc.conf, which seems
consistent with how we override other things.

Mark, can you revert the band-aids that I supplied previously and try
this instead?

Anyone else have a comment on this idea?


Doug


Hiroki Sato wrote:

> Author: hrs
> Date: Mon Oct  5 20:11:33 2009
> New Revision: 197790
> URL: http://svn.freebsd.org/changeset/base/197790
>
> Log:
>   Fix a case when both ${name}_program and ${command} are defined.
>  
>   Spotted by: Michio "Karl" Jinbo
>
> Modified:
>   head/etc/rc.subr
>
> Modified: head/etc/rc.subr
> ==============================================================================
> --- head/etc/rc.subr Mon Oct  5 19:56:56 2009 (r197789)
> +++ head/etc/rc.subr Mon Oct  5 20:11:33 2009 (r197790)
> @@ -602,7 +602,7 @@ run_rc_command()
>   esac
>  
>   eval _override_command=\$${name}_program
> - command=${command:-${_override_command}}
> + command=${command:+${_override_command:-$command}}
>  
>   _keywords="start stop restart rcvar $extra_commands"
>   rc_pid=
>

--

        Improve the effectiveness of your Internet presence with
        a domain name makeover!    http://SupersetSolutions.com/

_______________________________________________
freebsd-rc@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "freebsd-rc-unsubscribe@..."

[hrs]

Hi Doug,

Doug Barton <dougb@...> wrote
  in <4AD3A722.9060401@...>:

do> I think this is the wrong solution to the problem. In at least two
do> cases (routed and route6d) where $command is not defined in the rc.d
do> scripts this change is resulting in $command not being defined at all.
do>
do> If you look at the definition of the + parameter substitution this
do> makes sense:
do>
do>      ${parameter:+word}
do>              Use Alternate Value.  If parameter is unset or null, null
do>              is substituted; otherwise, the expansion of word is
do>              substituted.
do>
do> I think that what you really wanted to do was:

 I am sorry for the delay.  Your patch is reasonable to me.  This
 problem was there for a while, so it should be fixed asap.

 I noticed there was something wrong about ${name}_program but it
 seems I mistakenly changed it (sorry...).  Then I received a report
 "it does not work" so I just reverted it.

 IMO defining $command in rc.d scripts is not a good practice.
 "Always use ${name}_program and let load_rc_config() set the
 $command" would be consistent and useful to avoid this sort of
 problems.

-- Hiroki

[Doug Barton]

Hiroki Sato wrote:

> Hi Doug,
>
> Doug Barton <dougb@...> wrote
>   in <4AD3A722.9060401@...>:
>
> do> I think this is the wrong solution to the problem. In at least two
> do> cases (routed and route6d) where $command is not defined in the rc.d
> do> scripts this change is resulting in $command not being defined at all.
> do>
> do> If you look at the definition of the + parameter substitution this
> do> makes sense:
> do>
> do>      ${parameter:+word}
> do>              Use Alternate Value.  If parameter is unset or null, null
> do>              is substituted; otherwise, the expansion of word is
> do>              substituted.
> do>
> do> I think that what you really wanted to do was:
>
>  I am sorry for the delay.  Your patch is reasonable to me.  This
>  problem was there for a while, so it should be fixed asap.

Ok, I've committed the fix, thanks for getting back to me.

>  I noticed there was something wrong about ${name}_program but it
>  seems I mistakenly changed it (sorry...).  Then I received a report
>  "it does not work" so I just reverted it.

Understood. I am sure you realize that it's always Ok to ask for help
here on -rc. The rc.d system is not life-threateningly complex but it
does have a lot of "behind the scenes" interactions that are not
always obvious. I certainly don't hesitate to ask for review on
changes myself and I encourage others to do so (as you have done in
the past).

FWIW, what I do object to about your changes in r197144 and r197790
are that in the first case you neglected to mention that you were
changing that part of the code, and in the second you neglected to
mention that you were changing it back to what it was before you
changed it. That made debugging this problem more difficult for me
than (I think) it should have been. You also did not mention that you
were removing $command in your changes to route[6]d, which made
debugging Mark's original complaint harder, but only for about 30
seconds or so. :)

The two most important things about VCS logs are WHAT you did (should
be brief, but thorough) and WHY you did it. My logs are often
obnoxiously long, but you can generally find at least that information
in them. Please think about what people who read the logs years from
now will need to know. Of course, I realize that this is difficult to
do, especially when you are just wrapping up a project and all of the
information is fresh in your head and seems painfully obvious.

>  IMO defining $command in rc.d scripts is not a good practice.
>  "Always use ${name}_program and let load_rc_config() set the
>  $command" would be consistent and useful to avoid this sort of
>  problems.

In the past since there was not a reliable mechanism to allow
${name}_program to override $command, and because rc.subr needs to
have $command defined to work properly (see the description in the
run_rc_command comments in rc.subr) it was necessary to set command in
each script. When yar introduced the current version of the override
code almost 4 years ago he also went through and set command
explicitly in all the scripts that didn't have it, so the situation
that was created here never came up until now.

(FYI, the previous code that he replaced had the same effect as what I
just changed it to but was slightly more complex.)

I'm sort of ambivalent about whether we need to continue encouraging
people to use command in the script or not. As long as what's in the
script matches what's in /etc/defaults/rc.conf we're not hurting
anything, although we are duplicating effort.

My preference at this point is to let the change that I just made
settle for a while, mostly to see if it has any negative interactions
with scripts from ports, then MFC it after 8.0-RELEASE along with the
changes you've made to the IPv6 stuff. After that we can start talking
about ripping command= out of the individual rc.d scripts if people
think that's a good idea.


Doug

PS, Mark you're a slacker for not getting back to me. :)

--

        Improve the effectiveness of your Internet presence with
        a domain name makeover!    http://SupersetSolutions.com/

_______________________________________________
freebsd-rc@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "freebsd-rc-unsubscribe@..."

[hrs]

Hi Doug, sorry again for the delayed response.  I have been distracted
by a busy few weeks due to my day job...

Doug Barton <dougb@...> wrote
  in <4AD7B8FA.7020703@...>:

do> >  I noticed there was something wrong about ${name}_program but it
do> >  seems I mistakenly changed it (sorry...).  Then I received a report
do> >  "it does not work" so I just reverted it.
do>
do> Understood. I am sure you realize that it's always Ok to ask for help
do> here on -rc. The rc.d system is not life-threateningly complex but it
do> does have a lot of "behind the scenes" interactions that are not
do> always obvious. I certainly don't hesitate to ask for review on
do> changes myself and I encourage others to do so (as you have done in
do> the past).
do>
do> FWIW, what I do object to about your changes in r197144 and r197790
do> are that in the first case you neglected to mention that you were
do> changing that part of the code, and in the second you neglected to
do> mention that you were changing it back to what it was before you
do> changed it. That made debugging this problem more difficult for me
do> than (I think) it should have been. You also did not mention that you
do> were removing $command in your changes to route[6]d, which made
do> debugging Mark's original complaint harder, but only for about 30
do> seconds or so. :)

 Yes, indeed.  I should have been more careful and will keep it in
 mind.  Sorry about that.

do> I'm sort of ambivalent about whether we need to continue encouraging
do> people to use command in the script or not. As long as what's in the
do> script matches what's in /etc/defaults/rc.conf we're not hurting
do> anything, although we are duplicating effort.
do>
do> My preference at this point is to let the change that I just made
do> settle for a while, mostly to see if it has any negative interactions
do> with scripts from ports, then MFC it after 8.0-RELEASE along with the
do> changes you've made to the IPv6 stuff. After that we can start talking
do> about ripping command= out of the individual rc.d scripts if people
do> think that's a good idea.

 Sounds reasonable to me.

-- Hiroki

[Doug Barton]

Hiroki Sato wrote:
> Hi Doug, sorry again for the delayed response.  I have been distracted
> by a busy few weeks due to my day job...

No problem, I know the feeling. :)

> Doug Barton <dougb@...> wrote
>   in <4AD7B8FA.7020703@...>:
>
> do> >  I noticed there was something wrong about ${name}_program but it
> do> >  seems I mistakenly changed it (sorry...).  Then I received a report
> do> >  "it does not work" so I just reverted it.
> do>
> do> Understood. I am sure you realize that it's always Ok to ask for help
> do> here on -rc. The rc.d system is not life-threateningly complex but it
> do> does have a lot of "behind the scenes" interactions that are not
> do> always obvious. I certainly don't hesitate to ask for review on
> do> changes myself and I encourage others to do so (as you have done in
> do> the past).
> do>
> do> FWIW, what I do object to about your changes in r197144 and r197790
> do> are that in the first case you neglected to mention that you were
> do> changing that part of the code, and in the second you neglected to
> do> mention that you were changing it back to what it was before you
> do> changed it. That made debugging this problem more difficult for me
> do> than (I think) it should have been. You also did not mention that you
> do> were removing $command in your changes to route[6]d, which made
> do> debugging Mark's original complaint harder, but only for about 30
> do> seconds or so. :)
>
>  Yes, indeed.  I should have been more careful and will keep it in
>  mind.  Sorry about that.

These things happen, I'm not really that concerned about it. The whole
"keep in mind how people will read this commit message years from now"
thing is something that I have to constantly remind myself of as well.

> do> I'm sort of ambivalent about whether we need to continue encouraging
> do> people to use command in the script or not. As long as what's in the
> do> script matches what's in /etc/defaults/rc.conf we're not hurting
> do> anything, although we are duplicating effort.
> do>
> do> My preference at this point is to let the change that I just made
> do> settle for a while, mostly to see if it has any negative interactions
> do> with scripts from ports, then MFC it after 8.0-RELEASE along with the
> do> changes you've made to the IPv6 stuff. After that we can start talking
> do> about ripping command= out of the individual rc.d scripts if people
> do> think that's a good idea.
>
>  Sounds reasonable to me.

Given that no one has reported any problems with this, and that it
restores the code to what was happening prior to yar's command= fixes
I decided to ask for MFC permission but given the impending RC2 the
request is on hold. My feelings won't be hurt if it doesn't get
approved for 8.0-release though, so I'm happy either way.

FYI, I'm currently in the process of cleaning things up locally so
that I'll have time to give more review to your changes over the last
couple months. I'm seeing some weirdness with my wireless interface
that I think we can probably nail down without too much effort. At
this point I'm not actually sure that it is related to your changes in
any case.


hth,

Doug

--

        Improve the effectiveness of your Internet presence with
        a domain name makeover!    http://SupersetSolutions.com/

_______________________________________________
freebsd-rc@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-rc
To unsubscribe, send any mail to "freebsd-rc-unsubscribe@..."