tools/libvirt-guests.sh.in | 136 ++++++++++++++++++------------------- 1 file changed, 68 insertions(+), 68 deletions(-)
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).
Fixes: 08071ec0 "tools: variables clean-up in libvirt-guests script"
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
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
On Wed, Aug 19, 2020 at 12:15 PM Christian Ehrhardt
<christian.ehrhardt@canonical.com> 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@canonical.com>
> ---
> 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
On Thu, 20 Aug 2020, Christian Ehrhardt wrote:
> On Wed, Aug 19, 2020 at 12:15 PM Christian Ehrhardt
> <christian.ehrhardt@canonical.com> 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.
Arguably the big problem here is that 'local' isn't actually specified by
POSIX, so can not be used in a portable /bin/sh script. (It might end up
in POSIX eventually, see [1].)
If this were a Bash script, then all of those variable assignments
(whether they're local or not) would work as expected:
$ echo $BASH_VERSION
5.0.17(1)-release
$ uuid='2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3'
$ foo() { x=$uuid; echo "<$x>"; }
$ bar() { local x=$uuid; echo "<$x>"; }
$ foo
<2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3>
$ bar
<2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3>
This works even when Bash is running in POSIX mode.
But POSIX shells that do not support 'local' seem to be rare, and your
suggested change would make these assignments work even on shells that do
not apply special parsing rules for it.
[1] https://www.austingroupbugs.net/bug_view_page.php?bug_id=767
On Thu, Aug 20, 2020 at 10:50 AM Michael Chapman <mike@very.puzzling.org> wrote:
>
> On Thu, 20 Aug 2020, Christian Ehrhardt wrote:
> > On Wed, Aug 19, 2020 at 12:15 PM Christian Ehrhardt
> > <christian.ehrhardt@canonical.com> 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.
>
> Arguably the big problem here is that 'local' isn't actually specified by
> POSIX, so can not be used in a portable /bin/sh script. (It might end up
> in POSIX eventually, see [1].)
>
> If this were a Bash script, then all of those variable assignments
> (whether they're local or not) would work as expected:
>
> $ echo $BASH_VERSION
> 5.0.17(1)-release
> $ uuid='2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3'
> $ foo() { x=$uuid; echo "<$x>"; }
> $ bar() { local x=$uuid; echo "<$x>"; }
> $ foo
> <2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3>
> $ bar
> <2a49cb0f-1ff8-44b5-a61d-806b9e52dae2 2a49cb0f-1ff8-44b5-a61d-806b9e52dae3>
Thanks for these initial tests showing that this is a real issue
(depending on shell&mode being used).
> This works even when Bash is running in POSIX mode.
Dash in POSIX mode (used as /bin/sh interpreter) was what I used to
spot the issue.
Maybe because bash in POSIX mode works is why it wasn't spotted before.
> But POSIX shells that do not support 'local' seem to be rare
Agreed
> , and your
> suggested change would make these assignments work even on shells that do
> not apply special parsing rules for it.
Thanks, I also have pushed this through a bunch of tests that use
libvirt-guests.sh on multiple situations and architectures.
They all worked fine with the fix (and were the ones spotting the
issue in the first place).
So after adding these extra bits of info to the commit message, fixing
a typo in there and running a gitlab CI pipeline on it I'm pushing the
commit.
Thanks Michael for the extra thoughts on this!
>
> [1] https://www.austingroupbugs.net/bug_view_page.php?bug_id=767
--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd
On Friday, August 21, 2020 10:05:21 AM, Christian Ehrhardt wrote: > > , and your > > suggested change would make these assignments work even on shells that do > > not apply special parsing rules for it. > > Thanks, I also have pushed this through a bunch of tests that use > libvirt-guests.sh on multiple situations and architectures. > They all worked fine with the fix (and were the ones spotting the > issue in the first place). > So after adding these extra bits of info to the commit message, fixing > a typo in there and running a gitlab CI pipeline on it I'm pushing the > commit. While the patch itself looks good, and it's great that you managed to track down the issue as well as give the fix some serious testing, it looks like you ultimately pushed this change without getting a proper review. There are situations where pushing changes without review is warranted, but this doesn't seem to fall into one of those fairly narrow guidelines. In the future, please refrain from pushing changes that haven't gone through the usual review process. -- Andrea Bolognani / Red Hat / Virtualization
© 2016 - 2024 Red Hat, Inc.