awk splits the line on consecutive spaces, which breaks getting the name of a domain whose name contains spaces. Use sed instead to strip the "Name:" prefix from the line Signed-off-by: Philipp Hahn <hahn(a)univention.de&gt; --- tools/libvirt-guests.init.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tools/libvirt-guests.init.sh b/tools/libvirt-guests.init.sh index 8823d06..ea2189e 100644 --- a/tools/libvirt-guests.init.sh +++ b/tools/libvirt-guests.init.sh @@ -105,7 +105,7 @@ guest_name() { uuid=$2 name=$(run_virsh_c $uri dominfo $uuid 2>/dev/null | \ - awk '/^Name:/{print $2}') + sed -ne 's/^Name: *//p') [ -n "$name" ] || name=$uuid echo "$name" -- 1.7.1

On Fri, Mar 11, 2011 at 11:38:47AM +0100, Jiri Denemark wrote: virsh already *can* start VMs based on UUID. Any virsh command which needs an inactive domain can accept either name of UUID interchangably. Any virsh command which needs an active domain can accept name, UUID or ID Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 03/01/2011 10:57 AM, Philipp Hahn wrote: ACK - this patch is independently useful from Jirka's patch to not use $name in virsh, because we also use the output of guest_name in informational messages and should not truncate there. I've pushed it. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 03/09/2011 01:54 AM, Philipp Hahn wrote: Hmm. Valid URIs can contain '?', which _is_ a glob character, so I agree that we need to quote "$uri" everywhere that we have already correctly obtatined it in order to avoid inadvertent globbing. Meanwhile, $URIS is indeed a user-settable variable, via /etc/sysconfig/libvirt-guests, so we should want to protect ourselves from malicious input. (On the other hand, we already blindly source /etc/sysconfig/libvirt-guests without validating that it contains _only_ shell assignments, so a malicious user could already put arbitrary shell code in there. The real way to be safe would be to parse out _just_ the assignments we care about, using sed or read, and eval those assignments rather than sourcing the entire file. I guess we have to draw a line somewhere at how much paranoia we want to express. If we can trust that /etc/sysconfig/libvirt-guests can't be subverted by a malicious user, then we can trust that it won't contain a malicious $URIS and instead chalk up bugs in that file as system administration shortcomings.) However, that points out a shortcoming in your patch - we already do constructs like for uri in $URIS; do which _already_ corrupted any globbing and performed IFS splitting, at which point $uri would not contain any further globbing (except in the weird case of a glob that expanded to an existing file name which happens to also be a valid glob), so quoting just "$uri" as in your patch is a lost cause unless we address the problem from the point of the user input to begin with. Meanwhile, we already know that a valid URI does not contain whitespace (a valid URI can represent whitespace via encoding, where needed), so it's already acceptable to use IFS splitting on $URIS to break it into individual URIs, it's just that we need to suppress globbing in the process. 'set -f' can be used for this, if needed. If we could use bash shell arrays, it would be a matter of creating an array variable ${URI[]} that contains the individual URIs, doing the work of safely splitting things only once. But libvirt-guests has to run under /bin/sh == dash for Debian, which lacks shell arrays. I'm still thinking about how to handle this one... Meanwhile, I'm not a fan of blindly quoting everything; there are documented cases where you can trigger shell bugs by doing too much quoting. For example: foo=`some_command "with quotes"` is portable, but foo="`some_command "with quotes"`" is not. So I prefer to quote variables that come from external source, but not internal variables that are obviously safe (that said, quoting generally doesn't hurt, so even if something is overkill does not meant that I am rejecting the patch). This part's good. Quoting "$id" is overkill; we know it's numeric. But it probably doesn't hurt. Likewise, if we know $uuid is valid, then it contains neither $IFS nor globbing characters. Should we instead be tightening up the list_guests function to guarantee that we never encounter garbage data? To be really robust (for example, $confuri could be '(', which is known to trip up some buggy test(1) implementations), this should be: [ "x$confuri" = "x$uri" ] This one's overkill - we explicitly set $configured to either 'true' or 'false'; no external data possible. Overkill - we know $virsh_pid is numeric. Reasonable, since SHUTDOWN_TIMEOUT might come from external sources. Overkill, since once you are inside the while loop, you are guaranteed that $timeout is now numeric. Overkill, since $guest_running is under our control. Overkill, since $empty is under our control. Likewise. Overkill, since we know that $1 is one of three safe values. I think I'll apply your patch as-is, in spite of the overkill, then worry about how to safely split $URIS as a separate patch. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Hello Eric, On Friday Mar 11th 2011 21:48:03 Eric Blake wrote: I would call it root-configurable instead of user-settable, because that file is normally owned by root and has mod 0644. A sane root hopefully does not put a 'rm -rf /' in there ;-) It would be something else if that file was sourced by a program running with more privileges than the user, who is allowed to edit that file. But yes, even root should expect decent bahavior and should not be surprised by the shell expanding string containing wildcards. I've seen to many scripts not doing any quoting at all or all wrong, which fail in interesting to horrible ways when given arguments containing shell meta characters. I personally consider them a much greater risk than some ancient shell not being able to parse properly quoted strings. My rule of thumb is this: if the command dominfo is expecting one argument, I make sure it gets exactly on argument by properly quoting it. I don't care that I know that it will normally not contain blanks or wildcard, so even when someone other later changes the variable in some stupid way, it will still pass one required argument to the function. I prefer virsh to report an error like "'foo bar' is invalid" instead of "'foo' not found" or 'unexpected additional parameter "bar"'. I'm waiting for the day when someone dictates, that the UNIXroot-directory is no longer called '/' but '/ /'. That will give some lovely firefork ;-) Only #1 was the really important fix, the others were done because I was already looking for whitespace problems. To thanks for applying this one two. Sincerely Philipp -- Philipp Hahn Open Source Software Engineer hahn(a)univention.de Univention GmbH Linux for Your Business fon: +49 421 22 232- 0 Mary-Somerville-Str.1 D-28359 Bremen fax: +49 421 22 232-99 http://www.univention.de/

