Re: [PATCH 21/21] itotests/222: add test-case for copy-before-write filter

[Max Reitz <mreitz@redhat.com>]

On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
> New fleecing method becomes available: copy-before-write filter.
> 
> Actually we don't need backup job to setup image fleecing. Add test
> fore new recommended way of image fleecing.

*for

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   tests/qemu-iotests/222     | 56 ++++++++++++++++++++++-------
>   tests/qemu-iotests/222.out | 72 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 115 insertions(+), 13 deletions(-)

Looks good, just wondering about some variable usage (why is src_node a 
BB name?) and whether the virtio-blk qdev path couldn’t be expressed in 
some nicer way.

> diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
> index b48afe623e..8f8e1e4d7f 100755
> --- a/tests/qemu-iotests/222
> +++ b/tests/qemu-iotests/222
> @@ -48,11 +48,13 @@ remainder = [("0xd5", "0x108000",  "32k"), # Right-end of partial-left [1]
>                ("0xdc", "32M",       "32k"), # Left-end of partial-right [2]
>                ("0xcd", "0x3ff0000", "64k")] # patterns[3]
>   
> -with iotests.FilePath('base.img') as base_img_path, \
> -     iotests.FilePath('fleece.img') as fleece_img_path, \
> -     iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock_path, \
> -     iotests.VM() as vm:
> +def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
> +    if use_cbw:
> +        log('=== Test filter based fleecing ===')
> +    else:
> +        log('=== Test backup(sync=none) based fleecing ===')
>   
> +    log('')
>       log('--- Setting up images ---')
>       log('')
>   
> @@ -69,7 +71,7 @@ with iotests.FilePath('base.img') as base_img_path, \
>       log('--- Launching VM ---')
>       log('')
>   
> -    vm.add_drive(base_img_path)
> +    vm.add_drive(base_img_path, "node-name=node0")
>       vm.launch()
>       log('Done')
>

Afterwards, src_node is set to 'drive0'.  So src_node is actually a BB 
name...

> @@ -91,11 +93,22 @@ with iotests.FilePath('base.img') as base_img_path, \
>           "backing": src_node,
>       }))
>   
> -    # Establish COW from source to fleecing node
> -    log(vm.qmp("blockdev-backup",
> -               device=src_node,
> -               target=tgt_node,
> -               sync="none"))
> +    # Establish CBW from source to fleecing node
> +    if use_cbw:
> +        log(vm.qmp("blockdev-add", **{
> +            "driver": "copy-before-write",
> +            "node-name": "fl-cbw",
> +            "file": src_node,
> +            "target": tgt_node

Which makes this look strange, and also TIL (or maybe “today I was 
reminded”?) that you can use BB names as node references.

(It’s also already weird in the `"backing": src_node` line right at the 
beginning of this hunk...)

So I guess it works, but I’d find it would be nicer to distinguish 
between the node name and the BB name.

> +        }))
> +
> +        log(vm.qmp("qom-set", path="/machine/peripheral-anon/device[0]",
> +                   property="drive", value="fl-cbw"))

Is that path portable across targets?  Should we maybe instead manually 
add a virtio-scsi device that we can give a proper name?

Or I suppose we could do

path = next(dev for dev in vm.qmp('query-block')['return']
             if dev['device'] == 'drive0')['qdev']

> +    else:
> +        log(vm.qmp("blockdev-backup",
> +                   device=src_node,
> +                   target=tgt_node,
> +                   sync="none"))
>   
>       log('')
>       log('--- Setting up NBD Export ---')
> @@ -139,9 +152,15 @@ with iotests.FilePath('base.img') as base_img_path, \
>       log('--- Cleanup ---')
>       log('')
>   
> -    log(vm.qmp('block-job-cancel', device=src_node))
> -    log(vm.event_wait('BLOCK_JOB_CANCELLED'),
> -        filters=[iotests.filter_qmp_event])
> +    if use_cbw:
> +        log(vm.qmp("qom-set", path="/machine/peripheral-anon/device[0]",
> +                   property="drive", value="node0"))

If src_node were 'node0', we wouldn’t need to use the 'node0' literal 
here and could use src_node instead.  (Because it kind of seems like 
this test wants to use those variables instead of literals.  I mean, we 
could also just call the node 'src-node', but, well.)

Max

> +        log(vm.qmp("blockdev-del", node_name="fl-cbw"))
> +    else:
> +        log(vm.qmp('block-job-cancel', device=src_node))
> +        log(vm.event_wait('BLOCK_JOB_CANCELLED'),
> +            filters=[iotests.filter_qmp_event])
> +
>       log(vm.qmp('nbd-server-stop'))
>       log(vm.qmp('blockdev-del', node_name=tgt_node))
>       vm.shutdown()