[libvirt] [PATCH] libvirt-guests: emit most messages to stderr
Some of the shell functions in this script (e.g. check_guests_shutdown) produce output that is captured and processed by other parts of the script. Any error messages from these shell functions must go to stderr, not stdout. Rather than trying to determine which messages are safe for stdout and which are not (and maintaining this safety as the script is refactored), simplify things by emitting all messages to stderr, except for the output from gueststatus(), rh_status() and usage(). Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/libvirt-guests.sh.in | 94 +++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in index 7f74b85..80b1ec7 100644 --- a/tools/libvirt-guests.sh.in +++ b/tools/libvirt-guests.sh.in @@ -92,16 +92,16 @@ test_connect() i=${CONNECT_RETRIES} while [ $i -gt 0 ]; do - run_virsh "$uri" connect 2>/dev/null + run_virsh "$uri" connect >/dev/null 2>/dev/null if [ $? -eq 0 ]; then return 0; fi sleep ${RETRIES_SLEEP} - eval_gettext "Unable to connect to libvirt currently. Retrying .. \$i" + eval_gettext "Unable to connect to libvirt currently. Retrying .. \$i" >&2 i=$(($i-1)) done - eval_gettext "Can't connect to \$uri. Skipping." - echo + eval_gettext "Can't connect to \$uri. Skipping." >&2 + echo >&2 return 1 } @@ -163,8 +163,8 @@ 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 + gettext "libvirt-guests is configured not to start any guests on boot" >&2 + echo >&2 rm -f "$LISTFILE" started return 0 @@ -187,19 +187,19 @@ start() { done set +f if ! "$configured"; then - eval_gettext "Ignoring guests on \$uri URI"; echo + eval_gettext "Ignoring guests on \$uri URI" >&2; echo >&2 continue fi test_connect "$uri" || continue - eval_gettext "Resuming guests on \$uri URI..."; echo + eval_gettext "Resuming guests on \$uri URI..." >&2; echo >&2 for guest in $list; do name=$(guest_name "$uri" "$guest") - eval_gettext "Resuming guest \$name: " + eval_gettext "Resuming guest \$name: " >&2 if guest_is_on "$uri" "$guest"; then if "$guest_running"; then - gettext "already active"; echo + gettext "already active" >&2; echo >&2 else if "$isfirst"; then isfirst=false @@ -208,7 +208,7 @@ start() { fi retval run_virsh "$uri" start $bypass "$name" \ >/dev/null && \ - gettext "done"; echo + gettext "done" >&2; echo >&2 if "$sync_time"; then run_virsh "$uri" domtime --sync "$name" >/dev/null fi @@ -234,7 +234,7 @@ suspend_guest() bypass= slept=0 test "x$BYPASS_CACHE" = x0 || bypass=--bypass-cache - printf '%s...\n' "$label" + printf '%s...\n' "$label" >&2 run_virsh "$uri" managedsave $bypass "$guest" >/dev/null & virsh_pid=$! while true; do @@ -246,9 +246,9 @@ suspend_guest() progress=$(run_virsh_c "$uri" domjobinfo "$guest" 2>/dev/null | \ awk '/^Data processed:/{print $3, $4}') if [ -n "$progress" ]; then - printf '%s%s\n' "$label" "$progress" + printf '%s%s\n' "$label" "$progress" >&2 else - printf '%s%s\n' "$label" "..." + printf '%s%s\n' "$label" "..." >&2 fi fi done @@ -264,8 +264,8 @@ shutdown_guest() guest=$2 name=$(guest_name "$uri" "$guest") - eval_gettext "Starting shutdown on guest: \$name" - echo + eval_gettext "Starting shutdown on guest: \$name" >&2 + echo >&2 retval run_virsh "$uri" shutdown "$guest" >/dev/null || return timeout=$SHUTDOWN_TIMEOUT check_timeout=false @@ -283,24 +283,24 @@ shutdown_guest() if $check_timeout; then if [ $(($timeout % 5)) -eq 0 ]; then - printf "$format" "$name" "$timeout" + printf "$format" "$name" "$timeout" >&2 fi timeout=$(($timeout - 1)) else slept=$(($slept + 1)) if [ $(($slept % 5)) -eq 0 ]; then - printf "$format" "$name" + printf "$format" "$name" >&2 fi fi done if guest_is_on "$uri" "$guest"; then if "$guest_running"; then - eval_gettext "Shutdown of guest \$name failed to complete in time." + eval_gettext "Shutdown of guest \$name failed to complete in time." >&2 else - eval_gettext "Shutdown of guest \$name complete." + eval_gettext "Shutdown of guest \$name complete." >&2 fi - echo + echo >&2 fi } @@ -313,8 +313,8 @@ shutdown_guest_async() guest=$2 name=$(guest_name "$uri" "$guest") - eval_gettext "Starting shutdown on guest: \$name" - echo + eval_gettext "Starting shutdown on guest: \$name" >&2 + echo >&2 retval run_virsh "$uri" shutdown "$guest" > /dev/null } @@ -337,8 +337,8 @@ check_guests_shutdown() guests_up= for guest in $guests; do if ! guest_is_on "$uri" "$guest" >/dev/null 2>&1; then - eval_gettext "Failed to determine state of guest: \$guest. Not tracking it anymore." - echo + eval_gettext "Failed to determine state of guest: \$guest. Not tracking it anymore." >&2 + echo >&2 continue fi if "$guest_running"; then @@ -363,8 +363,8 @@ print_guests_shutdown() esac name=$(guest_name "$uri" "$guest") - eval_gettext "Shutdown of guest \$name complete." - echo + eval_gettext "Shutdown of guest \$name complete." >&2 + echo >&2 done } @@ -404,18 +404,18 @@ shutdown_guests_parallel() if $check_timeout; then if [ $(($timeout % 5)) -eq 0 ]; then - printf "$format" $(($guestcount + $shutdowncount)) "$timeout" + printf "$format" $(($guestcount + $shutdowncount)) "$timeout" >&2 fi timeout=$(($timeout - 1)) if [ $timeout -le 0 ]; then - eval_gettext "Timeout expired while shutting down domains"; echo + eval_gettext "Timeout expired while shutting down domains" >&2; echo >&2 RETVAL=1 return fi else slept=$(($slept + 1)) if [ $(($slept % 5)) -eq 0 ]; then - printf "$format" $(($guestcount + $shutdowncount)) + printf "$format" $(($guestcount + $shutdowncount)) >&2 fi fi @@ -435,8 +435,8 @@ stop() { if [ "x$ON_SHUTDOWN" = xshutdown ]; then suspending=false if [ $SHUTDOWN_TIMEOUT -lt 0 ]; then - gettext "SHUTDOWN_TIMEOUT must be equal or greater than 0" - echo + gettext "SHUTDOWN_TIMEOUT must be equal or greater than 0" >&2 + echo >&2 RETVAL=6 return fi @@ -449,21 +449,21 @@ stop() { test_connect "$uri" || continue - eval_gettext "Running guests on \$uri URI: " + eval_gettext "Running guests on \$uri URI: " >&2 list=$(list_guests "$uri") if [ $? -eq 0 ]; then empty=true for uuid in $list; do - "$empty" || printf ", " - printf %s "$(guest_name "$uri" "$uuid")" + "$empty" || printf ", " >&2 + printf %s "$(guest_name "$uri" "$uuid")" >&2 empty=false done if "$empty"; then - gettext "no running guests." + gettext "no running guests." >&2 fi - echo + echo >&2 fi if "$suspending"; then @@ -472,26 +472,26 @@ stop() { empty=true for uuid in $transient; do if "$empty"; then - eval_gettext "Not suspending transient guests on URI: \$uri: " + eval_gettext "Not suspending transient guests on URI: \$uri: " >&2 empty=false else - printf ", " + printf ", " >&2 fi - printf %s "$(guest_name "$uri" "$uuid")" + printf %s "$(guest_name "$uri" "$uuid")" >&2 done - echo + echo >&2 # reload domain list to contain only persistent guests list=$(list_guests "$uri" "--persistent") if [ $? -ne 0 ]; then - eval_gettext "Failed to list persistent guests on \$uri" - echo + eval_gettext "Failed to list persistent guests on \$uri" >&2 + echo >&2 RETVAL=1 set +f return fi else - gettext "Failed to list transient guests" - echo + gettext "Failed to list transient guests" >&2 + echo >&2 RETVAL=1 set +f return @@ -507,9 +507,9 @@ stop() { if [ -s "$LISTFILE" ]; then while read uri list; do if "$suspending"; then - eval_gettext "Suspending guests on \$uri URI..."; echo + eval_gettext "Suspending guests on \$uri URI..." >&2; echo >&2 else - eval_gettext "Shutting down guests on \$uri URI..."; echo + eval_gettext "Shutting down guests on \$uri URI..." >&2; echo >&2 fi if [ "$PARALLEL_SHUTDOWN" -gt 1 ] && -- 2.4.3
On Thu, Dec 31, 2015 at 04:53:54PM +1100, Michael Chapman wrote:
Some of the shell functions in this script (e.g. check_guests_shutdown) produce output that is captured and processed by other parts of the script. Any error messages from these shell functions must go to stderr, not stdout.
Rather than trying to determine which messages are safe for stdout and which are not (and maintaining this safety as the script is refactored), simplify things by emitting all messages to stderr, except for the output from gueststatus(), rh_status() and usage().
This is a good idea, but I just simply changing all outputs to stderr seems wrong. It doesn't matter for systemd systems, I guess, we output everything to stdout currently and it goes to the logs anyway. But for other systems it might pollute the output with just informative messages. For example on my system I would see stderr but not stdout (that goes to its logfile). What is the reasoning behind this change? Why can't we keep everything on stdout?
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/libvirt-guests.sh.in | 94 +++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 47 deletions(-)
diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in index 7f74b85..80b1ec7 100644 --- a/tools/libvirt-guests.sh.in +++ b/tools/libvirt-guests.sh.in @@ -92,16 +92,16 @@ test_connect()
i=${CONNECT_RETRIES} while [ $i -gt 0 ]; do - run_virsh "$uri" connect 2>/dev/null + run_virsh "$uri" connect >/dev/null 2>/dev/null
This could be fixed no matter how we decide to proceed with the rest. Martin
On Wed, 6 Jan 2016, Martin Kletzander wrote:
On Thu, Dec 31, 2015 at 04:53:54PM +1100, Michael Chapman wrote:
Some of the shell functions in this script (e.g. check_guests_shutdown) produce output that is captured and processed by other parts of the script. Any error messages from these shell functions must go to stderr, not stdout.
Rather than trying to determine which messages are safe for stdout and which are not (and maintaining this safety as the script is refactored), simplify things by emitting all messages to stderr, except for the output from gueststatus(), rh_status() and usage().
This is a good idea, but I just simply changing all outputs to stderr seems wrong. It doesn't matter for systemd systems, I guess, we output everything to stdout currently and it goes to the logs anyway. But for other systems it might pollute the output with just informative messages. For example on my system I would see stderr but not stdout (that goes to its logfile). What is the reasoning behind this change? Why can't we keep everything on stdout?
I was rebooting a hypervisor with running guests. My libvirt-guests config has: ON_SHUTDOWN=shutdown PARALLEL_SHUTDOWN=8 I noticed it was taking an awful long time to reboot. What had happened was that it had hit the error path in check_guests_shutdown, so shutdown_guests_parallel was stuck looping over: virsh domid Failed virsh domid to virsh domid determine virsh domid state virsh domid of virsh domid guest ... The error message had been printed to stdout, so it got captured as the list of guests still needing to be shutdown. I could have just fixed that one error case (but how? Not use $(...) captures?), but I decided to change them all since it seemed likely that this problem might crop up again in the future. Can you think of an alternative approach? I hadn't considered the case of systems redirecting only stdout from initscripts.
Signed-off-by: Michael Chapman <mike@very.puzzling.org> --- tools/libvirt-guests.sh.in | 94 +++++++++++++++++++++++----------------------- 1 file changed, 47 insertions(+), 47 deletions(-)
diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in index 7f74b85..80b1ec7 100644 --- a/tools/libvirt-guests.sh.in +++ b/tools/libvirt-guests.sh.in @@ -92,16 +92,16 @@ test_connect()
i=${CONNECT_RETRIES} while [ $i -gt 0 ]; do - run_virsh "$uri" connect 2>/dev/null + run_virsh "$uri" connect >/dev/null 2>/dev/null
This could be fixed no matter how we decide to proceed with the rest.
Martin
- Michael
On Thu, Jan 07, 2016 at 10:30:49AM +1100, Michael Chapman wrote:
On Wed, 6 Jan 2016, Martin Kletzander wrote:
On Thu, Dec 31, 2015 at 04:53:54PM +1100, Michael Chapman wrote:
Some of the shell functions in this script (e.g. check_guests_shutdown) produce output that is captured and processed by other parts of the script. Any error messages from these shell functions must go to stderr, not stdout.
Rather than trying to determine which messages are safe for stdout and which are not (and maintaining this safety as the script is refactored), simplify things by emitting all messages to stderr, except for the output from gueststatus(), rh_status() and usage().
This is a good idea, but I just simply changing all outputs to stderr seems wrong. It doesn't matter for systemd systems, I guess, we output everything to stdout currently and it goes to the logs anyway. But for other systems it might pollute the output with just informative messages. For example on my system I would see stderr but not stdout (that goes to its logfile). What is the reasoning behind this change? Why can't we keep everything on stdout?
I was rebooting a hypervisor with running guests. My libvirt-guests config has:
ON_SHUTDOWN=shutdown PARALLEL_SHUTDOWN=8
I noticed it was taking an awful long time to reboot. What had happened was that it had hit the error path in check_guests_shutdown, so shutdown_guests_parallel was stuck looping over:
virsh domid Failed virsh domid to virsh domid determine virsh domid state virsh domid of virsh domid guest ...
The error message had been printed to stdout, so it got captured as the list of guests still needing to be shutdown.
I could have just fixed that one error case (but how? Not use $(...) captures?), but I decided to change them all since it seemed likely that this problem might crop up again in the future.
Can you think of an alternative approach? I hadn't considered the case of systems redirecting only stdout from initscripts.
So sorry for not responding to this for so long. I would just say keep all the messages to stdout and output one error message (doesn't have to be from virsh, just 'echo $msg 2>&1' will suffice) to stderr for each domain that we failed to stop/start. I did not mean to hinder the fix by any means, sorry for that once again. I'd be more than happy to merge fixes for the script upstream. Thanks, Martin
participants (2)
-
Martin Kletzander -
Michael Chapman