[libvirt] [PATCH] Init script for suspending/resuming guests on shutdown/boot

[Jiri Denemark]

> > +
> > +# 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