Re: [libvirt] [PATCH] Init script for suspending/resuming guests on shutdown/boot
> +
> +# the following is the LSB init header see
> +#
http://www.linux-foundation.org/spec//booksets/LSB-Core-generic/LSB-Core-...
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-gen...
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