> > + > > +# the following is the LSB init header see > > +# http://www.linux-foundation.org/spec//booksets/LSB-Core-generic/LSB-Core-generic.html#INITSCRCOMCONV > > 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-generic/iniscrptact.html 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list