Re: [Qemu-devel] [PATCH for-2.11 2/3] qemu-iotests: remove file cleanup from bash tests

[Eric Blake <eblake@redhat.com>]

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

On 07/31/2017 12:04 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/189     |  6 ------
>  148 files changed, 18 insertions(+), 938 deletions(-)

Fun diffstat!

My test 190 is on Kevin's queue, presumably for 2.10; that will also
need this cleanup.
https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg08567.html

> +++ b/tests/qemu-iotests/048
> @@ -27,14 +27,6 @@ echo "QA output created by $seq"
>  
>  status=1        # failure is the default!
>  
> -_cleanup()
> -{
> -    echo "Cleanup"

Interesting outlier for being verbose about cleanup.

> -    _cleanup_test_img
> -    rm "${TEST_IMG_FILE2}"
> -}
> -trap "_cleanup; exit \$status" 0 1 2 3 15
> -
>  _compare()
>  {
>      $QEMU_IMG compare $QEMU_IMG_EXTRA_ARGS "$@" "$TEST_IMG" "${TEST_IMG2}"
> diff --git a/tests/qemu-iotests/048.out b/tests/qemu-iotests/048.out
> index 0bcf663..3318eed 100644
> --- a/tests/qemu-iotests/048.out
> +++ b/tests/qemu-iotests/048.out
> @@ -39,4 +39,3 @@ wrote 512/512 bytes at offset 512
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  Content mismatch at offset 512!
>  1
> -Cleanup

And evidence that you tested your change.

> +++ b/tests/qemu-iotests/058
> @@ -79,7 +79,6 @@ _cleanup()
>  {
>      _cleanup_nbd
>      _cleanup_test_img
> -    rm -f "$converted_image"
>  }
>  trap "_cleanup; exit \$status" 0 1 2 3 15

I understand keeping _cleanup_nbd in this exit trap; but should we
remove the _cleanup_test_img so that the temporary files can be left
behind after 3/3?

> +++ 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

Nice what the subdirectory lets you skip.


> +++ b/tests/qemu-iotests/091
> @@ -31,14 +31,6 @@ status=1    # failure is the default!
>  
>  MIG_FIFO="${TEST_DIR}/migrate"
>  
> -_cleanup()
> -{
> -    rm -f "${MIG_FIFO}"
> -    _cleanup_qemu
> -    _cleanup_test_img
> -}
> -trap "_cleanup; exit \$status" 0 1 2 3 15

Isn't _cleanup_qemu important here (especially given that you preserved
it elsewhere)?

> +++ b/tests/qemu-iotests/104
> @@ -27,8 +27,6 @@ echo "QA output created by $seq"
>  here=`pwd`
>  status=1	# failure is the default!
>  
> -trap "exit \$status" 0 1 2 3 15
> -

Unusual to install a trap like that.  Good riddance!

> +++ b/tests/qemu-iotests/182
> @@ -28,11 +28,7 @@ here="$PWD"
>  tmp=/tmp/$$
>  status=1	# failure is the default!
>  
> -_cleanup()
> -{
> -    _cleanup_test_img
> -}
> -trap "_cleanup; exit \$status" 0 1 2 3 15
> +trap "_cleanup_qemu; exit \$status" 0 1 2 3 15

Umm, why is _cleanup_qemu added?

Overall, looks nice. Given my comments, it will need a v2, preferably
rebased on top of Kevin's branch (if that hasn't landed yet)

-- 
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 --]