[Qemu-devel] [PATCH 0/3] iotests: More media change tests

[Qemu-devel] [PATCH 0/3] iotests: More media change tests

[Kevin Wolf]

This series is motivated by the bug fixed in commit 7cef3d1290
('scsi-cd: Fix inserting read-only media in empty drive'). After the
series is applied, media change is tested for all combinations of
floppy/ide-cd/scsi-cd with -drive/-blockdev and initially empty
drive/inserted media.

Kevin Wolf (3):
  iotests/118: Test media change for scsi-cd
  iotests/118: Create test classes dynamically
  iotests/118: Add -blockdev based tests

 tests/qemu-iotests/118     | 84 +++++++++++++++++++++-----------------
 tests/qemu-iotests/118.out |  4 +-
 2 files changed, 49 insertions(+), 39 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/3] iotests/118: Test media change for scsi-cd

[Kevin Wolf]

The test covered only floppy and ide-cd. Add scsi-cd as well.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/118     | 20 ++++++++++++++++++++
 tests/qemu-iotests/118.out |  4 ++--
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index 499c5f0901..3c20d2d61f 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -33,6 +33,8 @@ def interface_to_device_name(interface):
         return 'ide-cd'
     elif interface == 'floppy':
         return 'floppy'
+    elif interface == 'scsi':
+        return 'scsi-cd'
     else:
         return None
 
@@ -297,6 +299,8 @@ class TestInitiallyFilled(GeneralChangeTestsBaseClass):
         qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
         self.vm = iotests.VM()
         self.vm.add_drive(old_img, 'media=%s' % media, 'none')
+        if interface == 'scsi':
+            self.vm.add_device('virtio-scsi-pci')
         self.vm.add_device('%s,drive=drive0,id=%s' %
                            (interface_to_device_name(interface),
                             self.device_name))
@@ -330,6 +334,8 @@ class TestInitiallyEmpty(GeneralChangeTestsBaseClass):
     def setUp(self, media, interface):
         qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
         self.vm = iotests.VM().add_drive(None, 'media=%s' % media, 'none')
+        if interface == 'scsi':
+            self.vm.add_device('virtio-scsi-pci')
         self.vm.add_device('%s,drive=drive0,id=%s' %
                            (interface_to_device_name(interface),
                             self.device_name))
@@ -363,6 +369,20 @@ class TestCDInitiallyEmpty(TestInitiallyEmpty):
     def setUp(self):
         self.TestInitiallyEmpty.setUp(self, 'cdrom', 'ide')
 
+class TestSCSICDInitiallyFilled(TestInitiallyFilled):
+    TestInitiallyFilled = TestInitiallyFilled
+    has_real_tray = True
+
+    def setUp(self):
+        self.TestInitiallyFilled.setUp(self, 'cdrom', 'scsi')
+
+class TestSCSICDInitiallyEmpty(TestInitiallyEmpty):
+    TestInitiallyEmpty = TestInitiallyEmpty
+    has_real_tray = True
+
+    def setUp(self):
+        self.TestInitiallyEmpty.setUp(self, 'cdrom', 'scsi')
+
 class TestFloppyInitiallyFilled(TestInitiallyFilled):
     TestInitiallyFilled = TestInitiallyFilled
     has_real_tray = False
diff --git a/tests/qemu-iotests/118.out b/tests/qemu-iotests/118.out
index 4823c113d5..b4ff997a8c 100644
--- a/tests/qemu-iotests/118.out
+++ b/tests/qemu-iotests/118.out
@@ -1,5 +1,5 @@
-...............................................................
+.........................................................................................
 ----------------------------------------------------------------------
-Ran 63 tests
+Ran 89 tests
 
 OK
-- 
2.20.1

[Qemu-devel] [PATCH 2/3] iotests/118: Create test classes dynamically

[Kevin Wolf]

We're getting a ridiculous number of child classes of
TestInitiallyFilled and TestInitiallyEmpty that differ only in a few
attributes that we want to test in all combinations.

Instead of explicitly writing down every combination, let's use a loop
and create those classes dynamically.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/118 | 69 +++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 48 deletions(-)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index 3c20d2d61f..c281259215 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -294,15 +294,15 @@ class GeneralChangeTestsBaseClass(ChangeBaseClass):
 class TestInitiallyFilled(GeneralChangeTestsBaseClass):
     was_empty = False
 
-    def setUp(self, media, interface):
+    def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k')
         qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
         self.vm = iotests.VM()
