Re: [PATCH v5 16/16] iotests/image-fleecing: test push backup with fleecing

[Hanna Reitz <hreitz@redhat.com>]

On 28.02.22 12:39, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/tests/image-fleecing     | 120 ++++++++++++++------
>   tests/qemu-iotests/tests/image-fleecing.out |  63 ++++++++++
>   2 files changed, 151 insertions(+), 32 deletions(-)
>
> diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing
> index 33995612be..89c79af698 100755
> --- a/tests/qemu-iotests/tests/image-fleecing
> +++ b/tests/qemu-iotests/tests/image-fleecing

[...]

> @@ -170,6 +196,20 @@ def do_test(use_cbw, use_snapshot_access_filter, base_img_path,
>           log(cmd)
>           log(vm.hmp_qemu_io(qom_path, cmd, qdev=True))
>   
> +    if push_backup:
> +        # Check that previous operations were done during backup, not after
> +        result = vm.qmp('query-block-jobs')
> +        if len(result['return']) != 1:
> +            log('Backup finished too fast, COW is not tested')

I don’t understand why this log is here, its message sounds like “case 
not run”, but first this logged message will make the whole test fail...

> +
> +        result = vm.qmp('block-job-set-speed', device='push-backup', speed=0)
> +        assert result == {'return': {}}

...and then this will fail, too.

Either this is a hard failure, then the log shouldn’t include “COW is 
not tested” (because it is tested, and the case has failed); or it’s a 
casenotrun, and then nothing should be logged (the message should be 
appended to .casenotrun), and the block-job-set-speed call and waiting 
for BLOCK_JOB_COMPLETED should only be done when the job is still in the 
job list.

> +
> +        log(vm.event_wait(name='BLOCK_JOB_COMPLETED',
> +                          match={'data': {'device': 'push-backup'}}),
> +                          filters=[iotests.filter_qmp_event])
> +        log(vm.qmp('blockdev-del', node_name='target'))
> +
>       log('')
>       log('--- Verifying Data ---')
>       log('')
> @@ -177,15 +217,19 @@ def do_test(use_cbw, use_snapshot_access_filter, base_img_path,
>       for p in patterns + zeroes:
>           cmd = 'read -P%s %s %s' % p
>           log(cmd)
> -        out, ret = qemu_io_pipe_and_status('-r', '-f', 'raw', '-c', cmd, nbd_uri)
> -        if ret != 0:
> -            print(out)
> +        if push_backup:
> +            assert qemu_io_silent('-r', '-c', cmd, target_img_path) == 0
> +        else:
> +            out, ret = qemu_io_pipe_and_status('-r', '-f', 'raw', '-c', cmd, nbd_uri)
> +            if ret != 0:
> +                print(out)

The existing principle of “print qemu-io’s output on error” seemed 
perfectly fine to me.  Why not continue using it?

(e.g. like

args = ['-r', '-c', cmd]
if push_backup:
     args += [target_img_path]
else:
     args += ['-f', 'raw', nbd_uri]
out, ret = qemu_io_pipe_and_status(*args)

)

>       log('')
>       log('--- Cleanup ---')
>       log('')
>   
> -    log(vm.qmp('nbd-server-stop'))
> +    if not push_backup:
> +        log(vm.qmp('nbd-server-stop'))
>   
>       if use_cbw:
>           if use_snapshot_access_filter:
> +read -P0xcd 0x3ff0000 64k
> +
> +Done