[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@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Reviewed-by: Laszlo Ersek <lersek@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@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
On Mon, Jul 25, 2022 at 10:01:21AM -0500, Eric Blake wrote:
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@redhat.com>
In nbdkit as 6699ca24 In libnbd as 0cf89848
Patch looks good, thanks. I agree about reversing the cleanups - hard to know where all the implicit assumptions could be, especially in failure cases. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
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@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 07/26/22 10:11, Richard W.M. Jones wrote:
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?
This should work: local hook=_cleanup_hook$_i eval "\${${hook}[@]}" Thanks Laszlo
[...]
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.
On 07/26/22 12:17, Laszlo Ersek wrote:
On 07/26/22 10:11, Richard W.M. Jones wrote:
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?
This should work:
local hook=_cleanup_hook$_i eval "\${${hook}[@]}"
Sigh, nesting quoting is always fun; I forgot the outermost quotes. So please make that: eval "\"\${${hook}[@]}\""
Thanks Laszlo
[...]
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.
On Tue, Jul 26, 2022 at 12:22:11PM +0200, Laszlo Ersek wrote:
On 07/26/22 12:17, Laszlo Ersek wrote:
On 07/26/22 10:11, Richard W.M. Jones wrote:
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?
This should work:
local hook=_cleanup_hook$_i eval "\${${hook}[@]}"
Sigh, nesting quoting is always fun; I forgot the outermost quotes. So please make that:
eval "\"\${${hook}[@]}\""
So after quotes and variables are done this expands to: eval "${_cleanup_hook_X[@]}" It looks right to me -- and works in my limited test -- but I guess I'll let Eric decide on this one. Rich.
Thanks Laszlo
[...]
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 virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Tue, Jul 26, 2022 at 09:11:24AM +0100, Richard W.M. Jones wrote:
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?
This might work, although I'm not sure if the eval is safe ... diff --git a/tests/functions.sh.in b/tests/functions.sh.in index 82f284a..ab44128 100644 --- a/tests/functions.sh.in +++ b/tests/functions.sh.in @@ -88,8 +88,8 @@ _run_cleanup_hooks () echo $0: run cleanup hooks: exit code $_status for (( _i = 0; _i < $_cleanup_hook_count; ++_i )); do - local -n _hook=_cleanup_hook$_i - "${_hook[@]}" + local _hook=_cleanup_hook$_i + eval "\${$_hook[@]}" done exit $_status Rich.
[...]
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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
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
On Tue, Jul 26, 2022 at 06:56:04AM -0500, Eric Blake wrote:
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.
Works for me on RHEL 7, thanks. Rich.
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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones