rc-script review request

[Garrett Cooper]

On Wed, Nov 30, 2011 at 3:38 PM, Doug Barton <dougb at 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="${name}_start"
>>>
>>> over
>>>
>>> start_cmd="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:=%%PREFIX%%}
> (assuming that /usr/local is the default
>
> Then you need an additional function:
>
> zope213_check_instances () {
>        cmd="$1"
>        shift
>
>        if [ -n "$@" ]; then
>                zope213_instances="$@"
>        elif [ -z "$zope213_instances" ]; then
>                err 1 "No value for zope213_instances, so nothing to do"
>        fi
> }
>
> 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