Re: [PATCH hci-dev 4/6] tests/qtest/virtio-scsi: cover a replacement that changes the size #VSTOR-130961

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] qtest: replaced-node tests never check equal-size or multi-parent notification
--

Subject: Re: [PATCH] tests/qtest/virtio-scsi: cover a replacement that changes the size #VSTOR-130961

commit dfa745e49f7f99a718150defda33d68b4cb092a9
Author: Denis V. Lunev <den@openvz.org>

tests/qtest/virtio-scsi: cover a replacement that changes the size #VSTOR-130961

This adds a qtest that realizes a scsi-hd on a 1 MiB null-co node, replaces
it with a 128 MiB node via qom-set, and checks for the CAPACITY DATA HAS
CHANGED unit attention followed by a successful read that only fits in the
new node.

Message-ID: <20260812121500.1034178-4-den@openvz.org>

> diff --git a/tests/qtest/virtio-scsi-test.c b/tests/qtest/virtio-scsi-test.c
> --- a/tests/qtest/virtio-scsi-test.c
> +++ b/tests/qtest/virtio-scsi-test.c

[ ... ]

> +/* Test replacing the backing node with one of a different size */
> +static void test_replaced_node_size(void *obj, void *data,
> +                                    QGuestAllocator *t_alloc)
> +{

[ ... ]

> +    /* Swap in a 128 MiB node; the device is not reset by this */
> +    qtest_qmp_assert_success(global_qtest,
> +                             "{'execute': 'qom-set', 'arguments': {"
> +                             "  'path': '/machine/peripheral/scsi-hd0',"
> +                             "  'property': 'drive', 'value': 'big'}}");

[Severity: Low]
This isn't a bug, but is the coverage here complete enough?

test_replaced_node_size() only exercises a size-changing replacement
(small at 1 MiB to big at 128 MiB) with a single parent, scsi-hd0.

The notification is issued from bdrv_replace_resize_commit() in block.c:

    if (old_len >= 0 && new_len >= 0 && new_len != old_len) {
        bdrv_parent_cb_resize(s->new_bs);
    }

Would this test still pass if the new_len != old_len check were dropped?
An equal-size replacement would then send a spurious capacity-changed unit
attention, and nothing here would catch it.

Also, bdrv_parent_cb_resize() goes through bdrv_co_parent_cb_resize(),
which notifies every parent of new_bs, not only the replaced child.  Would
it be worth adding a case where new_bs has a second parent, to check that
behaviour?

Looking at the final state of the series, the later patches only add
further size-changing replacements (node4 at 2 MiB in the qom-set-drive
iotest, and the huge node via vz-x-blockdev-replace).  I did not see an
equal-size case or a multi-parent case added anywhere in the series.

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261104134539.1519657-1-den@openvz.org?part=4