[PATCH v2 0/3] libvirt-guests.sh: Declare and assign separately to avoid masking return values
v2 of: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/RXHDI... diff to v1: - perfected the syntax-check rule - due to this fewer changes to libvirt-guests.sh.in file are necessary Michal Prívozník (3): libvirt-guests.sh: Declare and assign separately to avoid masking return values libvirt-guest.sh.in: Fix logical error in guest_is_on() syntax-check: Introduce sc_prohibit_local_with_subshell rule build-aux/syntax-check.mk | 11 +++++++++++ tools/libvirt-guests.sh.in | 12 ++++++++---- 2 files changed, 19 insertions(+), 4 deletions(-) -- 2.52.0
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. Fix them. 1: bash_builtins(1) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/libvirt-guests.sh.in | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in index f2db1282ad..e05bfdba61 100644 --- a/tools/libvirt-guests.sh.in +++ b/tools/libvirt-guests.sh.in @@ -106,8 +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 return 1 @@ -482,7 +483,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 +500,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
From: Michal Privoznik <mprivozn@redhat.com> The guest_is_on() function is documented to check whether given domain is running and set guest_running variable accordingly. It does so by running virsh (transitively), then setting the variable and only after that comparing $? variable. This is obviously wrong, because after the guest_running variable assignment the $? variable no longer holds the exit code of virsh. Even worse, as explained in the previous commit, it never held that value in the first place. Fix this by firstly setting the global variable and only after that running virsh. Fixes: 08071ec0f113bb1fe8dcc263cb6bf87529e8b76b Resolves: https://gitlab.com/libvirt/libvirt/-/issues/839 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/libvirt-guests.sh.in | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/libvirt-guests.sh.in b/tools/libvirt-guests.sh.in index e05bfdba61..66a39b9178 100644 --- a/tools/libvirt-guests.sh.in +++ b/tools/libvirt-guests.sh.in @@ -132,9 +132,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 -- 2.52.0
From: Michal Privoznik <mprivozn@redhat.com> In shell, the following function doesn't echo '1' but '0': func() { local var=$(false) echo $? } This is because '$?' does not refer to 'false' but 'local'. The bash_builtins(1) manpage explains it well. And it also mentions other commands behaving the same: export, declare and readonly. Since it is really easy to miss this pattern, introduce a syntax-check rule. Mind you, the following patter (which passes the rule) does check for the subshell exit code: func() { local var var=$(false) echo $? } Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- build-aux/syntax-check.mk | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index f605c9b0e3..022e8e6550 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -394,6 +394,17 @@ sc_prohibit_g_autofree_const: halt='‘g_autofree’ discards ‘const’ qualifier from pointer target type' \ $(_sc_search_regexp) +sc_prohibit_local_with_subshell: + @err=0; for f in $$($(VC_LIST_EXCEPT) | $(GREP) -E '\.sh(\.in)?$$'); do \ + lines=$$( $(AWK) \ + '/^\s*(local|export|declare|readonly)\s+.*=/ { nr=NR; c=1; next } \ + /^\s*$$/ { if (c) next } \ + /\$$\?/ { if (c) { print nr; c=0; } next } \ + { c=0 }' $$f ) ; \ + for l in $$lines; do echo $$f:$$l 1>&2; err=1; done ; \ + done; \ + test $$err -eq 0 || { msg="Declare and assign separately to avoid masking return values" $(_sc_say_and_exit) } + # Many of the function names below came from this filter: # git grep -B2 '\<_('|grep -E '\.c- *[[:alpha:]_][[:alnum:]_]* ?\(.*[,;]$' \ -- 2.52.0
On a Monday in 2026, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
In shell, the following function doesn't echo '1' but '0':
func() { local var=$(false) echo $? }
This is because '$?' does not refer to 'false' but 'local'. The bash_builtins(1) manpage explains it well. And it also mentions other commands behaving the same: export, declare and readonly. Since it is really easy to miss this pattern, introduce a syntax-check rule. Mind you, the following patter (which passes
*pattern Jano
the rule) does check for the subshell exit code:
func() { local var var=$(false) echo $? }
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- build-aux/syntax-check.mk | 11 +++++++++++ 1 file changed, 11 insertions(+)
On a Monday in 2026, Michal Privoznik via Devel wrote:
v2 of:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/RXHDI...
diff to v1: - perfected the syntax-check rule - due to this fewer changes to libvirt-guests.sh.in file are necessary
Michal Prívozník (3): libvirt-guests.sh: Declare and assign separately to avoid masking return values libvirt-guest.sh.in: Fix logical error in guest_is_on() syntax-check: Introduce sc_prohibit_local_with_subshell rule
build-aux/syntax-check.mk | 11 +++++++++++ tools/libvirt-guests.sh.in | 12 ++++++++---- 2 files changed, 19 insertions(+), 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik