Re: [Qemu-devel] [PATCH V3 4/7] qemu-iotests: add 058 internal snapshot export with qemu-nbd case

[Eric Blake]

On 09/25/2013 06:16 PM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <address@hidden>
> ---

> +_export_nbd_snapshot()
> +{
> +    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -l $1 
> &"

Uggh.  Why do you need an eval here?  Especially given that there was
recently a patch to properly quote $TEST_IMG in case the tests are run
inside a directory whose absolute name included a space.  What's wrong
with just directly:

$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port "$TEST_IMG" -l $1 $

> +    NBD_SNAPSHOT_PID=$!
> +    sleep 1
> +}
> +
> +_export_nbd_snapshot1()
> +{
> +    eval "$QEMU_NBD -v -t -b 127.0.0.1 -p $nbd_snapshot_port $TEST_IMG -L 
> snapshot.name=$1 &"

Likewise; and given my complaint on 2-3/7, it would be nicer to support
this with only one option name spelling.

> +_cleanup()
> +{
> +    if [ -n "$NBD_SNAPSHOT_PID" ]; then
> +        kill $NBD_SNAPSHOT_PID
> +    fi
> +     _cleanup_test_img

Kill the TAB, fix the indentation.

> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.pattern
> +
> +# Any format supporting intenal snapshots

s/intenal/internal/

> +_supported_fmt qcow2
> +_supported_proto generic
> +_supported_os Linux

Is this test truly Linux-only?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature