Re: [PATCH v2 2/2] iotests: rewrite iotest 240 in python

[Max Reitz <mreitz@redhat.com>]

On 01.11.20 17:15, Maxim Levitsky wrote:
> The recent changes that brought RCU delayed device deletion,
> broke few tests and this test breakage went unnoticed.
> 
> Fix this test by rewriting it in python
> (which allows to wait for DEVICE_DELETED events before continuing).
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  tests/qemu-iotests/240     | 228 ++++++++++++++++---------------------
>  tests/qemu-iotests/240.out |  76 ++++++++-----
>  2 files changed, 143 insertions(+), 161 deletions(-)
> 
> diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240
> index 8b4337b58d..bfc9b72f36 100755
> --- a/tests/qemu-iotests/240
> +++ b/tests/qemu-iotests/240

[...]

> +class TestCase(iotests.QMPTestCase):
> +    test_driver = "null-co"
> +
> +    def required_drivers(self):
> +        return [self.test_driver]
> +
> +    @iotests.skip_if_unsupported(required_drivers)
> +    def setUp(self):
> +        self.vm = iotests.VM()
> +        self.vm.launch()

It seems to me like all tests create a null-co block device.  The only
difference is that test1() creates an R/W node, whereas all others
create an RO node.  I don’t think that matters though, so maybe we can
replace code duplication by creating the (RO) null-co node here.

Furthermore, we could also create two I/O threads and two accompanying
virtio-scsi devices here (tests that don’t use them shouldn’t care)...

> +
> +    def tearDown(self):

...and clean all of those up here.

> +        self.vm.shutdown()
> +
> +    def test1(self):
> +        iotests.log('==Unplug a SCSI disk and then plug it again==')
> +        self.vm.qmp_log('blockdev-add', driver='null-co', read_zeroes=True, node_name='hd0')
> +        self.vm.qmp_log('object-add', qom_type='iothread', id="iothread0")
> +        self.vm.qmp_log('device_add', id='scsi0', driver=iotests.get_virtio_scsi_device(), iothread='iothread0', filters = [iotests.filter_qmp_virtio_scsi])

A bit weird that you change your coding style for the @filters parameter
(i.e., putting spaces around '='), and pylint thinks so, too.

> +        self.vm.qmp_log('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0')
> +        self.vm.qmp_log('device_del', id='scsi-hd0')
> +        self.vm.event_wait('DEVICE_DELETED')
> +        self.vm.qmp_log('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0')
> +        self.vm.qmp_log('device_del', id='scsi-hd0')
> +        self.vm.event_wait('DEVICE_DELETED')
> +        self.vm.qmp_log('device_del', id='scsi0')

I don’t think it makes a difference for this test, but perhaps it would
be cleaner to wait for the DEVICE_DELETED event even for the virtio-scsi
devices?

> +        self.vm.qmp_log('blockdev-del', node_name='hd0')
> +
> +    def test2(self):
> +        iotests.log('==Attach two SCSI disks using the same block device and the same iothread==')
> +        self.vm.qmp_log('blockdev-add', driver='null-co', read_zeroes=True, node_name='hd0', read_only=True)
> +        self.vm.qmp_log('object-add', qom_type='iothread', id="iothread0")
> +        self.vm.qmp_log('device_add', id='scsi0', driver=iotests.get_virtio_scsi_device(), iothread='iothread0', filters = [iotests.filter_qmp_virtio_scsi])
> +
> +        self.vm.qmp_log('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0')
> +        self.vm.qmp_log('device_add', id='scsi-hd1', driver='scsi-hd', drive='hd0')
> +        self.vm.qmp_log('device_del', id='scsi-hd1')
> +        self.vm.event_wait('DEVICE_DELETED')
> +        self.vm.qmp_log('device_del', id='scsi-hd0')
> +        self.vm.event_wait('DEVICE_DELETED')

The bash version deleted them the other way around, but I suppose it
doesn’t matter.  (It just made me wonder why you changed the order.)

> +        self.vm.qmp_log('device_del', id='scsi0')
> +        self.vm.qmp_log('blockdev-del', node_name='hd0')
> +
> +    def test3(self):
> +        iotests.log('==Attach two SCSI disks using the same block device but different iothreads==')
> +
> +        self.vm.qmp_log('blockdev-add', driver='null-co', read_zeroes=True, node_name='hd0', read_only=True)
> +
> +        self.vm.qmp_log('object-add', qom_type='iothread', id="iothread0")
> +        self.vm.qmp_log('object-add', qom_type='iothread', id="iothread1")
> +
> +        self.vm.qmp_log('device_add', id='scsi0', driver=iotests.get_virtio_scsi_device(), iothread='iothread0', filters = [iotests.filter_qmp_virtio_scsi])
> +        self.vm.qmp_log('device_add', id='scsi1', driver=iotests.get_virtio_scsi_device(), iothread='iothread1', filters = [iotests.filter_qmp_virtio_scsi])
> +
> +        self.vm.qmp_log('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0', bus="scsi0.0")
> +        self.vm.qmp_log('device_add', id='scsi-hd1', driver='scsi-hd', drive='hd0', bus="scsi1.0")
> +
> +        self.vm.qmp_log('device_del', id='scsi-hd0')
> +        self.vm.event_wait('DEVICE_DELETED')
> +        self.vm.qmp_log('device_add', id='scsi-hd1', driver='scsi-hd', drive='hd0', bus="scsi1.0")
> +
> +        self.vm.qmp_log('device_del', id='scsi-hd1')
> +        self.vm.event_wait('DEVICE_DELETED')
> +
> +        self.vm.qmp_log('device_del', id='scsi1')
> +        self.vm.qmp_log('device_del', id='scsi0')
> +
> +        self.vm.qmp_log('blockdev-del', node_name='hd0')
> +
> +    def test4(self):
> +        iotests.log('==Attach a SCSI disks using the same block device as a NBD server==')
> +
> +        self.vm.qmp_log('blockdev-add', driver='null-co', read_zeroes=True, node_name='hd0', read_only=True)
> +
> +        self.vm.qmp_log('nbd-server-start',
> +                        filters=[iotests.filter_qmp_testfiles],
> +                        addr={'type':'unix', 'data':{'path':nbd_sock}})
> +
> +        self.vm.qmp_log('nbd-server-add', device='hd0')
> +
> +        self.vm.qmp_log('object-add', qom_type='iothread', id="iothread0")
> +        self.vm.qmp_log('device_add', id='scsi0', driver=iotests.get_virtio_scsi_device(), iothread='iothread0', filters = [iotests.filter_qmp_virtio_scsi])
> +        self.vm.qmp_log('device_add', id='scsi-hd0', driver='scsi-hd', drive='hd0')
> +
> +
> +if __name__ == '__main__':
> +    if 'null-co' not in iotests.supported_formats():
> +        iotests.notrun('null-co driver support missing')

I suppose it isn’t wrong to check this in two places (in the setUp()
decorator, and then here again), but it doesn’t seem necessary either.

Max

> +    iotests.activate_logging()
> +    iotests.main()