On 08/03/2012 08:52 AM, Gerd v. Egidy wrote:

With SysV-init libvirt-guests wrote status information to the console during shutdown of the guests.

Since the move to systemd libvirt-guests doesn't output this progress information anymore. This patch brings back this feature.

It is helpful to show the admin what the system is waiting for and what is left of the timeout (e.g. for calibrating the shutdown timing of a ups).

Rewriting the current line with \r doesn't work anymore in the context of systemd. So always write new lines, but move to 5 second intervals to avoid flooding the console. --- tools/libvirt-guests.init.sh | 65 +++++++++++++++++++++++++++++++---------- tools/libvirt-guests.service.in | 1 + 2 files changed, 51 insertions(+), 15 deletions(-) mode change 100644 => 100755 tools/libvirt-guests.init.sh

The mode change is spurious, and should not be part of this patch.

+++ b/tools/libvirt-guests.init.sh @@ -225,22 +225,27 @@ suspend_guest() name=$(guest_name "$uri" "$guest") label=$(eval_gettext "Suspending \$name: ") bypass= + slept=0 test "x$BYPASS_CACHE" = x0 || bypass=--bypass-cache - printf %s "$label" + printf '%s%-12s\n' "$label" "..."

Why are you printing trailing whitespace? You are left-justifying the 3-byte ..., which means you now always have 9 bytes of trailing space Also, the "..." doesn't technically need quoting, although I guess it doesn't hurt. Would it be worth folding the ... into the format string itself, as in: printf '%s...\n' "$label" If we were using \r, then I would understand the trailing space as a means of blanking out longer text printed on a previous loop, but you are abandoning \r.

run_virsh "$uri" managedsave $bypass "$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}') - if [ -n "$progress" ]; then - printf '\r%s%12s ' "$label" "$progress" - else - printf '\r%s%-12s ' "$label" "..." + + slept=$(($slept + 1)) + if [ $(($slept%5)) -eq 0 ]; then

Consistency argues that we should have spaces on both sides of '%'

+ progress=$(run_virsh_c "$uri" domjobinfo "$guest" 2>/dev/null | \ + awk '/^Data processed:/{print $3, $4}') + if [ -n "$progress" ]; then + printf '%s%12s\n' "$label" "$progress" + else + printf '%s%-12s\n' "$label" "..." + fi fi

Because you now print only every five seconds, you have lost out on the final printing when progress is non-empty if that happens on 4 of the 5 iterations. I think you want the logic to look more like: slept=$(($slept + 1)) progress=$(...) if [ -n "$progress" ]; then printf ... label progress elif [ $(($slept % 5)) -eq 0 ]; then printf ... label... fi

timeout=$SHUTDOWN_TIMEOUT check_timeout=false if [ $timeout -gt 0 ]; then check_timeout=true + format=$(eval_gettext "Waiting for guest %s to shut down, %4d seconds left\n")

Why the padding for the seconds left? If timeout is 60, I think this looks awkward: Waiting for guest foo to shut down, 60 seconds left

+ else + slept=0 + format=$(eval_gettext "Waiting for guest %s to shut down\n") fi while ! $check_timeout || [ "$timeout" -gt 0 ]; do sleep 1 guest_is_on "$uri" "$guest" || return "$guest_running" || break + if $check_timeout; then - timeout=$((timeout - 1)) - printf '\r%s%-12d ' "$label" "$timeout" + if [ $(($timeout%5)) -eq 0 ]; then

Spaces around %

+ printf "$format" "$name" "$timeout" + fi + timeout=$(($timeout - 1)) + else + slept=$(($slept + 1)) + if [ $(($slept%5)) -eq 0 ]; then

Again.

+ printf "$format" "$name"

This is new output that was not present beforehand. Is the intent that when there is no timeout, you want to show that the process is still alive waiting for the guest?

+ fi fi done

if guest_is_on "$uri" "$guest"; then if "$guest_running"; then - printf '\r%s%-12s\n' "$label" \ - "$(gettext "failed to shutdown in time")" + eval_gettext "Shutdown of guest \$name failed to complete in time." else - printf '\r%s%-12s\n' "$label" "$(gettext "done")" + eval_gettext "Shutdown of guest \$name complete." fi fi } @@ -356,6 +372,10 @@ shutdown_guests_parallel() timeout=$SHUTDOWN_TIMEOUT if [ $timeout -gt 0 ]; then check_timeout=true + format=$(eval_gettext "Waiting for %d guests to shut down, %4d seconds left\n")

Another awkward %4d.

+ else + slept=0 + format=$(eval_gettext "Waiting for %d guests to shut down\n") fi while [ -n "$on_shutdown" ] || [ -n "$guests" ]; do while [ -n "$guests" ] && @@ -368,14 +388,29 @@ shutdown_guests_parallel() on_shutdown="$on_shutdown $guest" done sleep 1 + + set -- $guests + guestcount=$# + set -- $on_shutdown + shutdowncount=$# + if $check_timeout; then + if [ $(($timeout%5)) -eq 0 ]; then + printf "$format" $(($guestcount+$shutdowncount)) "$timeout"

Spaces around % and +

+ fi timeout=$(($timeout - 1)) if [ $timeout -le 0 ]; then eval_gettext "Timeout expired while shutting down domains"; echo RETVAL=1 return fi + else + slept=$(($slept + 1)) + if [ $(($slept%5)) -eq 0 ]; then + printf "$format" $(($guestcount+$shutdowncount))

And again.

+ fi fi + on_shutdown_prev=$on_shutdown on_shutdown=$(check_guests_shutdown "$uri" "$on_shutdown") print_guests_shutdown "$uri" "$on_shutdown_prev" "$on_shutdown" diff --git a/tools/libvirt-guests.service.in b/tools/libvirt-guests.service.in index db28f3f..0f0c41c 100644 --- a/tools/libvirt-guests.service.in +++ b/tools/libvirt-guests.service.in @@ -10,6 +10,7 @@ ExecStart=/etc/init.d/libvirt-guests start ExecStop=/etc/init.d/libvirt-guests stop Type=oneshot RemainAfterExit=yes +StandardOutput=journal+console

Overall, it looks like this patch is headed in the right direction. Did you also check that on F16, where we still used sysvinit, that the output there is still reasonable? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org