Libguestfs - Jul 2022 - [nbdkit PATCH] tests: Better quoting for cleanup_fn

Testing with:
==#!/bin/bash
. tests/functions.sh
cleanup_fn echo 'a  b'
cleanup_fn echo 'c  d'
==
gives output:
==a b
c d
==
That is, our commands were munged by IFS splitting, because we stored
commands in a flat variable.  Fix it by instead using an array
variable per cleanup_fn invocation.

This does not fix the issue that commands are run in FIFO order; the
comments in the recent test-nbd-client.sh mention that reverse order
might be nicer, however, our existing kill_nbdkit() function assumes
that it can call cleanup_fn during an existing cleanup function and
that such cleanups will still be reached.  Running cleanups in reverse
order from the top level while still allowing multiple rounds of
cleanup once cleanup is started is harder to achieve.
---

If we like this, I'll apply the same patch to libnbd.

Running the cleanups in reverse order is a tougher nut to crack; the
nbdkit testsuite passed 'make check' when I did that (basically, swap
the iteration of the for loop over _i in _run_cleanup_hooks), but that
only tests the success paths, and it is the failure path of
kill_nbdkit() where sane semantics are harder to guarantee with
reverse ordering.

 tests/functions.sh.in | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/functions.sh.in b/tests/functions.sh.in
index 6c5c481a..82f284a7 100644
--- a/tests/functions.sh.in
+++ b/tests/functions.sh.in
@@ -65,15 +65,18 @@ largest_qemu_disk=9223372035781033984
 #
 # Run the command ???cmd [args]??? when the test script exits.  This is
 # run in all cases when the script exits, so is a reliable way to
-# clean up test files, external processes etc.
+# clean up test files, external processes etc.  Cleanup hooks are run
+# in the order of registration.
 #
 # Examples:
 #   cleanup_fn rm -f test.out
 #   cleanup_fn kill $pid
