rc-script review request

[Ruslan Mahmatkhanov]

Doug Barton wrote on 01.12.2011 03:38:
> On 11/30/2011 10:53, Ruslan Mahmatkhanov wrote:
>
>> 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?

Yes, zopectl actually handling that - it drops privileges to user that's
defined in ${instance}/etc/zope.conf (default `www'). I changed REQUIRE
to LOGIN though.

>> 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()

I merged all the changes from you except this part:

 cmd="$1"
-[ $# -gt 0 ] && shift
-[ -n  "$*" ] && zope213_instances="$*"
+shift
+zope213_instances="$@"

Dunno why, but it didn't working - service just doesn't starting. To be
honest, i'm half-understand your changes because i mooched
shell-scripting lessons in the school.

> 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

I understand what you talking about, and i'm agree that this is not much
correct to not have default value for an instance. I'll try to follow
you suggestions and kode to implement this behaviour, thank you!

> 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. :)
> 
> 
> hth,
> 
> Doug
> 


-- 
Regards,
Ruslan

Tinderboxing kills... the drives.