[Libguestfs] [nbdkit PATCH] tests: Better quoting for cleanup_fn

[Laszlo Ersek]

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
>