rc-script review request

[Doug Barton]

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. :)


hth,

Doug

-- 

		"We could put the whole Internet into a book."
		"Too practical."

	Breadth of IT experience, and depth of knowledge in the DNS.
	Yours for the right price.  :)  http://SupersetSolutions.com/

-------------- next part --------------
#!/bin/sh
#
# Startup script for Zope server.
#
# $FreeBSD: ports/www/zope211/files/zope211.in,v 1.3 2011/05/15 02:49:17 dougb Exp $
#
# PROVIDE: zope213
# REQUIRE: DAEMON
# KEYWORD: shutdown

# Define these zope213_* variables in one of these files:
# /etc/rc.conf
# /etc/rc.conf.local
# /etc/rc.conf.d/zope213
#
# zope213_enable : bool
# Enable Zope ("YES") or not ("NO", the default).
#
# zope213_instances : list
# List of dirs with Zope's instances ("" by default).
#

. /etc/rc.subr

name="zope213"
rcvar=`set_rcvar`

load_rc_config $name

: ${zope213_enable:="NO"}

zope213ctl () {
	local instance

	for instance in $zope213_instances; do
		if [ -d ${instance} ]; then
			echo -n " Zope instance ${instance} -> "
			${instance}/bin/zopectl "$1"
		fi
	done
}

zope213_start () {
	echo -n 'Starting Zope 2.13:'
	zope213ctl "start"
	echo '.'
}

zope213_stop () {
	echo -n 'Stopping Zope 2.13:'
	zope213ctl "stop"
	echo '.'
}

zope213_restart () {
	echo -n 'Restarting Zope 2.13:'
	zope213ctl "restart"
	echo '.'
}

start_cmd="${name}_start"
stop_cmd="${name}_stop"
restart_cmd="${name}_restart"

cmd="$1"
shift
zope213_instances="$@"

run_rc_command "${cmd}"