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

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 Mon, Jul 25, 2022 at 04:13:41PM +0200, Laszlo Ersek wrote:
> > 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.
> > 
> 
> Reviewed-by: Laszlo Ersek <lersek at redhat.com>
>
In nbdkit as 6699ca24
In libnbd as 0cf89848

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org