Re: [Libguestfs] [PATCH nbdkit 4/4] tests: Add a helper function which waits for nbdkit to start up.

On 9/11/18 1:47 PM, Richard W.M. Jones wrote: Is seq universally available, even on BSD? It's not in POSIX. Since you're already using bashisms, you could instead do for (( i=1; i <= $#; i++)); do or even: for i in {1..$#}; do and drop the need for seq or even a fork (the former works regardless of $#, the latter has poor scaling effects if $# is large since it is expanded in advance - but then again, none of our tests pass that large of $#) Unspecified behavior if ${!i} evaluates to "!" (which none of the tests do). Since you're already using bashisms, you could instead do if [[ ${!i} == -P ]]; then This parse loop requires tests to spell it "-P /path/to/file" as two separate args, rather than also permitting "-P/path/to/file". A reasonable restriction but might be worth a comment. You could also change the contract of this function to REQUIRE the pid filename be first, so that you don't have to hunt over $# where -P lives, something like: start_nbdkit() { pidfile=$1 shift nbdkit -P $pidfile "$@" and change callers to look like: start_nbdkit foo.pid --myoption1 myplugin ... I guess we were already relying on seq. Could also be spelled {1..10} More need for source ./functions.sh throughout the patch If the first process starts but the second fails, then you have not set pid1=, and leak the first process. This is a regression from the old code, which managed to capture pid1 before triggering the trap to cleanup(), and thus killed the successful nbdkit. Several tests are impacted. Less importantly, the old code performed startup in parallel (kick off both nbdkit processes, then start the loop; if a pid file was not produced by either one of the nbdkit processes, the test determined that within 10 seconds); now startup is serial (you don't start a second nbdkit until the first one has succeeded). For a test with two nbdkit processes, that could expand typical test time from 1 second to 2 (if the pid file creation is not instantaneous, but the first sleep 1 was sufficient for both processes to be ready); and expand worst-case failure detection under heavy load from 10 into 20 seconds (if the first succeeded on the last iteration, but the second timed out). I don't see anything wrong with the change, although it might be worth a mention in the commit message as intentional. I'm also seeing a common pattern of assigning pid=$(cat file.pid) right after calling start_nbdkt. Would it be worth changing the signature of start_nbdkit() to also populate a pid variable on success, as in: start_nbdkit blocksize1.pid pid1 myarg1 start_nbdkit() { pidfile=$1 pidvar=$2 shift 2 ... pid=$(cat $pidfile) eval $pidvar=$pid which would also have the added benefit of avoiding the regression of pid1 not being set if the second nbdkit process doesn't start? But overall, I like the idea of refactoring the common code for less repetition. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org