+ +# the following is the LSB init header see +# http://www.linux-foundation.org/spec//booksets/LSB-Core-generic/LSB-Core-gen...
Is the second // necessary, or can it just be spec/booksets? For that matter, that URL didn't work for me; I found:
http://refspecs.freestandards.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generi...
Hmm, just copied from libvirtd.init.in, better remove it completely I guess.
+sysconfdir=@sysconfdir@ +localstatedir=@localstatedir@ + +# Source function library. +. $sysconfdir/rc.d/init.d/functions
Technically, it would be safer to quote $sysconfdir, even if in practice, it never contains whitespace.
Done.
+ +URIS=default + +test -f $sysconfdir/sysconfig/libvirt-guests && . $sysconfdir/sysconfig/libvirt-guests
Likewise. Also, should we fail if sourcing the config file failed?
Hmm, not sure what's the policy for this. libvirtd.init.in does exactly this.
+run_virsh() { + uri=$1 + shift + + if [ "x$uri" = xdefault ]; then + conn= + else + conn="-c $uri" + fi + + virsh $conn "$@"
Fails if $uri contains spaces, but that's not a valid URI in the first place, so no big deal.
Yes, exactly. Also we have a space-separated list of URIs in URIS...
+} + +run_virsh_c() { + ( export LC_ALL=C; run_virsh "$@" ) +} + +list_guests() { + uri=$1 + + list=`run_virsh_c $uri list`
While bashisms in init scripts is questionable, use of POSIX is not; you can safely use $() instead of `` for readability.
Cool, I wasn't sure about this one... I also prefer $().
+ if [ $? -ne 0 ]; then + RETVAL=1 + return 1 + fi + + for id in `echo "$list" | awk 'NR > 2 {print $1}'`; do + run_virsh_c $uri dominfo $id | awk '/^UUID:/{print $2}' + done
Failure to run virsh doesn't affect our exit status?
In v2 it does.
+guest_is_on() { + uri=$1 + uuid=$2 + + id=`run_virsh_c $uri dominfo $uuid 2>/dev/null | \ + awk '/^Id:/{print $2}'` + + [ -n "$id" ] && ! [ "x$id" = x- ]
If we were worried about portability to non-POSIX, I would have written this as '[ "x$id" != x- ]' rather than '! [ "x$id" = x- ]', but either way works here since we assume POSIX.
OK, fixed. I actually prefer the != way.
This fails if $LISTFILE does not exist, which will be the case if you run start() twice in a row. But LSB requires that a start() on an already started service be a successful no-op. Maybe all you need is a line at the start of the function: test -f $LISTFILE || return 0
Fixed.
+stop() { + >$LISTFILE
Bash-ism; trivial to use ': >$LISTFILE' instead to be portable to POSIX.
Fixed.
+ for uri in $URIS; do + echo -n $"Running guests on $uri URI: " + list=`list_guests $uri` + if [ $? -eq 0 ]; then + empty=true + for uuid in $list; do + $empty || echo -n ", "
Another echo -n. Unlike the $"" case (where the style is pervasive), here, I'd like to see the use of 'printf ", "'.
OK, if it makes you feel better ;-)
+ kill -0 $virsh_pid >&/dev/null || break + progress=`run_virsh_c $uri domjobinfo $guest 2>/dev/null | \ + awk '/^Data processed:/{print $3, $4}'` + if [ -n "$progress" ]; then + printf '\r%s%12s ' "$label" "$progress" + else + printf '\r%s%-12s ' "$label" "..." + fi
Don't you also need to print the ANSI sequence for clear-to-eol, so that a shorter line overwriting a previous longer line doesn't leave garbage from the longer line?
Hmm, not sure how well it would work with various terminals and serial consoles. I chose enough space so that it shouldn't happen.
Missing restart, force-reload, and status actions, per the LSB. Also, status should be logged prior to exit, with log_*_msg.
Fixed. Thanks for reviewing, I'll send v2 with the fixes and enhancements later once I finish some testing. Jirka