Date: Fri, 19 Oct 2007 07:34:50 -0600 From: John E Hein <jhein@timing.com> To: Mike Makonnen <mtm@FreeBSD.Org> Cc: freebsd-rc@FreeBSD.Org Subject: Re: rc.subr, 1.34.2.22, breaks amd_map_program="ypcat -k amd.master" in RELENG_6 Message-ID: <18200.45690.542280.695857@gromit.timing.com> In-Reply-To: <20071019085154.GA3185@terra.mike.lan> References: <18199.34219.154950.645190@gromit.timing.com> <18199.44324.813707.124793@gromit.timing.com> <20071019085154.GA3185@terra.mike.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?18200.45690.542280.695857>