Libguestfs - Sep 2018 - Re: [PATCH v2 nbdkit 5/5] tests: Add a helper function which waits for nbdkit to start up.

On 9/13/18 11:09 AM, Richard W.M. Jones wrote:
> This assumes bashisms, but bash is required to run the tests.
> 
> This is mostly refactoring.  However the changes (simplifications) are
> quite substantial:
> 
>   - Since the new start_nbdkit helper function cleans up nbdkit on
>     exit, most scripts no longer need to deal with the pid or kill the
>     pid in the cleanup function.
> 
>   - As a result, cleanup functions are radically simpler, and often
>     disappear completely.
> 
>   - Also removed the comment "# The cleanup() function is called
>     implicitly on exit" passim partly because the cleanup functions
>     have mostly been removed and partly because it should be clear from
>     looking at functions.sh.
> 
> There is one additional change: In the test-memory*.sh tests, nbdkit
> used to run in the foreground, but that seems to be a consequence of
> some left over debugging.  I changed it so they work like the other
> tests.
> ---
> +# start_nbdkit -P pidfile args...
> +#
> +# Run nbdkit with args and wait for it to start up.  If it fails to
> +# start up, exit with an error message.  Also a cleanup handler is
> +# installed automatically which kills nbdkit on exit.
> +start_nbdkit ()
> +{
> +    # -P <pidfile> must be the first two parameters.
> +    if [ "$1" != "-P" ]; then
> +       echo "$0: start_nbdkit: -P <pidfile> option must be
first"
> +       exit 1
> +    fi
> +    pidfile="$2"
Although none of the tests passes a space within $2, it's better to 
consistently quote...
> +
> +    # Run nbdkit.
> +    nbdkit "$@"
> +
> +    # Wait for the pidfile to appear.
> +    for i in {1..10}; do
> +        if test -f "$pidfile"; then
> +            break
> +        fi
> +        sleep 1
> +    done
> +    if ! test -f "$pidfile"; then
> +        echo "$0: start_nbdkit: PID file $pidfile was not
created"
> +        exit 1
> +    fi
> +
> +    # Kill nbdkit on exit.
> +    cleanup_fn kill "$(cat $pidfile)"
...and thus this should be "$(cat "$pidfile")"
> +++ b/tests/test-ip.sh
> -if ! test -f ip.pid; then
> -    echo "$0: PID file was not created"
> -    exit 1
> -fi
> -
> +start_nbdkit -P ip.pid -p $port example1
>   pid="$(cat ip.pid)"
>   
>   # Check the process exists.
     kill -s 0 $pid

Hmm. Should the 'kill -s 0 $pid' be moved into the common code for all 
tests to benefit from (detecting early crashes where nbdkit started and 
created the pidfile, but then died)?  If so, you don't need $pid in this 
file any longer.
> +++ b/tests/test-start.sh
> -if ! test -f start.pid; then
> -    echo "$0: PID file was not created"
> -    exit 1
> -fi
> -
> +start_nbdkit -P start.pid -U start.sock example1
>   pid="$(cat start.pid)"
>   
>   # Check the process exists.
and another one.


The stuff I found is minor enough that I don't see a problem with you 
fixing and committing the series, rather than going through a v3.  And I 
love the overall diffstat :)

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

On Thu, Sep 13, 2018 at 01:35:11PM -0500, Eric Blake
wrote:
> 
> ...and thus this should be "$(cat "$pidfile")"
Woah wait, this actually works?!

(Tests ...)

It does!  Never knew that.

I'll fix this ...
> >+++ b/tests/test-ip.sh
> 
> >-if ! test -f ip.pid; then
> >-    echo "$0: PID file was not created"
> >-    exit 1
> >-fi
> >-
> >+start_nbdkit -P ip.pid -p $port example1
> >  pid="$(cat ip.pid)"
> >  # Check the process exists.
>     kill -s 0 $pid
> 
> Hmm. Should the 'kill -s 0 $pid' be moved into the common code for
> all tests to benefit from (detecting early crashes where nbdkit
> started and created the pidfile, but then died)?  If so, you don't
> need $pid in this file any longer.
> 
> >+++ b/tests/test-start.sh
> 
> >-if ! test -f start.pid; then
> >-    echo "$0: PID file was not created"
> >-    exit 1
> >-fi
> >-
> >+start_nbdkit -P start.pid -U start.sock example1
> >  pid="$(cat start.pid)"
> >  # Check the process exists.
> 
> and another one.
These are problems in the existing tests, so a job for another day.
> The stuff I found is minor enough that I don't see a problem with
> you fixing and committing the series, rather than going through a
> v3.  And I love the overall diffstat :)
Thanks for the review,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 9/13/18 1:54 PM, Richard W.M. Jones wrote:
> On Thu, Sep 13, 2018 at 01:35:11PM -0500, Eric Blake wrote:
>>
>> ...and thus this should be "$(cat "$pidfile")"
> 
> Woah wait, this actually works?!
> 
> (Tests ...)
$() is MUCH nicer than `` for nesting commands :)  You don't have to 
think about nesting "" or adding \ to random places.
> 
> It does!  Never knew that.
> 
> I'll fix this ...
> 
>>> +start_nbdkit -P ip.pid -p $port example1
>>>   pid="$(cat ip.pid)"
>>>   # Check the process exists.
>>      kill -s 0 $pid
>>
>> Hmm. Should the 'kill -s 0 $pid' be moved into the common code
for
>> all tests to benefit from (detecting early crashes where nbdkit
>> started and created the pidfile, but then died)?  If so, you don't
>> need $pid in this file any longer.
>>
>>> +++ b/tests/test-start.sh
>>
>>> -if ! test -f start.pid; then
>>> -    echo "$0: PID file was not created"
>>> -    exit 1
>>> -fi
>>> -
>>> +start_nbdkit -P start.pid -U start.sock example1
>>>   pid="$(cat start.pid)"
>>>   # Check the process exists.
>>
>> and another one.
> 
> These are problems in the existing tests, so a job for another day.
If checking that the process exists is common, then start_nbdkit should 
do it.  Otherwise, what makes these two tests special that they have to 
check, when the others don't?

But yes, I'm okay with it being an additional cleanup for another day.

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