[Bug 199127] rc.d/ntpd: user-set ntpd_flags stomps over rc-defined ones (pidfile, driftfile) (fwd)

[Rodney W. Grimes]

> Hi Rod,
> 
> [Please copy rc@ in for discussion]

Added, seemed to be missing from your reply to me.
And please keep me in CC: as I am not on that mail list.


> Rodney W. Grimes wrote:
> > ----- Forwarded message from bugzilla-noreply at freebsd.org -----
> >
> > Delivered-To: freebsd-rc at mailman.ysv.freebsd.org
> > From: bugzilla-noreply at freebsd.org
> > To: freebsd-rc at freebsd.org
> > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=199127
> >
> > Chris Rees <crees at FreeBSD.org> changed:
> >
> >             What    |Removed                     |Added
> > ----------------------------------------------------------------------------
> >                   CC|                            |crees at FreeBSD.org
> >
> > --- Comment #2 from Chris Rees <crees at FreeBSD.org> ---
> > Created attachment 185920
> >    --> https://bugs.freebsd.org/bugzilla/attachment.cgi?id=185920&action=edit
> > Use command_args not rc_flags to avoid rc.conf messing up configuration
> >
> > I've come across this bug too, but only realised after seeing this one :)
> >
> > On my machine the pidfile doesn't appear because I've overridden ntpd_flags in
> > rc.conf.
> >
> > The correct use is to have command_args for non-configurable stuff, not (ab)use
> > ${name}_flags and hope no-one changes them!
> >
> > Please would someone approve me to commit this?
> >
> > Cheers!
> >
> > Chris
> >
> RG> I actually think this is pilot error, when you override any *_flags
> 
> RG> in /etc/rc.conf you must take into consideration all things that
> RG> the defaults did.
> 
> RG>For ntpd_flags: ntpd_flags="-p /var/run/ntpd.pid -f /var/db/ntpd.drift"
> RG>If you override that, and want -p and -f args you should be setting
> RG>the correct way to do this for your example rc.conf would be:
> RG>ntpd_enable="yes"
> RG>ntpd_config="/conf/ME/ntp.conf"
> RG>ntpd_sync_on_start="yes"
> RG>ntpd_flags="-p /var/run/ntpd.pid -f ${ntpd_config} -4"
> 
> RG>Your patch would actually hardcode ntpd_flags into the
> RG>rc.d file which would be a very bad idea.
> 
> Sorry, but I have to disagree here.  ntpd is the only file I
> can see that sets any flags in defaults/rc.conf that are actually
> unnecessary to configure.  In fact, changing the argument to -p
> completely breaks the rc script and makes it unable to stop ntpd.
> This can't possibly by pilot error, more code error.

Ok, I can see the issue with -p being non trivial and probably
an extreme case for anyone wanting to mess with.  I agree that
-p should be removed from ntpd_flags in /etc/defaults/rc.conf
and /etc/rc.d/ntpd

It may be desirable at some time to add routines to parse a -p
or -P out of a _flags so that these cases can be handled, but
at this point in time that is far beyond the issues here.

My original point that if your are going to overide a _flags
from /etc/defaults/rc.conf it is your responsibility to copy
the current value being set from the defaults and modify it
to suit your needs.  Expecting _flags to actually be an
_addflags is a bad assumption the user has made in this
bug report.  My above recomended settings to /etc/rc.conf
would infact eleviate the problem the original bug submitter
had.

> If you look down defaults/rc.conf, nearly all the _flags
> values are empty by default, or have a couple of *configurable*
> defaults.  Perhaps you could argue that the placement of the
> database file should be configurable (I'd suggest not as it's
> pointless), but you can't  possibly say that an rc.conf setting
> should easily be able to break the script?

Then we shall disagree on that point, if it was so pointless to allow
altering the location of the "database"(sic), I think you mean config file,
then why does the -f option even exist for ntpd?

> 
> This is what the variable command_args is for, and why I suggest using it! 
> No one needs to mess about with pidfiles in rc.conf.
I think that is a fairly reasonble view, but I would change the wording
from "No one" to "Extermly rare"

> Most other scripts do the right thing:
> 
> [crees at zeus]/etc/rc.d% grep command_args * | grep pidfile
> bsnmpd:command_args="-p ${pidfile}"
Agreeable with that.
> bthidd:command_args="-c ${config} -H ${hids} -p ${pidfile}"

I am not sure I see that adding 2 variables to /etc/defaults/rc.conf
is any better than a properly crafted bthidd_flags variable.

What your advocating would be having every possible option
to every command be listed as a variable in /etc/defaults/rc.conf
and removing all the _flags.  I am not sure this is the right
path to go down.

I do concide that -p and -P are probably easier to just
deal with for now, but I can not agree that we need
_config _hids _etc as has started to happen.

> hostapd:command_args="-P ${pidfile} -B ${conf_file}"
> wpa_supplicant:command_args="-B -i $ifn -c $conf_file -D $driver -P $pidfile"
I wont even start on how broekn wpa_supplicant's rc files are....

> 
> [crees at zeus]/etc/rc.d% grep _flags /etc/defaults/rc.conf | grep pid
> pppoed_flags="-P /var/run/pppoed.pid"	# Flags to pppoed (if enabled).
> ntpd_flags="-p /var/run/ntpd.pid -f /var/db/ntpd.drift"
> 
> I think pppoed is also wrong.

What makes it wrong?  Your unwillingness to copy the _flags from
/etc/defaults/rc.conf into your /etc/rc.conf and edit what it is
you want to add/change/remove?  Yes, you can break things, but
unix has always allowed one to walk around with a fairly full
load of bullets in the toe gun.

PS:  The patch as provided appears to totally remove ntpd_flags
from /etc/defaults/rc.conf, that is an error that must be corrected.

-- 
Rod Grimes                                                 rgrimes at freebsd.org