Re: [Libguestfs] [PATCH nbdkit 4/4] tests: Add a helper function which waits for nbdkit to start up.
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