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[@]}"
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
>

On Tue, Jul 26, 2022 at 07:13:41AM +0200, Laszlo Ersek
wrote:
> On 07/25/22 16:02, Eric Blake wrote:
> > +    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
Indeed it doesn't work on RHEL 7, eg:

$ make && make -C tests check TESTS=test-eflags.sh

leaves tests/eflags.{err,out} around.

Do you have a patch for this?

[...]
> 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.
Actually I'd like to keep things working on RHEL 7 if possible.  It's
still a widely used RHEL release.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Tue, Jul 26, 2022 at 07:13:41AM +0200, Laszlo Ersek
wrote:
> > 
> > -    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. Okay, namerefs are out.  But indirect variables aren't much better:
> 
> However, I couldn't find a syntax that made this feature work with
array
> variables. In particular, ${!name[@]} stands for "List of array
keys".
On the web, I've found:

var=varname
tmp=${var}[@]
"${!tmp}"

https://mywiki.wooledge.org/BashFAQ/006

but that's still horrible; I'd rather just go for eval at this point.
> 
> 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.
At least RHEL 7 has bash 4.2, which added 'local -ag' for declaring
global arrays even within function scope.  Bash 4.1 (hello RHEL 6)
lacked even that.

At any rate, I'll push the patch to switch to something that works on
bash 4.2 and RHEL 6 shortly.

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