Re: [Qemu-devel] [PATCH v3 3/3] docker: Add test-block

[Fam Zheng <famz@redhat.com>]

On Fri, 09/01 13:35, Eric Blake wrote:
> On 09/01/2017 10:22 AM, Fam Zheng wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  tests/docker/test-block | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >  create mode 100755 tests/docker/test-block
> > 
> > diff --git a/tests/docker/test-block b/tests/docker/test-block
> > new file mode 100755
> > index 0000000000..efc77b4b91
> > --- /dev/null
> > +++ b/tests/docker/test-block
> > @@ -0,0 +1,22 @@
> > +#!/bin/bash -e
> 
> 'set -e' is awful; it exists for backwards-compatibility with shell
> scripts that pre-date functions, but is NOT intuitive and generally does
> NOT do what you want when shell functions are added to the mix.  You're
> better off doing manual error checking than relying on 'set -e' to check
> for errors on your behalf.
> 
> > +#
> > +# Run block test cases
> > +#
> > +# Copyright (c) 2017 Red Hat Inc.
> > +#
> > +# Authors:
> > +#  Fam Zheng <famz@redhat.com>
> > +#
> > +# This work is licensed under the terms of the GNU GPL, version 2
> > +# or (at your option) any later version. See the COPYING file in
> > +# the top-level directory.
> > +
> > +. common.rc
> 
> Are we sure that '.' is early enough on $PATH that this will pick up the
> right common.rc? Safer is '. ./common.rc'.
> 
> > +
> > +cd "$BUILD_DIR"
> > +
> > +build_qemu --target-list=x86_64-softmmu
> > +cd tests/qemu-iotests
> > +for t in raw qcow2 nbd luks; do
> > +    ./check -g quick -$t
> > +done
> 
> That said, this script (and common.rc) is small enough to verify that
> you probably aren't going to trip over those non-intuitive 'set -e'
> behaviors, so the real reason to avoid it is if we want to be able to
> copy-and-paste into other scripts that aren't using 'set -e' (on the
> premise that avoiding implicit global state, like 'set -e', and being
> explicit instead, is a good thing).

OK, I will revise this patch and drop set -e.

Fam