-        self.vm.add_drive(old_img, 'media=%s' % media, 'none')
-        if interface == 'scsi':
+        self.vm.add_drive(old_img, 'media=%s' % self.media, 'none')
+        if self.interface == 'scsi':
             self.vm.add_device('virtio-scsi-pci')
         self.vm.add_device('%s,drive=drive0,id=%s' %
-                           (interface_to_device_name(interface),
+                           (interface_to_device_name(self.interface),
                             self.device_name))
         self.vm.launch()
 
@@ -331,13 +331,13 @@ class TestInitiallyFilled(GeneralChangeTestsBaseClass):
 class TestInitiallyEmpty(GeneralChangeTestsBaseClass):
     was_empty = True
 
-    def setUp(self, media, interface):
+    def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
-        self.vm = iotests.VM().add_drive(None, 'media=%s' % media, 'none')
-        if interface == 'scsi':
+        self.vm = iotests.VM().add_drive(None, 'media=%s' % self.media, 'none')
+        if self.interface == 'scsi':
             self.vm.add_device('virtio-scsi-pci')
         self.vm.add_device('%s,drive=drive0,id=%s' %
-                           (interface_to_device_name(interface),
+                           (interface_to_device_name(self.interface),
                             self.device_name))
         self.vm.launch()
 
@@ -355,50 +355,23 @@ class TestInitiallyEmpty(GeneralChangeTestsBaseClass):
         # Should be a no-op
         self.assert_qmp(result, 'return', {})
 
-class TestCDInitiallyFilled(TestInitiallyFilled):
-    TestInitiallyFilled = TestInitiallyFilled
-    has_real_tray = True
-
-    def setUp(self):
-        self.TestInitiallyFilled.setUp(self, 'cdrom', 'ide')
-
-class TestCDInitiallyEmpty(TestInitiallyEmpty):
-    TestInitiallyEmpty = TestInitiallyEmpty
-    has_real_tray = True
-
-    def setUp(self):
-        self.TestInitiallyEmpty.setUp(self, 'cdrom', 'ide')
+# Do this in a function to avoid leaking variables like case into the global
+# name space (otherwise tests would be run for the abstract base classes)
+def create_basic_test_classes():
+    for (media, interface, has_real_tray) in [ ('cdrom', 'ide', True),
+                                               ('cdrom', 'scsi', True),
+                                               ('disk', 'floppy', False) ]:
 
-class TestSCSICDInitiallyFilled(TestInitiallyFilled):
-    TestInitiallyFilled = TestInitiallyFilled
-    has_real_tray = True
+        for case in [ TestInitiallyFilled, TestInitiallyEmpty ]:
 
-    def setUp(self):
-        self.TestInitiallyFilled.setUp(self, 'cdrom', 'scsi')
+            attr = { 'media': media,
+                     'interface': interface,
+                     'has_real_tray': has_real_tray }
 
-class TestSCSICDInitiallyEmpty(TestInitiallyEmpty):
-    TestInitiallyEmpty = TestInitiallyEmpty
-    has_real_tray = True
+            name = '%s_%s_%s' % (case.__name__, media, interface)
+            globals()[name] = type(name, (case, ), attr)
 
-    def setUp(self):
-        self.TestInitiallyEmpty.setUp(self, 'cdrom', 'scsi')
-
-class TestFloppyInitiallyFilled(TestInitiallyFilled):
-    TestInitiallyFilled = TestInitiallyFilled
-    has_real_tray = False
-
-    def setUp(self):
-        self.TestInitiallyFilled.setUp(self, 'disk', 'floppy')
-
-class TestFloppyInitiallyEmpty(TestInitiallyEmpty):
-    TestInitiallyEmpty = TestInitiallyEmpty
-    has_real_tray = False
-
-    def setUp(self):
-        self.TestInitiallyEmpty.setUp(self, 'disk', 'floppy')
-        # FDDs not having a real tray and there not being a medium inside the
-        # tray at startup means the tray will be considered open
-        self.has_opened = True
+create_basic_test_classes()
 
 class TestChangeReadOnly(ChangeBaseClass):
     device_name = 'qdev0'
-- 
2.20.1

[Qemu-devel] [PATCH 3/3] iotests/118: Add -blockdev based tests

[Kevin Wolf]

The code path for -device drive=<node-name> or without a drive=...
option for empty drives, which is supposed to be used with -blockdev
differs enough from the -drive based path with a user-owned
BlockBackend, so we want to test both paths at least for the basic tests
implemented by TestInitiallyFilled and TestInitiallyEmpty.

This would have caught the bug recently fixed for inserting read-only
nodes into a scsi-cd created without a drive=... option.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/118     | 43 ++++++++++++++++++++++++++------------
 tests/qemu-iotests/118.out |  4 ++--
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index c281259215..6f45779ee9 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -42,10 +42,14 @@ class ChangeBaseClass(iotests.QMPTestCase):
     has_opened = False
     has_closed = False
 
+    device_name = 'qdev0'
+    use_drive = False
+
     def process_events(self):
         for event in self.vm.get_qmp_events(wait=False):
             if (event['event'] == 'DEVICE_TRAY_MOVED' and
-                event['data']['device'] == 'drive0'):
+                (event['data']['device'] == 'drive0' or
+                 event['data']['id'] == self.device_name)):
                 if event['data']['tray-open'] == False:
                     self.has_closed = True
                 else:
@@ -69,9 +73,11 @@ class ChangeBaseClass(iotests.QMPTestCase):
 
 class GeneralChangeTestsBaseClass(ChangeBaseClass):
 
-    device_name = 'qdev0'
-
     def test_change(self):
+        # 'change' requires a drive name, so skip the test for blockdev
+        if not self.use_drive:
+            return
+
         result = self.vm.qmp('change', device='drive0', target=new_img,
                                        arg=iotests.imgfmt)
         self.assert_qmp(result, 'return', {})
@@ -298,7 +304,13 @@ class TestInitiallyFilled(GeneralChangeTestsBaseClass):
         qemu_img('create', '-f', iotests.imgfmt, old_img, '1440k')
         qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
         self.vm = iotests.VM()
-        self.vm.add_drive(old_img, 'media=%s' % self.media, 'none')
+        if self.use_drive:
+            self.vm.add_drive(old_img, 'media=%s' % self.media, 'none')
+        else:
+            self.vm.add_blockdev([ 'node-name=drive0',
+                                   'driver=%s' % iotests.imgfmt,
+                                   'file.driver=file',
+                                   'file.filename=%s' % old_img ])
         if self.interface == 'scsi':
             self.vm.add_device('virtio-scsi-pci')
         self.vm.add_device('%s,drive=drive0,id=%s' %
@@ -333,11 +345,14 @@ class TestInitiallyEmpty(GeneralChangeTestsBaseClass):
 
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, new_img, '1440k')
-        self.vm = iotests.VM().add_drive(None, 'media=%s' % self.media, 'none')
+        self.vm = iotests.VM()
+        if self.use_drive:
+            self.vm.add_drive(None, 'media=%s' % self.media, 'none')
         if self.interface == 'scsi':
             self.vm.add_device('virtio-scsi-pci')
-        self.vm.add_device('%s,drive=drive0,id=%s' %
+        self.vm.add_device('%s,%sid=%s' %
                            (interface_to_device_name(self.interface),
+                            'drive=drive0,' if self.use_drive else '',
                             self.device_name))
         self.vm.launch()
 
@@ -363,13 +378,15 @@ def create_basic_test_classes():
                                                ('disk', 'floppy', False) ]:
 
         for case in [ TestInitiallyFilled, TestInitiallyEmpty ]:
-
-            attr = { 'media': media,
-                     'interface': interface,
-                     'has_real_tray': has_real_tray }
-
-            name = '%s_%s_%s' % (case.__name__, media, interface)
-            globals()[name] = type(name, (case, ), attr)
+            for use_drive in [ True, False ]:
+                attr = { 'media': media,
+                         'interface': interface,
+                         'has_real_tray': has_real_tray,
+                         'use_drive': use_drive }
+
+                name = '%s_%s_%s_%s' % (case.__name__, media, interface,
+                                        'drive' if use_drive else 'blockdev')
+                globals()[name] = type(name, (case, ), attr)
 
 create_basic_test_classes()
 
diff --git a/tests/qemu-iotests/118.out b/tests/qemu-iotests/118.out
index b4ff997a8c..bf5bfd5aca 100644
--- a/tests/qemu-iotests/118.out
+++ b/tests/qemu-iotests/118.out
@@ -1,5 +1,5 @@
-.........................................................................................
+.......................................................................................................................................................................
 ----------------------------------------------------------------------
-Ran 89 tests
+Ran 167 tests
 
 OK
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/3] iotests/118: Test media change for scsi-cd

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 362 bytes --]

