1. Patchew
  2. Libvirt
  3. [PATCH 0/2] libvirt-guests.sh: Declare and assign separately to avoid masking return values
  4. View patch

[PATCH 1/2] libvirt-guests.sh: Declare and assign separately to avoid masking return values

Michal Privoznik via Devel posted 2 patches 2 weeks, 2 days ago
There is a newer version of this series
[PATCH 1/2] libvirt-guests.sh: Declare and assign separately to avoid masking return values
Posted by Michal Privoznik via Devel 2 weeks, 2 days ago 
From: Michal Privoznik <mprivozn@redhat.com>

In Bash, the following code does not do what you think it does:

  func() {
      local var=$(false)
      echo $?
  }

Here, '0' is echoed even though false is designed to exit with a
non-zero code. This is because in fact the last executed command
is 'local' not 'false' and thus '$?' contains zero as in "yeah,
the variable is successfully declared" [1]. In our libvirt-guest
shell script, there are a few places like this. Now, in next
commit a syntax-check rule is introduced and to keep it simple
(by avoiding multi line grep) it'll deny declaring local
variables and assigning their value via a subshell regardless of
subsequent '$?' check. Thus, this commit may change more than
necessary, strictly speaking. But in the long run, we're on the
safe side.

1: https://www.shellcheck.net/wiki/SC2155
Fixes: 08071ec0f113bb1fe8dcc263cb6bf87529e8b76b
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/839
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/libvirt-guests.sh.in | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
index f2db1282ad..f57eda66fe 100644
--- a/tools/libvirt-guests.sh.in
+++ b/tools/libvirt-guests.sh.in
@@ -106,7 +106,9 @@ test_connect()
 list_guests() {
     local uri="$1"
     local persistent="$2"
-    local list="$(run_virsh_c "$uri" list --uuid $persistent)"
+    local list=
+
+    list="$(run_virsh_c "$uri" list --uuid $persistent)"
 
     if [ $? -ne 0 ]; then
         RETVAL=1
@@ -131,9 +133,10 @@ guest_name() {
 guest_is_on() {
     local uri="$1"
     local uuid="$2"
-    local id="$(run_virsh "$uri" domid "$uuid")"
+    local id=
 
     guest_running="false"
+    id="$(run_virsh "$uri" domid "$uuid")"
     if [ $? -ne 0 ]; then
         RETVAL=1
         return 1
@@ -193,7 +196,8 @@ start() {
 
         eval_gettext "Resuming guests on \$uri URI..."; echo
         for guest in $list; do
-            local name="$(guest_name "$uri" "$guest")"
+            local name=
+            name="$(guest_name "$uri" "$guest")"
             eval_gettext "Resuming guest \$name: "
             if guest_is_on "$uri" "$guest"; then
                 if "$guest_running"; then
@@ -226,11 +230,13 @@ suspend_guest()
 {
     local uri="$1"
     local guest="$2"
-    local name="$(guest_name "$uri" "$guest")"
-    local label="$(eval_gettext "Suspending \$name: ")"
+    local name=
+    local label=
     local bypass=
     local slept=0
 
+    name="$(guest_name "$uri" "$guest")"
+    label="$(eval_gettext "Suspending \$name: ")"
     test "x$BYPASS_CACHE" = x0 || bypass="--bypass-cache"
     printf '%s...\n' "$label"
     run_virsh "$uri" managedsave $bypass "$guest" >/dev/null &
@@ -241,7 +247,8 @@ suspend_guest()
 
         slept=$(($slept + 1))
         if [ $(($slept % 5)) -eq 0 ]; then
-            local progress="$(run_virsh_c "$uri" domjobinfo "$guest" 2>/dev/null | \
+            local progress=
+            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"
@@ -260,12 +267,13 @@ shutdown_guest()
 {
     local uri="$1"
     local guest="$2"
-    local name="$(guest_name "$uri" "$guest")"
+    local name=
     local timeout="$SHUTDOWN_TIMEOUT"
     local check_timeout="false"
     local format=
     local slept=
 
+    name="$(guest_name "$uri" "$guest")"
     eval_gettext "Starting shutdown on guest: \$name"
     echo
     retval run_virsh "$uri" shutdown "$guest" >/dev/null || return
@@ -312,8 +320,9 @@ shutdown_guest_async()
 {
     local uri="$1"
     local guest="$2"
-    local name="$(guest_name "$uri" "$guest")"
+    local name=
 
+    name="$(guest_name "$uri" "$guest")"
     eval_gettext "Starting shutdown on guest: \$name"
     echo
     retval run_virsh "$uri" shutdown "$guest" > /dev/null
@@ -365,7 +374,8 @@ print_guests_shutdown()
             *" $guest "*) continue;;
         esac
 
-        local name="$(guest_name "$uri" "$guest")"
+        local name=
+        name="$(guest_name "$uri" "$guest")"
         if [ -n "$name" ]; then
             eval_gettext "Shutdown of guest \$name complete."
             echo
@@ -482,7 +492,8 @@ stop() {
 
         eval_gettext "Running guests on \$uri URI: "
 
-        local list="$(list_guests "$uri")"
+        local list=
+        list="$(list_guests "$uri")"
         if [ $? -eq 0 ]; then
             local empty=true
             for uuid in $list; do
@@ -498,7 +509,8 @@ stop() {
         fi
 
         if "$persistent_only"; then
-            local transient="$(list_guests "$uri" "--transient")"
+            local transient=
+            transient="$(list_guests "$uri" "--transient")"
             if [ $? -eq 0 ]; then
                 local empty="true"
                 local uuid=
-- 
2.52.0
Re: [PATCH 1/2] libvirt-guests.sh: Declare and assign separately to avoid masking return values
Posted by Martin Kletzander via Devel 2 weeks, 2 days ago 
On Fri, Jan 16, 2026 at 10:22:13AM +0100, Michal Privoznik via Devel wrote:
>From: Michal Privoznik <mprivozn@redhat.com>
>
>In Bash, the following code does not do what you think it does:
>
>  func() {
>      local var=$(false)
>      echo $?
>  }
>
>Here, '0' is echoed even though false is designed to exit with a
>non-zero code. This is because in fact the last executed command
>is 'local' not 'false' and thus '$?' contains zero as in "yeah,
>the variable is successfully declared" [1]. In our libvirt-guest
>shell script, there are a few places like this. Now, in next
>commit a syntax-check rule is introduced and to keep it simple
>(by avoiding multi line grep) it'll deny declaring local
>variables and assigning their value via a subshell regardless of
>subsequent '$?' check. Thus, this commit may change more than
>necessary, strictly speaking. But in the long run, we're on the
>safe side.
>
>1: https://www.shellcheck.net/wiki/SC2155
>Fixes: 08071ec0f113bb1fe8dcc263cb6bf87529e8b76b
>Resolves: https://gitlab.com/libvirt/libvirt/-/issues/839
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> tools/libvirt-guests.sh.in | 34 +++++++++++++++++++++++-----------
> 1 file changed, 23 insertions(+), 11 deletions(-)
>
>diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in
>index f2db1282ad..f57eda66fe 100644
>--- a/tools/libvirt-guests.sh.in
>+++ b/tools/libvirt-guests.sh.in
>@@ -106,7 +106,9 @@ test_connect()
> list_guests() {
>     local uri="$1"
>     local persistent="$2"
>-    local list="$(run_virsh_c "$uri" list --uuid $persistent)"
>+    local list=

You don't need the equals sign here, just:

     local list
     list="$(...)"

also everywhere below.

[...]

>@@ -226,11 +230,13 @@ suspend_guest()
> {
>     local uri="$1"
>     local guest="$2"
>-    local name="$(guest_name "$uri" "$guest")"
>-    local label="$(eval_gettext "Suspending \$name: ")"
>+    local name=
>+    local label=
>     local bypass=
>     local slept=0

here you can just remove all `^local ` and start the function with:

     local uri guest name label bypass slept progress #(from below) etc.

it would be nicer, I think.  If you changed it with a script you could
modify it for all functions, but if you did it manually, then it's a
`good-first-issue` candidate, I guess.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.