On Wed, Aug 19, 2020 at 12:15 PM Christian Ehrhardt
<christian.ehrhardt@xxxxxxxxxxxxx> wrote:
>
> In libvirt 6.6 stopping guests with libvirt-guests.sh is broken.
> As soon as there is more than one guest one can see
> `systemctl stop libvirt-guests` faiing and in the log we see:
> libvirt-guests.sh[2455]: Running guests on default URI:
> libvirt-guests.sh[2457]: /usr/lib/libvirt/libvirt-guests.sh: 120:
> local: 2a49cb0f-1ff8-44b5-a61d-806b9e52dae2: bad variable name
> libvirt-guests.sh[2462]: no running guests.
>
> That is due do mutliple guests becoming a list of UUIDs. Without
> recognizing this as one single string the assignment breaks when using 'local'
> (which was recently added in 6.3.0). This is because local is defined as
> local [option] [name[=value] ... | - ]
> which makes the shell trying handle the further part of the string as
> variable names. In the error above that string isn't a valid variable
> name triggering the issue that is seen.
>
> To resolve that 'textify' all assignments that are strings or potentially
> can become such lists (even if they are not using the local qualifier).
Just to illustrate the problem, this never was great and could cause
warnings/errors,
but amplified due to the 'local' it makes the script break now.
$ cat test.sh
#!/bin/sh
foo () {
f=2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3
echo f
}
bar () {
local b=2a49cb0f-1ff8-44b5-a61d-806b9e52dae2
2a49cb0f-1ff8-44b5-a61d-806b9e52dae3
echo b
}
foo
bar
echo end of script
$ ./test.sh
./test.sh: 4: 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3: not found
f
./test.sh: 9: local: 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3: bad variable name
> Fixes: 08071ec0 "tools: variables clean-up in libvirt-guests script"
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx>
> ---
> tools/libvirt-guests.sh.in | 136 ++++++++++++++++++-------------------
> 1 file changed, 68 insertions(+), 68 deletions(-)
>
> diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
> index 534c4d5e0f..d69df908d2 100644
> --- a/tools/libvirt-guests.sh.in
> +++ b/tools/libvirt-guests.sh.in
> @@ -30,9 +30,9 @@ test ! -r "$sysconfdir"/rc.d/init.d/functions ||
>
> export TEXTDOMAIN="@PACKAGE@" TEXTDOMAINDIR="@localedir@"
>
> -URIS=default
> -ON_BOOT=start
> -ON_SHUTDOWN=suspend
> +URIS="default"
> +ON_BOOT="start"
> +ON_SHUTDOWN="suspend"
> SHUTDOWN_TIMEOUT=300
> PARALLEL_SHUTDOWN=0
> START_DELAY=0
> @@ -65,7 +65,7 @@ retval() {
> # If URI is "default" virsh is called without the "-c" argument
> # (using libvirt's default connection)
> run_virsh() {
> - local uri=$1
> + local uri="$1"
> shift
>
> if [ "x$uri" = xdefault ]; then
> @@ -86,7 +86,7 @@ run_virsh_c() {
> # check if URI is reachable
> test_connect()
> {
> - local uri=$1
> + local uri="$1"
>
> if run_virsh "$uri" connect 2>/dev/null; then
> return 0;
> @@ -103,9 +103,9 @@ test_connect()
> # --transient: list only transient guests
> # [none]: list both persistent and transient guests
> list_guests() {
> - local uri=$1
> - local persistent=$2
> - local list=$(run_virsh_c "$uri" list --uuid $persistent)
> + local uri="$1"
> + local persistent="$2"
> + local list="$(run_virsh_c "$uri" list --uuid $persistent)"
>
> if [ $? -ne 0 ]; then
> RETVAL=1
> @@ -118,8 +118,8 @@ list_guests() {
> # guest_name URI UUID
> # return name of guest UUID on URI
> guest_name() {
> - local uri=$1
> - local uuid=$2
> + local uri="$1"
> + local uuid="$2"
>
> run_virsh "$uri" domname "$uuid" 2>/dev/null
> }
> @@ -128,17 +128,17 @@ guest_name() {
> # check if guest UUID on URI is running
> # Result is returned by variable "guest_running"
> guest_is_on() {
> - local uri=$1
> - local uuid=$2
> - local id=$(run_virsh "$uri" domid "$uuid")
> + local uri="$1"
> + local uuid="$2"
> + local id="$(run_virsh "$uri" domid "$uuid")"
>
> - guest_running=false
> + guest_running="false"
> if [ $? -ne 0 ]; then
> RETVAL=1
> return 1
> fi
>
> - [ -n "$id" ] && [ "x$id" != x- ] && guest_running=true
> + [ -n "$id" ] && [ "x$id" != x- ] && guest_running="true"
> return 0
> }
>
> @@ -151,9 +151,9 @@ started() {
> # start
> # Start or resume the guests
> start() {
> - local isfirst=true
> + local isfirst="true"
> local bypass=
> - local sync_time=false
> + local sync_time="false"
> local uri=
> local list=
>
> @@ -167,10 +167,10 @@ start() {
> return 0
> fi
>
> - test "x$BYPASS_CACHE" = x0 || bypass=--bypass-cache
> - test "x$SYNC_TIME" = x0 || sync_time=true
> + test "x$BYPASS_CACHE" = x0 || bypass="--bypass-cache"
> + test "x$SYNC_TIME" = x0 || sync_time="true"
> while read uri list; do
> - local configured=false
> + local configured="false"
> local confuri=
> local guest=
>
> @@ -178,7 +178,7 @@ start() {
> for confuri in $URIS; do
> set +f
> if [ "x$confuri" = "x$uri" ]; then
> - configured=true
> + configured="true"
> break
> fi
> done
> @@ -192,14 +192,14 @@ start() {
>
> eval_gettext "Resuming guests on \$uri URI..."; echo
> for guest in $list; do
> - local name=$(guest_name "$uri" "$guest")
> + local name="$(guest_name "$uri" "$guest")"
> eval_gettext "Resuming guest \$name: "
> if guest_is_on "$uri" "$guest"; then
> if "$guest_running"; then
> gettext "already active"; echo
> else
> if "$isfirst"; then
> - isfirst=false
> + isfirst="false"
> else
> sleep $START_DELAY
> fi
> @@ -223,25 +223,25 @@ start() {
> # was saved.
> suspend_guest()
> {
> - local uri=$1
> - local guest=$2
> - local name=$(guest_name "$uri" "$guest")
> - local label=$(eval_gettext "Suspending \$name: ")
> + local uri="$1"
> + local guest="$2"
> + local name="$(guest_name "$uri" "$guest")"
> + local label="$(eval_gettext "Suspending \$name: ")"
> local bypass=
> local slept=0
>
> - test "x$BYPASS_CACHE" = x0 || bypass=--bypass-cache
> + test "x$BYPASS_CACHE" = x0 || bypass="--bypass-cache"
> printf '%s...\n' "$label"
> run_virsh "$uri" managedsave $bypass "$guest" >/dev/null &
> - local virsh_pid=$!
> + local virsh_pid="$!"
> while true; do
> sleep 1
> kill -0 "$virsh_pid" >/dev/null 2>&1 || break
>
> slept=$(($slept + 1))
> if [ $(($slept % 5)) -eq 0 ]; then
> - local progress=$(run_virsh_c "$uri" domjobinfo "$guest" 2>/dev/null | \
> - awk '/^Data processed:/{print $3, $4}')
> + local 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"
> else
> @@ -257,11 +257,11 @@ suspend_guest()
> # was successfully shutdown or the timeout defined by $SHUTDOWN_TIMEOUT expired.
> shutdown_guest()
> {
> - local uri=$1
> - local guest=$2
> - local name=$(guest_name "$uri" "$guest")
> - local timeout=$SHUTDOWN_TIMEOUT
> - local check_timeout=false
> + local uri="$1"
> + local guest="$2"
> + local name="$(guest_name "$uri" "$guest")"
> + local timeout="$SHUTDOWN_TIMEOUT"
> + local check_timeout="false"
> local format=
> local slept=
>
> @@ -270,11 +270,11 @@ shutdown_guest()
> retval run_virsh "$uri" shutdown "$guest" >/dev/null || return
>
> if [ $timeout -gt 0 ]; then
> - check_timeout=true
> - format=$(eval_gettext "Waiting for guest %s to shut down, %d seconds left\n")
> + check_timeout="true"
> + format="$(eval_gettext "Waiting for guest %s to shut down, %d seconds left\n")"
> else
> slept=0
> - format=$(eval_gettext "Waiting for guest %s to shut down\n")
> + format="$(eval_gettext "Waiting for guest %s to shut down\n")"
> fi
> while ! $check_timeout || [ "$timeout" -gt 0 ]; do
> sleep 1
> @@ -309,9 +309,9 @@ shutdown_guest()
> # was issued to libvirt to allow parallel shutdown.
> shutdown_guest_async()
> {
> - local uri=$1
> - local guest=$2
> - local name=$(guest_name "$uri" "$guest")
> + local uri="$1"
> + local guest="$2"
> + local name="$(guest_name "$uri" "$guest")"
>
> eval_gettext "Starting shutdown on guest: \$name"
> echo
> @@ -332,8 +332,8 @@ guest_count()
> # Result is returned in "guests_shutting_down"
> check_guests_shutdown()
> {
> - local uri=$1
> - local guests_to_check=$2
> + local uri="$1"
> + local guests_to_check="$2"
> local guest=
>
> guests_shutting_down=
> @@ -354,9 +354,9 @@ check_guests_shutdown()
> # a shutdown complete notice for guests that have finished
> print_guests_shutdown()
> {
> - local uri=$1
> - local before=$2
> - local after=$3
> + local uri="$1"
> + local before="$2"
> + local after="$3"
> local guest=
>
> for guest in $before; do
> @@ -364,7 +364,7 @@ print_guests_shutdown()
> *" $guest "*) continue;;
> esac
>
> - local name=$(guest_name "$uri" "$guest")
> + local name="$(guest_name "$uri" "$guest")"
> if [ -n "$name" ]; then
> eval_gettext "Shutdown of guest \$name complete."
> echo
> @@ -376,28 +376,28 @@ print_guests_shutdown()
> # Shutdown guests GUESTS on machine URI in parallel
> shutdown_guests_parallel()
> {
> - local uri=$1
> - local guests=$2
> + local uri="$1"
> + local guests="$2"
> local on_shutdown=
> - local check_timeout=false
> - local timeout=$SHUTDOWN_TIMEOUT
> + local check_timeout="false"
> + local timeout="$SHUTDOWN_TIMEOUT"
> local slept=
> local format=
>
> if [ $timeout -gt 0 ]; then
> - check_timeout=true
> - format=$(eval_gettext "Waiting for %d guests to shut down, %d seconds left\n")
> + check_timeout="true"
> + format="$(eval_gettext "Waiting for %d guests to shut down, %d seconds left\n")"
> else
> slept=0
> - format=$(eval_gettext "Waiting for %d guests to shut down\n")
> + format="$(eval_gettext "Waiting for %d guests to shut down\n")"
> fi
> while [ -n "$on_shutdown" ] || [ -n "$guests" ]; do
> while [ -n "$guests" ] &&
> [ $(guest_count "$on_shutdown") -lt "$PARALLEL_SHUTDOWN" ]; do
> set -- $guests
> - local guest=$1
> + local guest="$1"
> shift
> - guests=$*
> + guests="$*"
> if [ -z "$(echo $on_shutdown | grep $guest)" ] &&
> [ -n "$(guest_name "$uri" "$guest")" ]; then
> shutdown_guest_async "$uri" "$guest"
> @@ -428,7 +428,7 @@ shutdown_guests_parallel()
> fi
> fi
>
> - local on_shutdown_prev=$on_shutdown
> + local on_shutdown_prev="$on_shutdown"
> check_guests_shutdown "$uri" "$on_shutdown"
> on_shutdown="$guests_shutting_down"
> print_guests_shutdown "$uri" "$on_shutdown_prev" "$on_shutdown"
> @@ -438,14 +438,14 @@ shutdown_guests_parallel()
> # stop
> # Shutdown or save guests on the configured uris
> stop() {
> - local suspending=true
> + local suspending="true"
> local uri=
>
> # last stop was not followed by start
> [ -f "$LISTFILE" ] && return 0
>
> if [ "x$ON_SHUTDOWN" = xshutdown ]; then
> - suspending=false
> + suspending="false"
> if [ $SHUTDOWN_TIMEOUT -lt 0 ]; then
> gettext "SHUTDOWN_TIMEOUT must be equal or greater than 0"
> echo
> @@ -463,13 +463,13 @@ stop() {
>
> eval_gettext "Running guests on \$uri URI: "
>
> - local list=$(list_guests "$uri")
> + local list="$(list_guests "$uri")"
> if [ $? -eq 0 ]; then
> local empty=true
> for uuid in $list; do
> "$empty" || printf ", "
> printf %s "$(guest_name "$uri" "$uuid")"
> - empty=false
> + empty="false"
> done
>
> if "$empty"; then
> @@ -479,15 +479,15 @@ stop() {
> fi
>
> if "$suspending"; then
> - local transient=$(list_guests "$uri" "--transient")
> + local transient="$(list_guests "$uri" "--transient")"
> if [ $? -eq 0 ]; then
> - local empty=true
> + local empty="true"
> local uuid=
>
> for uuid in $transient; do
> if "$empty"; then
> eval_gettext "Not suspending transient guests on URI: \$uri: "
> - empty=false
> + empty="false"
> else
> printf ", "
> fi
> @@ -495,7 +495,7 @@ stop() {
> done
> echo
> # reload domain list to contain only persistent guests
> - list=$(list_guests "$uri" "--persistent")
> + list="$(list_guests "$uri" "--persistent")"
> if [ $? -ne 0 ]; then
> eval_gettext "Failed to list persistent guests on \$uri"
> echo
> @@ -582,7 +582,7 @@ rh_status() {
> # usage [val]
> # Display usage string, then exit with VAL (defaults to 2).
> usage() {
> - local program_name=$0
> + local program_name="$0"
> eval_gettext "Usage: \$program_name {start|stop|status|restart|"\
> "condrestart|try-restart|reload|force-reload|gueststatus|shutdown}"; echo
> exit ${1-2}
> @@ -612,7 +612,7 @@ case "$1" in
> rh_status
> ;;
> shutdown)
> - ON_SHUTDOWN=shutdown
> + ON_SHUTDOWN="shutdown"
> stop
> ;;
> *)
> --
> 2.28.0
>
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
