Date: Sat, 16 Feb 2013 23:54:40 -0800 From: Xin Li <delphij@delphij.net> To: Chris Rees <crees@FreeBSD.org> Cc: Jeremy Chadwick <jdc@koitsu.org>, FreeBSD <freebsd-stable@freebsd.org>, Boris Samorodov <bsam@passap.ru> Subject: Re: some issues with /usr/sbin/service Message-ID: <51208CC0.8060104@delphij.net> In-Reply-To: <CADLo83-yq-2PhAoTjaNSKGLnP1djpFXZvv3BN%2BUOP030bGNe9g@mail.gmail.com> References: <511E0D43.6070900@dssgmbh.de> <20130215105710.GA6130@icarus.home.lan> <20130215193210.GB85777@in-addr.com> <20130215212020.GA17516@icarus.home.lan> <20130215213257.GA20155@icarus.home.lan> <511F4205.50509@passap.ru> <20130216092133.GA31449@icarus.home.lan> <1FD54589-7C32-47B5-A400-02FEE1459B02@gromit.dlib.vt.edu> <CADLo83972a=yv_dGNazNywFpF22vJGVG_sMVYVDAMM7BnQXwpA@mail.gmail.com> <20130216180859.GF85777@in-addr.com> <CADLo83-yq-2PhAoTjaNSKGLnP1djpFXZvv3BN%2BUOP030bGNe9g@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
On 2/16/13 10:24 AM, Chris Rees wrote:
> On 16 February 2013 18:08, Gary Palmer <gpalmer@freebsd.org>
> wrote:
>> On Sat, Feb 16, 2013 at 05:38:56PM +0000, Chris Rees wrote:
>>> On 16 February 2013 17:05, Paul Mather
>>> <paul@gromit.dlib.vt.edu> wrote:
>>>> On Feb 16, 2013, at 4:21 AM, Jeremy Chadwick wrote:
>>>>
>>>>> On Sat, Feb 16, 2013 at 12:23:33PM +0400, Boris Samorodov
>>>>> wrote:
>>>>>> 16.02.2013 01:32, Jeremy Chadwick ??????????:
>>>>>>
>>>>>>> Follow up -- I read Alfred's most recent mail. Lo and
>>>>>>> behold, I find this in /var/log/messages (but such did
>>>>>>> not come to my terminal):
>>>>>>>
>>>>>>> Feb 15 13:26:20 icarus jdc: /usr/sbin/service: WARNING:
>>>>>>> $svnserve_enable is not set properly - see rc.conf(5).
>>>>>>> Feb 15 13:26:20 icarus jdc: /usr/sbin/service: WARNING:
>>>>>>> $smartd_enable is not set properly - see rc.conf(5).
>>>>>>> Feb 15 13:26:20 icarus jdc: /usr/sbin/service: WARNING:
>>>>>>> $rsyncd_enable is not set properly - see rc.conf(5).
>>>>>>> Feb 15 13:26:20 icarus jdc: /usr/sbin/service: WARNING:
>>>>>>> $htcacheclean_enable is not set properly - see
>>>>>>> rc.conf(5). Feb 15 13:26:20 icarus jdc:
>>>>>>> /usr/sbin/service: WARNING: $fetchmail_enable is not
>>>>>>> set properly - see rc.conf(5).
>>>>>>>
>>>>>>> Cute. Agreed -- this is unacceptable on two levels (as
>>>>>>> I see it):
>>>>>>>
>>>>>>> 1) These messages should be going to stdout or stderr
>>>>>>> in some way, so honestly logger(8) should be called
>>>>>>> with the "-s" flag (IMO).
>>>>>>
>>>>>> Fully agreed here.
>>>>>
>>>>> It turns out logger -s has no effect, just like how the
>>>>> echo 1>&2 statements in warn() and err() have no effect
>>>>> either (these should be outputting the warnings in question
>>>>> to stderr) -- see rc.subr's source for what I'm referring
>>>>> to.
>>>>>
>>>>> Gary and I have been discussing this off-list and the
>>>>> reason has been found: service(8) has this code in it:
>>>>>
>>>>> checkyesno $rcvar 2>/dev/null && echo $file
>>>>>
>>>>> This explains why there's no warn() or err() output on the
>>>>> terminal -- it's being redirected to /dev/null prior.
>>>>>
>>>>> I do not know who maintains the rc(8) and rc.subr(8)
>>>>> framework, but they've got their work cut out for them.
>>>>>
>>>>> (Note: the echo statements in warn() and err() could be
>>>>> replaced with "logger -s" as I said; this would allow the
>>>>> "echo 1>&2" to be removed)
>>>>>
>>>>>>> 2) These messages should not be displayed at all (i.e.
>>>>>>> lack of an xxx_enable variable should imply
>>>>>>> xxx_enable="no").
>>>>>>
>>>>>> I see this message as one more level of supervision.
>>>>>>
>>>>>> If undefined at /etc/make.conf the value of xxx_enable is
>>>>>> "no" from the system's POV (i.e. the service is not
>>>>>> strarted). From the admininstrators's POV the port was
>>>>>> installed BUT is not used. It's up to admininstrator
>>>>>> whether it's OK or not -- just let him remind.
>>>>>
>>>>> I believe the point you're trying to make is that the
>>>>> warning in question should 'act as a reminder to the
>>>>> administrator that they need to set xxx_enable="yes" in
>>>>> rc.conf'.
>>>>>
>>>>> If not: please explain if you could what you mean, because
>>>>> I don't understand.
>>>>>
>>>>> If so: I strongly disagree with this method of approach, as
>>>>> what you've proposed is a borderline straw man argument.
>>>>>
>>>>> Reminding the admin to set xxx_enable is presently done
>>>>> inside most ports' pkg-message. IMO, this should really be
>>>>> done inside bsd.port.mk when USE_RC_SUBR is used, emitting
>>>>> a message during install that says something like:
>>>>>
>>>>> To enable the xxx service, please add the following to
>>>>> /etc/rc.conf: xxx_enable="yes"
>>>>>
>>>>> Of course, I don't know if this would work for packages.
>>>>>
>>>>> The current message for <missing xxx_enable in rc.conf> is
>>>>> this:
>>>>>
>>>>> WARNING: $xxx_enable is not set properly - see rc.conf(5).
>>>>>
>>>>> The message is entirely misleading for this specific
>>>>> situation; it isn't "reminding" an administrator -- if
>>>>> anything it's confusing them (thread is case in point). If
>>>>> we're going to cater to ignorance, then the message should
>>>>> reflect the situation.
>>>>>
>>>>> Thus IMO, this is what ***should*** happen:
>>>>>
>>>>> Definition in rc.conf Behaviour/result
>>>>> -----------------------
>>>>> -------------------------------------------
>>>>> myprog_enable="yes" emit no warnings, service should
>>>>> run myprog_enable="no" emit no warnings, service
>>>>> should not run myprog_enable="abc123" emit a warning,
>>>>> service should not run <no definition> emit no
>>>>> warnings, service should not run
>>>>
>>>>
>>>> I think case 4 ("<no definition>") is a case where a warning
>>>> should be emitted because it is arguably not immediately
>>>> apparent what will actually happen if no definition is
>>>> present. In the case of services in the base OS it is
>>>> well-defined: every service should have an explicit default
>>>> in /etc/defaults/rc.conf that you can easily consult to know
>>>> definitively what will happen with that service. (If it
>>>> doesn't, that is a bug, IMHO.)
>>>>
>>>> For ports, the case is not so clear. There is a general
>>>> trend for the port rc.d script to default its respective
>>>> xxx_enable explicitly to "NO". But it is not a universal
>>>> rule that "no definition" = default to "NO". The
>>>> net/avahi-app port, for example, doesn't default to "NO" if
>>>> xxx_enable is not set: it defaults to whatever the
>>>> gnome_enable setting is defined to be.
>>>
>>> With few exceptions, it should be considered a rule that ports
>>> rc scripts contain:
>>>
>>> : ${xxx_enable=no}
>>>
>>> to avoid this. If you see any ports that don't define the
>>> _enable variable at all, they are wrong and need fixing.
>>
>> Except the 'service' command doesn't parse the individual ports
>> rc.d script so the default is never found. It relies purely on
>> rc.conf and rc.conf.local settings.
>>
>> So no matter if the port has
>>
>> : ${xxx_enable=no}
>>
>> or not, 'service -e' will still spit out the warnings
>>
>> Not loading the individual ports files is probably done for two
>> reasons I can think of - performance, and safety (in case the
>> rc.d file is bad for some reason). However it leads to these
>> rather irritating warnings, especially when you find them later
>> and you don't know what you did to cause them.
>>
>> I was away to suggest that having /usr/local/etc/rc.conf.d would
>> help as ports could then specify their defaults safely without
>> having to do file rewrites, however that appears to be only
>> looked at in load_rc_config(), and 'service -e' bypasses that as
>> well by doing
>>
>> load_rc_config 'XXX'
>>
>> so only /usr/local/etc/rc.conf.d/xxx would be looked at.
>
> Yes, OK. A variation on Alfred's checkyes patch is at [1].
>
> I have not made it into a function deliberately because this is
> already rc idiom as used in other rc scripts where a warning is
> not necessary, and also because it is very rarely the right thing
> to do (i.e. it would be tempting to overuse it if it were a
> function).
Actually, I think the approach used in service(8) is bogus. It seems
that it's intended to be fast?
I *think* the code should be actually changed in a way like this:
Index: service.sh
===================================================================
- --- service.sh (revision 246844)
+++ service.sh (working copy)
@@ -69,15 +69,15 @@ if [ -n "$RESTART" ]; then
for file in `reverse_list ${files}`; do
if grep -q ^rcvar $file; then
- - eval `grep ^name= $file`
eval `grep ^rcvar $file`
+ eval `$file rcvar | grep ^${rcvar}`
checkyesno $rcvar 2>/dev/null && run_rc_script
${file} stop
fi
done
for file in $files; do
if grep -q ^rcvar $file; then
- - eval `grep ^name= $file`
eval `grep ^rcvar $file`
+ eval `$file rcvar | grep ^${rcvar}`
checkyesno $rcvar 2>/dev/null && run_rc_script
${file} start
fi
done
@@ -98,8 +98,8 @@ fi
if [ -n "$ENABLED" ]; then
for file in $files; do
if grep -q ^rcvar $file; then
- - eval `grep ^name= $file`
eval `grep ^rcvar $file`
+ eval `$file rcvar | grep ^${rcvar}`
checkyesno $rcvar 2>/dev/null && echo $file
fi
done
However, that this would reveal some issues with the existing rc.d
scripts, e.g. some scripts execute commands regardless it's start,
stop or rcvar (securelevel come to mind; another problem is that
rc.subr checks pid when it doesn't need to do so, e.g. when doing
rcvar), and that should be fixed first in my opinion.
Cheers,
-----BEGIN PGP SIGNATURE-----
iQEcBAEBCAAGBQJRIIzAAAoJEG80Jeu8UPuzC8MIALcFCEXOWwqvqFbTJTphpSwx
2i28r829yoi3cwI1RYByfGBnUfscCGFXEUd13pUvx1vx/1h4kKipQAsS0jiIg2a9
gchtleoUGmw4XN4TNQJfUuyh2eM/yJH1EV7PwvKWhOHJ+iyvYqA9pust2ABxT0Dt
n90ioqMsGI7r7qFt8QBrUE0gfR2mBFKCsV1VFFujC20mrJEJqKDLlfJHA61g1VMP
O+6BexKydN3k/j4/KX1PYJ54KLu2nCJHXFp0SWwVMp8F4s0Ln+lg2C7sE4lSMI8f
UJ2I90ZynNS7uBr03/7x//AMCcsa2tZoddXnPQWf1T2JxaZzk31+msRcYsvsjoo=
=3ewP
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?51208CC0.8060104>