Date: Wed, 30 Nov 2011 17:14:33 -0800 From: Garrett Cooper <yanegomi@gmail.com> To: Doug Barton <dougb@freebsd.org> Cc: freebsd-rc@freebsd.org, Ruslan Mahmatkhanov <cvs-src@yandex.ru> Subject: Re: rc-script review request Message-ID: <CAGH67wSwdbmsYQeeBbqATwx-WhMxNugVqY-cUo%2BEzxOz=N830A@mail.gmail.com> In-Reply-To: <4ED6BE87.4060408@FreeBSD.org> References: <4ED66DCB.1040102@yandex.ru> <CADLo83-RR945MKbdvpdghHsHpO1_MA4OA21WkA_3xCTjOOhDsQ@mail.gmail.com> <4ED67B8F.50109@yandex.ru> <4ED6BE87.4060408@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Nov 30, 2011 at 3:38 PM, Doug Barton <dougb@freebsd.org> wrote:
> On 11/30/2011 10:53, Ruslan Mahmatkhanov wrote:
>> Chris Rees wrote on 30.11.2011 22:15:
>
>>> I could get yelled at for this, but normally I'd prefer:
>>
>> Please yell, i'm not experienced in rc at all, so i'll be glad any
>> guidance (in any form) to raise it (experience) :). But i thought that
>> it's safe to use existing scripts from the tree.
>
> Ruslan, the problem is that there are a lot of bad examples already in
> the tree. :)
>
>>> start_cmd=3D"${name}_start"
>>>
>>> over
>>>
>>> start_cmd=3D"zope213_start".
>
> Yes, using variables where it's clear what's being done is preferred,
> since that will facilitate reuse of *good* examples.
>
>> Fixed, thank you. I also added `KEYWORD: shutdown' per PH, because of
>> zope starting under non-root user.
>
> You use 'shutdown' because it starts a persistent service, and we want
> to shut those down cleanly and in order. If the service runs under a
> non-root user it needs REQUIRE: LOGIN instead of DAEMON. However, I
> don't see that it runs as a non-root user, unless zopectl handles that
> for you?
>
>> Is there still any problems in the script?
>
> 1. Always use tabs
> 2. Make the start/stop/restart printouts fit rc.d style a little more
> 3. Simplify the shell code for dealing with command line arguments
> 4. $@ should be used there instead of $* because the former treats the
> elements as discrete, which is what you want to feed a for loop.
> 5. Move the default for _enable up to where we like it to be.
> 6. Localize the variable in zope213ctl()
>
> But there is a more fundamental problem. You seem to be requiring the
> user to supply an instance argument for the script to work at all.
> That's contrary to how we generally do things, and I'm fairly confident
> that this is not going to work on startup.
>
> I think that what you need is to provide at least one default, so after
> the default for _enable you'd have something like this:
>
> : ${zope213_instances:=3D%%PREFIX%%}
> (assuming that /usr/local is the default
>
> Then you need an additional function:
>
> zope213_check_instances () {
> =A0 =A0 =A0 =A0cmd=3D"$1"
> =A0 =A0 =A0 =A0shift
>
> =A0 =A0 =A0 =A0if [ -n "$@" ]; then
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0zope213_instances=3D"$@"
> =A0 =A0 =A0 =A0elif [ -z "$zope213_instances" ]; then
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0err 1 "No value for zope213_instances, so =
nothing to do"
> =A0 =A0 =A0 =A0fi
> }
>
> And call that function first in each of your start/stop/restart functions=
.
>
> You should test that of course. :)
Crazy thought -- should a script be made for rc scripts, similar
to portcheck?
Thanks!
-Garrett
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGH67wSwdbmsYQeeBbqATwx-WhMxNugVqY-cUo%2BEzxOz=N830A>