Declare variables used in function only as local. Signed-off-by: Philipp Hahn <hahn(a)univention.de&gt; --- tools/libvirt-guests.init.sh | 10 ++++++++++ 1 files changed, 10 insertions(+), 0 deletions(-) diff --git a/tools/libvirt-guests.init.sh b/tools/libvirt-guests.init.sh index ab2b907..8da7576 100644 --- a/tools/libvirt-guests.init.sh +++ b/tools/libvirt-guests.init.sh @@ -62,6 +62,7 @@ retval() { } run_virsh() { + local uri uri=$1 shift @@ -77,6 +78,7 @@ run_virsh_c() { } list_guests() { + local uri list uuids id uuid uri=$1 list=$(run_virsh_c "$uri" list) @@ -99,6 +101,7 @@ list_guests() { } guest_name() { + local uri uuid name uri=$1 uuid=$2 @@ -110,6 +113,7 @@ guest_name() { } guest_is_on() { + local uri uuid info id uri=$1 uuid=$2 @@ -131,6 +135,7 @@ started() { } start() { + local configured confuri guest name [ -f "$LISTFILE" ] || { started; return 0; } if [ "x$ON_BOOT" != xstart ]; then @@ -175,6 +180,7 @@ start() { suspend_guest() { + local uri guest name label virsh_pid uri=$1 guest=$2 @@ -199,6 +205,7 @@ suspend_guest() shutdown_guest() { + local uri guest name label timeout uri=$1 guest=$2 @@ -226,6 +233,7 @@ shutdown_guest() } stop() { + local suspending uri list guest # last stop was not followed by start [ -f "$LISTFILE" ] && return 0 @@ -286,6 +294,7 @@ stop() { } gueststatus() { + local uri for uri in $URIS; do echo "* $uri URI:" retval run_virsh "$uri" list || echo @@ -313,6 +322,7 @@ rh_status() { # usage [val] # Display usage string, then exit with VAL (defaults to 2). usage() { + local program_name program_name=$0 eval_gettext "Usage: \$program_name {start|stop|status|restart|"\ "condrestart|try-restart|reload|force-reload|gueststatus|shutdown}"; echo -- 1.7.1

The current libvirt-guest init script only prints to stdout, which isn't shown when using a graphical splash screen: During a reboot the shutdown screen might not show any progress while the domains are suspended. This patch mostly drops the use of gettext and gettext_eval and used the appropriate lsb_*-functions. Splash screens integrate with LSB compatibe init scripts by overwriting the lsb_*-functions defined in /lib/lsb/init-functions. Signed-off-by: Philipp Hahn <hahn(a)univention.de&gt; --- tools/libvirt-guests.init.sh | 67 +++++++++++++++++++++-------------------- 1 files changed, 34 insertions(+), 33 deletions(-) diff --git a/tools/libvirt-guests.init.sh b/tools/libvirt-guests.init.sh index 8da7576..00030b8 100644 --- a/tools/libvirt-guests.init.sh +++ b/tools/libvirt-guests.init.sh @@ -36,6 +36,8 @@ test ! -r "$sysconfdir"/rc.d/init.d/functions || # Make sure this file is recognized as having translations: _("dummy") . &quot;@bindir(a)&quot;/gettext.sh +. /lib/lsb/init-functions + export TEXTDOMAIN="@PACKAGE@" TEXTDOMAINDIR="@localedir@" URIS=default @@ -139,8 +141,7 @@ start() { [ -f "$LISTFILE" ] || { started; return 0; } if [ "x$ON_BOOT" != xstart ]; then - gettext "libvirt-guests is configured not to start any guests on boot" - echo + log_action_msg "libvirt-guests is configured not to start any guests on boot" rm -f "$LISTFILE" started return 0 @@ -155,20 +156,20 @@ start() { fi done if ! "$configured"; then - eval_gettext "Ignoring guests on \$uri URI"; echo + log_action_msg "Ignoring guests on $uri URI" continue fi - eval_gettext "Resuming guests on \$uri URI..."; echo + log_action_msg "Resuming guests on $uri URI..." for guest in $list; do name=$(guest_name "$uri" "$guest") - eval_gettext "Resuming guest \$name: " + log_action_begin_msg "Resuming guest $name: " if guest_is_on "$uri" "$guest"; then if "$guest_running"; then - gettext "already active"; echo + log_action_end_msg 0 "already active" else retval run_virsh "$uri" start "$name" >/dev/null && \ - gettext "done"; echo + log_action_end_msg 0 "done" fi fi done @@ -185,22 +186,26 @@ suspend_guest() guest=$2 name=$(guest_name "$uri" "$guest") - label=$(eval_gettext "Suspending \$name: ") - printf %s "$label" + log_action_begin_msg "Suspending $name: " run_virsh "$uri" managedsave "$guest" >/dev/null & virsh_pid=$! while true; do sleep 1 kill -0 "$virsh_pid" >/dev/null 2>&1 || break progress=$(run_virsh_c "$uri" domjobinfo "$guest" 2>/dev/null | \ - awk '/^Data processed:/{print $3, $4}') + awk ' + BEGIN {processed=0;total=0} + /^Data processed:/ {processed=$3} + /^Data total:/ {total=$3} + END {if(processed>=0 && total>0) printf "%d\n", processed/total * 100} + ') if [ -n "$progress" ]; then - printf '\r%s%12s ' "$label" "$progress" + log_action_cont_msg "${progress}%" else - printf '\r%s%-12s ' "$label" "..." + log_action_cont_msg "..." fi done - retval wait "$virsh_pid" && printf '\r%s%-12s\n' "$label" "$(gettext "done")" + retval wait "$virsh_pid" && log_action_end_msg 0 "done" } shutdown_guest() @@ -210,8 +215,7 @@ shutdown_guest() guest=$2 name=$(guest_name "$uri" "$guest") - label=$(eval_gettext "Shutting down \$name: ") - printf %s "$label" + log_action_begin_msg "Shutting down $name: " retval run_virsh "$uri" shutdown "$guest" >/dev/null || return timeout=$SHUTDOWN_TIMEOUT while [ "$timeout" -gt 0 ]; do @@ -219,15 +223,14 @@ shutdown_guest() timeout=$((timeout - 1)) guest_is_on "$uri" "$guest" || return "$guest_running" || break - printf '\r%s%-12d ' "$label" "$timeout" + log_action_cont_msg "$timeout" done if guest_is_on "$uri" "$guest"; then if "$guest_running"; then - printf '\r%s%-12s\n' "$label" \ - "$(gettext "failed to shutdown in time")" + log_action_end_msg 1 "failed to shutdown in time" else - printf '\r%s%-12s\n' "$label" "$(gettext "done")" + log_action_end_msg 0 "done" fi fi } @@ -241,8 +244,7 @@ stop() { if [ "x$ON_SHUTDOWN" = xshutdown ]; then suspending=false if [ $SHUTDOWN_TIMEOUT -le 0 ]; then - gettext "Shutdown action requested but SHUTDOWN_TIMEOUT was not set" - echo + log_failure_msg "Shutdown action requested but SHUTDOWN_TIMEOUT was not set" RETVAL=6 return fi @@ -250,25 +252,24 @@ stop() { : >"$LISTFILE" for uri in $URIS; do - eval_gettext "Running guests on \$uri URI: " - if [ "x$uri" = xdefault ] && [ ! -x "$libvirtd" ]; then - gettext "libvirtd not installed; skipping this URI."; echo + log_failure_msg "libvirtd not installed; skipping URI $uri." continue fi + log_action_begin_msg "Running guests on $uri URI: " + list=$(list_guests "$uri") if [ $? -eq 0 ]; then empty=true for uuid in $list; do - "$empty" || printf ", " - printf %s "$(guest_name "$uri" "$uuid")" + log_action_cont_msg "$(guest_name "$uri" "$uuid")" empty=false done if "$empty"; then - gettext "no running guests."; echo + log_action_end_msg 0 "no running guests." else - echo + log_action_end_msg 0 echo "$uri" "$list" >>"$LISTFILE" fi fi @@ -276,9 +277,9 @@ stop() { while read uri list; do if "$suspending"; then - eval_gettext "Suspending guests on \$uri URI..."; echo + log_action_msg "Suspending guests on $uri URI..." else - eval_gettext "Shutting down guests on \$uri URI..."; echo + log_action_msg "Shutting down guests on $uri URI..." fi for guest in $list; do @@ -307,13 +308,13 @@ gueststatus() { # since there is no external daemon process matching this init script. rh_status() { if [ -f "$LISTFILE" ]; then - gettext "stopped, with saved guests"; echo + log_action_msg "stopped, with saved guests" RETVAL=3 else if [ -f "$VAR_SUBSYS_LIBVIRT_GUESTS" ]; then - gettext "started"; echo + log_action_msg "started" else - gettext "stopped, with no saved guests"; echo + log_action_msg "stopped, with no saved guests" fi RETVAL=0 fi -- 1.7.1