On Thu, Dec 07, 2006 at 04:10:06PM +0100, Bas van Schaik wrote:
> Just like stated in the TODO.Debian file: the package is missing an
> initscript to automatically export devices at boot time.
Some comments:
> #!/bin/sh
>
> # Start or stop one or more vblade-processes, including a logger process
> #
> # Bas van Schaik <[EMAIL PROTECTED]>
> # Inspired by the init.d script from the Postfix package
>
No set -e?
> PATH=/bin:/usr/bin:/sbin:/usr/sbin
> DAEMON=/usr/sbin/vblade
> NAME=vbladed
> CONFIGFILE=/etc/vbladed.conf
It's more like a table of blade devices, much like fstab, crypttab,
raidtab, etc. Maybe calling it vbladetab would make more sense,
consistency-wise?
> PIDSFILE=/var/run/vbladed.pids
>
> test -f /etc/default/vbladed && . /etc/default/vbladed # include
> /etc/default/vbladed if it exists
Is that really needed? By default vblade can ship with an empty vblade
config, and then is already no-action. Having yet another config file to
change if you want to start using it seems a bit pointless here.
> test -f $CONFIGFILE || exit 0 # exit if configfile doesn't exist
> test -x $DAEMON || exit 0 # exit if daemon is not installed
>
> if [ -f /etc/default/vbladed ]; then
> . /etc/default/vbladed
This is the second time you include /etc/default/vbladed, anyway.
> if [ ! "$ENABLE_VBLADED" = "Y" ] && [ ! "$ENABLE_VBLADED" = "y" ]; then
> # vbladed disabled in /etc/default/vbladed
> exit 0
> fi
> fi
>
> # Expects a file which will return a normalized version of the configfile
> # (i.e. without comments and with double spaces trimmed out)
> function normalizeConfig(){
> configfile="$1"
> if [ ! -r "$configfile" ]; then
> return
> fi
>
> cat "$configfile" | while read ln; do
> ln=`echo $ln | grep -v "^#" | tr -s " " | cut -d "#" -f 1`
> if [ ! "$ln" = "" ]; then
> echo $ln
> fi
> done
> }
>
> case "$1" in
> start)
> echo "Starting vblade daemon(s): "
>
> # parse /etc/vbladed.conf
> counter="0"
>
> normalizeConfig /etc/vbladed.conf | while read ln; do
> eth=`echo $ln | cut -d " " -f 1`
> shelf=`echo $ln | cut -d " " -f 2`
> slot=`echo $ln | cut -d " " -f 3`
> bdevice=`echo $ln | cut -d " " -f 4`
Why not 'read eth shelf slot device', which does basicly the right thing
already?
> if [ "$eth" = "" ] || [ "$shelf" = "" ] || [ "$slot" = "" ] ||
> [ "$bdevice" = "" ]; then
> echo " malformed configuration line: \"$ln\""
Maybe you want to bail out with an error at this time?
> else
> echo -n " $bdevice ($eth, shelf $shelf, slot $slot): "
>
> # Create temporary directory with named pipe
> npipedir=`mktemp -d`
> npipe="$npipedir/temp_pipe"
> mkfifo $npipe
>
> # launchDaemon $shelf $slot $eth $bdevice | logger -t
> "vbladed[$pid]" & ## werkt
>
> (
> $DAEMON $shelf $slot $eth $bdevice 2>&1 >
> $npipe &
This will not do what you expect, it'll *not* redirect stderr to $npipe,
but instead leaves it at what stdout used to point to (you're copying fd
1 to postion 2 before you've pointed fd 1 to $npipe).
> pid=$!
>
> logger -t "vbladed[$pid]" -t "vbladed[$pid]" <
> $npipe &
Duplicate -t?
> echo $pid >> $PIDSFILE
You do not actually clear the $PIDSFILE at the beginning of start, nor
do you guard for starting twice. Perhaps it'd be best to have one
pidfile for each vblade instance, by systematically name the .pid-files
like /var/run/vblade.$eth.$shelf.$slot.pid ? Then you can here detect
that such pid file already exists and say "not starting because already
started".
> # It's safe to remove the temporary directory
> now
> rm -r "$npipedir"
> ) && echo "OK" || echo "FAILED"
The exit status of the (...) is determined by the last command, here 'rm
-r'. This is not really representing whether the vblade daemon was
succesfully launched. Also, because this happens in a loop for each
vblade instance, it might be more insightful for the admin to name the
bdevice that's being exported each time that one blade is started, or
similar (see Debian Policy 9.4, "If the script starts one or more
daemons (...)", http://www.debian.org/doc/debian-policy/ch-opersys.html#s9.4)
> fi
> done
> ;;
>
> stop)
> # Read file with PIDs
> echo -n "Stopping vblade daemon(s): "
>
> if [ ! -r "$PIDSFILE" ]; then
> echo "not running?"
> else
> pids=`cat $PIDSFILE`
> for pid in $pids; do
> echo -n "$pid "
> kill $pid &> /dev/null
> done
> echo ""
> sleep 1 && killall -9 $pids &> /dev/null
>
> rm "$PIDSFILE"
> fi
> ;;
>
> restart)
> $0 stop || true
> $0 start
> ;;
>
> *)
You seem to miss the (required by policy) force-reload target.
> echo "Usage: /etc/init.d/vbladed {start|stop|restart}"
> exit 1
> ;;
> esac
>
> exit 0
> ## Configuration file for /etc/init.d/vbladed
> ## Also checkout /etc/default/vbladed
>
> ##ETH SHELF BLADE DEVICE OR FILE
In all the other documentation the term 'slot' is used instead of
'blade', for consistency, I'd suggest also here calling it 'slot'.
--Jeroen
--
Jeroen van Wolffelaar
[EMAIL PROTECTED] (also for Jabber & MSN; ICQ: 33944357)
http://Jeroen.A-Eskwadraat.nl
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