-declare -a _cleanup_hook
+_cleanup_hook_count=0
 cleanup_fn ()
 {
-    _cleanup_hook[${#_cleanup_hook[@]}]="$@"
+    local _hook=_cleanup_hook$((_cleanup_hook_count++))
+    declare -ag $_hook
+    eval "$_hook=(\"\$@\")"
 }

 _run_cleanup_hooks ()
@@ -84,8 +87,9 @@ _run_cleanup_hooks ()
     trap '' INT QUIT TERM EXIT ERR
     echo $0: run cleanup hooks: exit code $_status

-    for (( _i = 0; _i < ${#_cleanup_hook[@]}; ++_i )); do
-        ${_cleanup_hook[_i]}
+    for (( _i = 0; _i < $_cleanup_hook_count; ++_i )); do
+        local -n _hook=_cleanup_hook$_i
+        "${_hook[@]}"
     done

     exit $_status
-- 
2.37.1

On 07/25/22 16:02, Eric Blake wrote:
> Testing with:
> ==> #!/bin/bash
> . tests/functions.sh
> cleanup_fn echo 'a  b'
> cleanup_fn echo 'c  d'
> ==> 
> gives output:
> ==> a b
> c d
> ==> 
> That is, our commands were munged by IFS splitting, because we stored
> commands in a flat variable.  Fix it by instead using an array
> variable per cleanup_fn invocation.
> 
> This does not fix the issue that commands are run in FIFO order; the
> comments in the recent test-nbd-client.sh mention that reverse order
> might be nicer, however, our existing kill_nbdkit() function assumes
> that it can call cleanup_fn during an existing cleanup function and
> that such cleanups will still be reached.  Running cleanups in reverse
> order from the top level while still allowing multiple rounds of
> cleanup once cleanup is started is harder to achieve.
> ---
> 
> If we like this, I'll apply the same patch to libnbd.
> 
> Running the cleanups in reverse order is a tougher nut to crack; the
> nbdkit testsuite passed 'make check' when I did that (basically,
swap
> the iteration of the for loop over _i in _run_cleanup_hooks), but that
> only tests the success paths, and it is the failure path of
> kill_nbdkit() where sane semantics are harder to guarantee with
> reverse ordering.
> 
>  tests/functions.sh.in | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/functions.sh.in b/tests/functions.sh.in
> index 6c5c481a..82f284a7 100644
> --- a/tests/functions.sh.in
> +++ b/tests/functions.sh.in
> @@ -65,15 +65,18 @@ largest_qemu_disk=9223372035781033984
>  #
>  # Run the command ?cmd [args]? when the test script exits.  This is
>  # run in all cases when the script exits, so is a reliable way to
> -# clean up test files, external processes etc.
> +# clean up test files, external processes etc.  Cleanup hooks are run
> +# in the order of registration.
>  #
>  # Examples:
>  #   cleanup_fn rm -f test.out
>  #   cleanup_fn kill $pid
> -declare -a _cleanup_hook
> +_cleanup_hook_count=0
>  cleanup_fn ()
>  {
> -    _cleanup_hook[${#_cleanup_hook[@]}]="$@"
> +    local _hook=_cleanup_hook$((_cleanup_hook_count++))
> +    declare -ag $_hook
> +    eval "$_hook=(\"\$@\")"
>  }
> 
>  _run_cleanup_hooks ()
> @@ -84,8 +87,9 @@ _run_cleanup_hooks ()
>      trap '' INT QUIT TERM EXIT ERR
>      echo $0: run cleanup hooks: exit code $_status
> 
> -    for (( _i = 0; _i < ${#_cleanup_hook[@]}; ++_i )); do
> -        ${_cleanup_hook[_i]}
> +    for (( _i = 0; _i < $_cleanup_hook_count; ++_i )); do
> +        local -n _hook=_cleanup_hook$_i
> +        "${_hook[@]}"
>      done
> 
>      exit $_status
> 
> 
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
> 
Reviewed-by: Laszlo Ersek <lersek at redhat.com>

On 07/25/22 16:02, Eric Blake wrote:
> Testing with:
> ==> #!/bin/bash
> . tests/functions.sh
> cleanup_fn echo 'a  b'
> cleanup_fn echo 'c  d'
> ==> 
> gives output:
> ==> a b
> c d
> ==> 
> That is, our commands were munged by IFS splitting, because we stored
> commands in a flat variable.  Fix it by instead using an array
> variable per cleanup_fn invocation.
> 
> This does not fix the issue that commands are run in FIFO order; the
> comments in the recent test-nbd-client.sh mention that reverse order
> might be nicer, however, our existing kill_nbdkit() function assumes
> that it can call cleanup_fn during an existing cleanup function and
> that such cleanups will still be reached.  Running cleanups in reverse
> order from the top level while still allowing multiple rounds of
> cleanup once cleanup is started is harder to achieve.
> ---
> 
> If we like this, I'll apply the same patch to libnbd.
> 
> Running the cleanups in reverse order is a tougher nut to crack; the
> nbdkit testsuite passed 'make check' when I did that (basically,
swap
> the iteration of the for loop over _i in _run_cleanup_hooks), but that
> only tests the success paths, and it is the failure path of
> kill_nbdkit() where sane semantics are harder to guarantee with
> reverse ordering.
> 
>  tests/functions.sh.in | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/functions.sh.in b/tests/functions.sh.in
> index 6c5c481a..82f284a7 100644
> --- a/tests/functions.sh.in
> +++ b/tests/functions.sh.in
> @@ -65,15 +65,18 @@ largest_qemu_disk=9223372035781033984
>  #
>  # Run the command ?cmd [args]? when the test script exits.  This is
>  # run in all cases when the script exits, so is a reliable way to
> -# clean up test files, external processes etc.
> +# clean up test files, external processes etc.  Cleanup hooks are run
> +# in the order of registration.
>  #
>  # Examples:
>  #   cleanup_fn rm -f test.out
>  #   cleanup_fn kill $pid
> -declare -a _cleanup_hook
> +_cleanup_hook_count=0
>  cleanup_fn ()
>  {
> -    _cleanup_hook[${#_cleanup_hook[@]}]="$@"
> +    local _hook=_cleanup_hook$((_cleanup_hook_count++))
> +    declare -ag $_hook
> +    eval "$_hook=(\"\$@\")"
>  }
> 
>  _run_cleanup_hooks ()
> @@ -84,8 +87,9 @@ _run_cleanup_hooks ()
>      trap '' INT QUIT TERM EXIT ERR
>      echo $0: run cleanup hooks: exit code $_status
> 
> -    for (( _i = 0; _i < ${#_cleanup_hook[@]}; ++_i )); do
> -        ${_cleanup_hook[_i]}
> +    for (( _i = 0; _i < $_cleanup_hook_count; ++_i )); do
> +        local -n _hook=_cleanup_hook$_i
> +        "${_hook[@]}"
BTW, "nameref" variables seem like a relatively new addition to bash;
for example RHEL7 does not have them. Interestingly, the RHEL7 manual
also does not document a nameref-like construct that *does* work in
RHEL7, namely:

$ bar=foo
$ baz=bar
$ echo ${!baz}
foo

Oh wait, the RHEL7 manual does document it, my search-fu wasn't good
enough. It's called "indirect expansion":

       If  the  first  character  of parameter is an exclamation point
       (!), a level of variable indirection is introduced.  Bash  uses
       the  value of the variable formed from the rest of parameter as
       the name of the variable; this variable is  then  expanded  and
       that value is used in the rest of the substitution, rather than
       the value of parameter  itself.   This  is  known  as  indirect
       expansion.   The  exceptions  to  this  are  the  expansions of
       ${!prefix*} and ${!name[@]} described below.   The  exclamation
       point must immediately follow the left brace in order to intro?
       duce indirection.

However, I couldn't find a syntax that made this feature work with array
variables. In particular, ${!name[@]} stands for "List of array keys".

I wonder if the bash release notes mention "nameref" as a more
flexible
version of "indirect expansion"... Hm, nameref was new in bash-4.3
<https://lwn.net/Articles/589566/>, and RHEL7 has bash-4.2.46-35.el7_9,
so the lack of "nameref" is understandable (not that it matters for
upstream nbdkit :)); indirect expansion is not mentioned however as a
similar feature.

The above section of the manual got updated for namerefs, FWIW:

https://www.gnu.org/software/bash/manual/bash.html#Shell-Parameter-Expansion

       If the first character of parameter is an exclamation point (!),
       and parameter is not a nameref, it introduces a level of
       indirection [...]

Laszlo
>      done
> 
>      exit $_status
> 
> 
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
>