Re: [Qemu-devel] [PATCH v5 04/10] qemu-iotests: remove file cleanup from bash tests

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5603 bytes --]

On 10/17/2017 11:31 AM, Jeff Cody wrote:
> All files for a given test are now self-contained in a subdirectory,
> and therefore the "./check" script can do all file-related cleanup
> without any help.
> 
> This removes file cleanups from the bash tests.  The only cleanup left
> is whatever is needed to kill any spawned processes; e.g. _cleanup_qemu.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---

>  tests/qemu-iotests/197     |  7 -------

Looks like you have rebased to the current state of the tree.  There may
be a couple more attempting to sneak in before things get merged, but I
trust you'll stay on top of that.

>  156 files changed, 32 insertions(+), 1023 deletions(-)

Fun diffstat, but took me a while to scan through for any mistakes.

> +++ b/tests/qemu-iotests/007
> @@ -27,13 +27,6 @@ echo "QA output created by $seq"
>  here=`pwd`
>  status=1	# failure is the default!
>  
> -_cleanup()
> -{
> -	_cleanup_test_img
> -	true
> -}
> -trap "_cleanup; exit \$status" 0 1 2 3 15

Interesting use of 'true' (why? Since the only call to _cleanup changes
exit status to $status anyways, it does not matter what state $? was
left in after _cleanup)!  Not a problem to see it disappear.

> +++ b/tests/qemu-iotests/028
> @@ -30,14 +30,6 @@ echo "QA output created by $seq"
>  here=`pwd`
>  status=1	# failure is the default!
>  
> -_cleanup()
> -{
> -    _cleanup_qemu
> -    rm -f "${TEST_IMG}.copy"
> -    _cleanup_test_img
> -}
> -trap "_cleanup; exit \$status" 0 1 2 3 15

Is this removal of _cleanup_qemu correct, or does it belong in a
different patch in the series? [1]

> +++ b/tests/qemu-iotests/058
> @@ -29,23 +29,22 @@ echo "QA output created by $seq"
>  here=`pwd`
>  status=1	# failure is the default!
>  
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.pattern
> +
> +_supported_fmt qcow2
> +_supported_proto file
> +_supported_os Linux
> +_require_command QEMU_NBD
> +# Internal snapshots are (currently) impossible with refcount_bits=1
> +_unsupported_imgopts 'refcount_bits=1[^0-9]'
> +

Code motion...

>  nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
>  nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
>  rm -f "${TEST_DIR}/qemu-nbd.pid"
>  
> -_cleanup_nbd()
> -{
> -    local NBD_SNAPSHOT_PID
> -    if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then
> -        read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid"
> -        rm -f "${TEST_DIR}/qemu-nbd.pid"
> -        if [ -n "$NBD_SNAPSHOT_PID" ]; then
> -            kill "$NBD_SNAPSHOT_PID"
> -        fi
> -    fi
> -    rm -f "$nbd_unix_socket"
> -}

...and dropping redundant code that used to be overridden by the common
includes but now would take priority...

> -
>  _wait_for_nbd()
>  {
>      for ((i = 0; i < 300; i++))
> @@ -64,6 +63,7 @@ converted_image=$TEST_IMG.converted
>  _export_nbd_snapshot()
>  {
>      _cleanup_nbd
> +    rm -f "$nbd_unix_socket"
>      $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l $1 &
>      _wait_for_nbd
>  }
> @@ -71,30 +71,11 @@ _export_nbd_snapshot()
>  _export_nbd_snapshot1()
>  {
>      _cleanup_nbd
> +    rm -f "$nbd_unix_socket"

...tweaks to account for the change in common ordering,

>      $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l snapshot.name=$1 &
>      _wait_for_nbd
>  }
>  
> -_cleanup()
> -{
> -    _cleanup_nbd
> -    _cleanup_test_img
> -    rm -f "$converted_image"
> -}
> -trap "_cleanup; exit \$status" 0 1 2 3 15
> -
> -# get standard environment, filters and checks
> -. ./common.rc
> -. ./common.filter
> -. ./common.pattern
> -
> -_supported_fmt qcow2
> -_supported_proto file
> -_supported_os Linux
> -_require_command QEMU_NBD
> -# Internal snapshots are (currently) impossible with refcount_bits=1
> -_unsupported_imgopts 'refcount_bits=1[^0-9]'
> -

...and back to code motion.  This file is different enough from the
usual patterns of cleanups in the rest of the series; should some of
this rearrangement be split into a separate patch?  But only if you have
to respin; if we have nothing else preventing v5 from going in, I don't
think we need the review churn.

> +++ b/tests/qemu-iotests/085
> @@ -37,18 +37,7 @@ snapshot_virt1="snapshot-v1.qcow2"
>  
>  SNAPSHOTS=10
>  
> -_cleanup()
> -{
> -    _cleanup_qemu
> -    for i in $(seq 1 ${SNAPSHOTS})
> -    do
> -        rm -f "${TEST_DIR}/${i}-${snapshot_virt0}"
> -        rm -f "${TEST_DIR}/${i}-${snapshot_virt1}"
> -    done
> -    rm -f "${TEST_IMG}" "${TEST_IMG}.1" "${TEST_IMG}.2" "${TEST_IMG}.base"
> -
> -}
> -trap "_cleanup; exit \$status" 0 1 2 3 15
> +trap "_cleanup_qemu; exit \$status" 0 1 2 3 15

[1] Evidence that the change to 28 may be broken.

> +++ b/tests/qemu-iotests/191
> @@ -31,10 +31,6 @@ MIG_SOCKET="${TEST_DIR}/migrate"
>  
>  _cleanup()
>  {
> -    rm -f "${TEST_IMG}.mid"
> -    rm -f "${TEST_IMG}.ovl2"
> -    rm -f "${TEST_IMG}.ovl3"
> -    _cleanup_test_img
>      _cleanup_qemu
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15

Elsewhere, you removed the _cleanup function entirely, and changed the
trap to directly call _cleanup_qemu.  Worth doing here?

It looks like only 28 was broken, and that fix is an obvious pattern, as
well as any other minor tweaks you want to make based on my comments.
With that correction,
Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]