On 31.07.19 22:42, Kevin Wolf wrote:
> The test covered only floppy and ide-cd. Add scsi-cd as well.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/118     | 20 ++++++++++++++++++++
>  tests/qemu-iotests/118.out |  4 ++--
>  2 files changed, 22 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/3] iotests/118: Create test classes dynamically

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 783 bytes --]

On 31.07.19 22:42, Kevin Wolf wrote:
> We're getting a ridiculous number of child classes of
> TestInitiallyFilled and TestInitiallyEmpty that differ only in a few
> attributes that we want to test in all combinations.
> 
> Instead of explicitly writing down every combination, let's use a loop
> and create those classes dynamically.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/118 | 69 +++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 48 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +            name = '%s_%s_%s' % (case.__name__, media, interface)
> +            globals()[name] = type(name, (case, ), attr)

Äh.  OK.  Well, learned about that (trailing comma) today, I guess.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 3/3] iotests/118: Add -blockdev based tests

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 796 bytes --]

On 31.07.19 22:42, Kevin Wolf wrote:
> The code path for -device drive=<node-name> or without a drive=...
> option for empty drives, which is supposed to be used with -blockdev
> differs enough from the -drive based path with a user-owned
> BlockBackend, so we want to test both paths at least for the basic tests
> implemented by TestInitiallyFilled and TestInitiallyEmpty.
> 
> This would have caught the bug recently fixed for inserting read-only
> nodes into a scsi-cd created without a drive=... option.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/118     | 43 ++++++++++++++++++++++++++------------
>  tests/qemu-iotests/118.out |  4 ++--
>  2 files changed, 32 insertions(+), 15 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]