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

[Eric Blake]

On 9/11/18 4:14 PM, Richard W.M. Jones wrote:

>> 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.
> 
> So one other idea I had is to have start_nbdkit create a trap function
> to clean up the nbdkit process (we'd still need trap functions in the
> main program for deleting temporary files).  I don't know if this
> would work, but if it does it would fix the above problem.

Calling 'trap' more than once overwrites the previous version of the 
trap; it's rather awkward to query trap to see what is already installed 
in order to append to that before reinstalling a new trap.  But you 
COULD have sourcing the common stuff unconditionally install a generic 
trap handler:

declare -a cleanup_hook
trap cleanup INT QUIT TERM EXIT ERR
cleanup()
{
   for (( i=0; i < ${#cleanup_hook[@]}; i++ )); do
     ${cleanup_hook[i]}
   done
}
# Appends all arguments as a new command on the list of actions run at 
script exit
cleanup_append()
{
   cleanup_hook[${#cleanup_hook[@]}]="$@"
}

then have the various tests do:

cleanup_append myfunc arg

without having to call trap again.  The arguments to cleanup_append will 
undergo word splitting during the cleanup function execution, but it's 
not a full eval so you can't rely on other shell metacharacters.  Then 
again, because you can append shell function names with arguments as one 
of the commands on the list, it's fairly easy to encapsulate any 
test-specific cleanup in a function.

In the case of start_nbdkit, it would mean you set up a secondary 
function that takes a pid file as a name, then call 'cleanup_append 
helper $pidfile' at the appropriate point, where only the helper 
function has to cat $pidfile if it exists.  Then the test files won't 
have to track pid1= variables.

So that looks even nicer than my idea of passing a variable name:

> 
>> 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
> 
> It was nice that start_nbdkit has the same "signature" as
> a regular nbdkit command.

True, you do have that aspect, so continuing to hunt for -P may still be 
worthwhile, especially if the cleanup hook idea means you don't need to 
track a pid= variable.

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