rc.subr, 1.34.2.22, breaks amd_map_program="ypcat -k amd.master" in RELENG_6

[John E Hein]

Mike Makonnen wrote at 11:51 +0300 on Oct 19, 2007:
 > On Thu, Oct 18, 2007 at 12:59:48PM -0600, John E Hein wrote:
 > > The new run_rc_doit() function does this:
 > > 
 > > +       eval "$@"
 > > 
 > > If you remove the quotes, it starts working again.
 > > 
 > > Unfortunately, you can have different problems if you don't quote $@
 > > (for instance, args with spaces become two args).  I don't know if
 > > yar@ added that to address a specific issue or just to be more
 > > future-proof in terms of quoting.  But it breaks if the command
 > > or args have a newline.
 > > 
 > > Maybe it's best to just do the echo in /etc/rc.d/amd that I showed in
 > > the previous email and document that $rc_flags and $command, etc., should
 > > not have newlines.  Here's that patch again:
 > 
 > I agree.
 > I think that modifying rc.d/amd is the proper solution here. It's probably
 > a bug that it depends on run_rc_command()'s internal behaviour.

I agree - it'd be best if run_rc_command could handle whatever you
throw at it, but I'm not sure that's practical.

If run_rc_command can't have newlines in the vars it uses as
arguments, then one way to handle the bug is just to document it
clearly in rc.subr (with the rc_flags and command_args comments).

A quick glance shows that the /etc/rc.d/syslogd script is the only(?)
one that does something similar.  But it takes pains to convert
newlines to spaces using tr(1) (although I think it's unnecessary to
do so in a for loop - so, IMO, that tr(1) is superfluous and could be
removed for the efficiency conscious - not tested ;).


 > Can you try the following slightly different patch?
 >
 > Index: etc/rc.d/amd
 > ===================================================================
 > RCS file: /home/ncvs/src/etc/rc.d/amd,v
 > retrieving revision 1.18
 > diff -u -r1.18 amd
 > --- etc/rc.d/amd	18 Oct 2006 15:56:11 -0000	1.18
 > +++ etc/rc.d/amd	19 Oct 2007 08:35:36 -0000
 > @@ -34,7 +34,7 @@
 >  	[Nn][Oo] | '')
 >  		;;
 >  	*)
 > -		rc_flags="${rc_flags} `eval ${amd_map_program}`"
 > +		rc_flags="${rc_flags} `echo $(eval ${amd_map_program})`"
 >  		;;
 >  	esac
 >  
 > @@ -46,7 +46,8 @@
 >  		fi
 >  		;;
 >  	*)
 > -		rc_flags="-p ${rc_flags} > /var/run/amd.pid 2> /dev/null"
 > +		rc_flags="-p ${rc_flags}"
 > +		command_args=" > /var/run/amd.pid 2> /dev/null"
 >  		;;
 >  	esac
 >  	return 0

That works fine, too (don't need the space in front of '>'), but I
don't see the benefit to splitting it into two variables/lines.  Is it
just a preferred style issue?  If so, please commit that separately or
note it in the commit message so as not to confuse the issue for
future cvs miners.

Thanks for taking a look.  If you want me to send-pr this, just let me
know.