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

[Eric Blake]

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