[PATCH v2 0/2] Assorted fixes to tests that were broken by recent scsi changes

[PATCH v2 0/2] Assorted fixes to tests that were broken by recent scsi changes

[Maxim Levitsky]

While most of the patches in V1 of this series are already merged upstream,
the patch that fixes iotest 240 was broken on s390 and was not accepted.

This is	an updated version of this patch, based on Paulo's suggestion,
that hopefully makes this iotest work on both x86 and s390.

Best regards,
	Maxim Levitsky

Maxim Levitsky (2):
  iotests: add filter_qmp_virtio_scsi function
  iotests: rewrite iotest 240 in python

 tests/qemu-iotests/240        | 228 +++++++++++++++-------------------
 tests/qemu-iotests/240.out    |  76 +++++++-----
 tests/qemu-iotests/iotests.py |  10 ++
 3 files changed, 153 insertions(+), 161 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] iotests: add filter_qmp_virtio_scsi function

[Maxim Levitsky]

filter_qmp_virtio_scsi can be used to filter virtio-scsi-pci/ccw differences.
Note that this patch was only tested on x86.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 tests/qemu-iotests/iotests.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 63d2ace93c..18b7437600 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -392,6 +392,16 @@ def filter_qmp_testfiles(qmsg):
         return value
     return filter_qmp(qmsg, _filter)
 
+def filter_virtio_scsi(output: str) -> str:
+    return re.sub(r'(virtio-scsi)-(ccw|pci)', r'\1', output)
+
+def filter_qmp_virtio_scsi(qmsg):
+    def _filter(_key, value):
+        if is_str(value):
+            return filter_virtio_scsi(value)
+        return value
+    return filter_qmp(qmsg, _filter)
+
 def filter_generated_node_ids(msg):
     return re.sub("#block[0-9]+", "NODE_NAME", msg)
 
-- 
2.26.2

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

[Maxim Levitsky]

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
@@ -1,5 +1,5 @@
-#!/usr/bin/env bash
-#
+#!/usr/bin/env python3
+
 # Test hot plugging and unplugging with iothreads
 #
 # Copyright (C) 2019 Igalia, S.L.
@@ -17,133 +17,99 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
-#
 
-# creator
-owner=berto@igalia.com
-
-seq=`basename $0`
-echo "QA output created by $seq"
-
-status=1	# failure is the default!
-
-_cleanup()
-{
-    rm -f "$SOCK_DIR/nbd"
-}
-trap "_cleanup; exit \$status" 0 1 2 3 15
-
-# get standard environment, filters and checks
-. ./common.rc
-. ./common.filter
-
-_supported_fmt generic
-_supported_proto generic
-
-do_run_qemu()
-{
-    echo Testing: "$@"
-    $QEMU -nographic -qmp stdio -serial none "$@"
-    echo
-}
-
-# Remove QMP events from (pretty-printed) output. Doesn't handle
-# nested dicts correctly, but we don't get any of those in this test.
-_filter_qmp_events()
-{
-    tr '\n' '\t' | sed -e \
-	's/{\s*"timestamp":\s*{[^}]*},\s*"event":[^,}]*\(,\s*"data":\s*{[^}]*}\)\?\s*}\s*//g' \
-	| tr '\t' '\n'
-}
-
-run_qemu()
-{
-    do_run_qemu "$@" 2>&1 | _filter_qmp | _filter_qmp_events
-}
-
-case "$QEMU_DEFAULT_MACHINE" in
-  s390-ccw-virtio)
-      virtio_scsi=virtio-scsi-ccw
-      ;;
-  *)
-      virtio_scsi=virtio-scsi-pci
-      ;;
-esac
-
-echo
-echo === Unplug a SCSI disk and then plug it again ===
-echo
-
-run_qemu <<EOF
-{ "execute": "qmp_capabilities" }
-{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "read-zeroes": true, "node-name": "hd0"}}
-{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0"}}
-{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0"}}
-{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}}
-{ "execute": "device_del", "arguments": {"id": "scsi0"}}
-{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
-{ "execute": "quit"}
-EOF
-
-echo
-echo === Attach two SCSI disks using the same block device and the same iothread ===
-echo
-
-run_qemu <<EOF
-{ "execute": "qmp_capabilities" }
-{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "read-zeroes": true, "node-name": "hd0", "read-only": true}}
-{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0"}}
-{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}}
-{ "execute": "device_del", "arguments": {"id": "scsi-hd1"}}
-{ "execute": "device_del", "arguments": {"id": "scsi0"}}
-{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
-{ "execute": "quit"}
-EOF
-
-echo
-echo === Attach two SCSI disks using the same block device but different iothreads ===
-echo
-
-run_qemu <<EOF
-{ "execute": "qmp_capabilities" }
-{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "read-zeroes": true, "node-name": "hd0", "read-only": true}}
-{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
-{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread1"}}
-{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi1", "driver": "${virtio_scsi}", "iothread": "iothread1"}}
-{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi0.0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}}
-{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}}
-{ "execute": "device_del", "arguments": {"id": "scsi-hd1"}}
-{ "execute": "device_del", "arguments": {"id": "scsi0"}}
-{ "execute": "device_del", "arguments": {"id": "scsi1"}}
-{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
-{ "execute": "quit"}
-EOF
-
-echo
-echo === Attach a SCSI disks using the same block device as a NBD server ===
-echo
-
-run_qemu <<EOF
-{ "execute": "qmp_capabilities" }
-{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "read-zeroes": true, "node-name": "hd0", "read-only": true}}
-{ "execute": "nbd-server-start", "arguments": {"addr":{"type":"unix","data":{"path":"$SOCK_DIR/nbd"}}}}
-{ "execute": "nbd-server-add", "arguments": {"device":"hd0"}}
-{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}}
-{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi0.0"}}
-{ "execute": "quit"}
-EOF
-
-# success, all done
-echo "*** done"
-rm -f $seq.full
-status=0
+import iotests
+import os
+
+nbd_sock = iotests.file_path('nbd.sock', base_dir=iotests.sock_dir)
+
+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()
+
+    def tearDown(self):
+        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])
+        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')
+        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')
+        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')
+    iotests.activate_logging()
+    iotests.main()
diff --git a/tests/qemu-iotests/240.out b/tests/qemu-iotests/240.out
index d00df50297..064c8eaa41 100644
--- a/tests/qemu-iotests/240.out
+++ b/tests/qemu-iotests/240.out
@@ -1,67 +1,83 @@
-QA output created by 240
-
-=== Unplug a SCSI disk and then plug it again ===
-
-Testing:
-QMP_VERSION
-{"return": {}}
+==Unplug a SCSI disk and then plug it again==
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-zeroes": true}}
 {"return": {}}
+{"execute": "object-add", "arguments": {"id": "iothread0", "qom-type": "iothread"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"driver": "virtio-scsi", "id": "scsi0", "iothread": "iothread0"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"drive": "hd0", "driver": "scsi-hd", "id": "scsi-hd0"}}
 {"return": {}}
+{"execute": "device_del", "arguments": {"id": "scsi-hd0"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"drive": "hd0", "driver": "scsi-hd", "id": "scsi-hd0"}}
 {"return": {}}
+{"execute": "device_del", "arguments": {"id": "scsi-hd0"}}
 {"return": {}}
+{"execute": "device_del", "arguments": {"id": "scsi0"}}
 {"return": {}}
+{"execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
 {"return": {}}
+==Attach two SCSI disks using the same block device and the same iothread==
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true, "read-zeroes": true}}
 {"return": {}}
-
-=== Attach two SCSI disks using the same block device and the same iothread ===
-
-Testing:
-QMP_VERSION
+{"execute": "object-add", "arguments": {"id": "iothread0", "qom-type": "iothread"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"driver": "virtio-scsi", "id": "scsi0", "iothread": "iothread0"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"drive": "hd0", "driver": "scsi-hd", "id": "scsi-hd0"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"drive": "hd0", "driver": "scsi-hd", "id": "scsi-hd1"}}
 {"return": {}}
+{"execute": "device_del", "arguments": {"id": "scsi-hd1"}}
 {"return": {}}
+{"execute": "device_del", "arguments": {"id": "scsi-hd0"}}
 {"return": {}}
+{"execute": "device_del", "arguments": {"id": "scsi0"}}
 {"return": {}}
+{"execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
 {"return": {}}
+==Attach two SCSI disks using the same block device but different iothreads==
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true, "read-zeroes": true}}
 {"return": {}}
+{"execute": "object-add", "arguments": {"id": "iothread0", "qom-type": "iothread"}}
 {"return": {}}
+{"execute": "object-add", "arguments": {"id": "iothread1", "qom-type": "iothread"}}
 {"return": {}}
-
-=== Attach two SCSI disks using the same block device but different iothreads ===
-
-Testing:
-QMP_VERSION
-{"return": {}}
-{"return": {}}
-{"return": {}}
-{"return": {}}
+{"execute": "device_add", "arguments": {"driver": "virtio-scsi", "id": "scsi0", "iothread": "iothread0"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"driver": "virtio-scsi", "id": "scsi1", "iothread": "iothread1"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"bus": "scsi0.0", "drive": "hd0", "driver": "scsi-hd", "id": "scsi-hd0"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"bus": "scsi1.0", "drive": "hd0", "driver": "scsi-hd", "id": "scsi-hd1"}}
 {"error": {"class": "GenericError", "desc": "Cannot change iothread of active block backend"}}
+{"execute": "device_del", "arguments": {"id": "scsi-hd0"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"bus": "scsi1.0", "drive": "hd0", "driver": "scsi-hd", "id": "scsi-hd1"}}
 {"return": {}}
+{"execute": "device_del", "arguments": {"id": "scsi-hd1"}}
 {"return": {}}
+{"execute": "device_del", "arguments": {"id": "scsi1"}}
 {"return": {}}
+{"execute": "device_del", "arguments": {"id": "scsi0"}}
 {"return": {}}
+{"execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
 {"return": {}}
+==Attach a SCSI disks using the same block device as a NBD server==
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true, "read-zeroes": true}}
 {"return": {}}
-
-=== Attach a SCSI disks using the same block device as a NBD server ===
-
-Testing:
-QMP_VERSION
-{"return": {}}
-{"return": {}}
+{"execute": "nbd-server-start", "arguments": {"addr": {"data": {"path": "SOCK_DIR/PID-nbd.sock"}, "type": "unix"}}}
 {"return": {}}
+{"execute": "nbd-server-add", "arguments": {"device": "hd0"}}
 {"return": {}}
+{"execute": "object-add", "arguments": {"id": "iothread0", "qom-type": "iothread"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"driver": "virtio-scsi", "id": "scsi0", "iothread": "iothread0"}}
 {"return": {}}
+{"execute": "device_add", "arguments": {"drive": "hd0", "driver": "scsi-hd", "id": "scsi-hd0"}}
 {"return": {}}
-{"return": {}}
-*** done
+....
+----------------------------------------------------------------------
+Ran 4 tests
+
+OK
-- 
2.26.2

Re: [PATCH v2 0/2] Assorted fixes to tests that were broken by recent scsi changes

[Paolo Bonzini]

On 01/11/20 17:15, Maxim Levitsky wrote:
> While most of the patches in V1 of this series are already merged upstream,
> the patch that fixes iotest 240 was broken on s390 and was not accepted.
> 
> This is	an updated version of this patch, based on Paulo's suggestion,
> that hopefully makes this iotest work on both x86 and s390.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (2):
>   iotests: add filter_qmp_virtio_scsi function
>   iotests: rewrite iotest 240 in python
> 
>  tests/qemu-iotests/240        | 228 +++++++++++++++-------------------
>  tests/qemu-iotests/240.out    |  76 +++++++-----
>  tests/qemu-iotests/iotests.py |  10 ++
>  3 files changed, 153 insertions(+), 161 deletions(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH v2 0/2] Assorted fixes to tests that were broken by recent scsi changes

[Christian Borntraeger]

On 01.11.20 17:15, Maxim Levitsky wrote:
> While most of the patches in V1 of this series are already merged upstream,
> the patch that fixes iotest 240 was broken on s390 and was not accepted.
> 
> This is	an updated version of this patch, based on Paulo's suggestion,
> that hopefully makes this iotest work on both x86 and s390.
> 
> Best regards,
> 	Maxim Levitsky
> 
> Maxim Levitsky (2):
>   iotests: add filter_qmp_virtio_scsi function
>   iotests: rewrite iotest 240 in python

Both patches on s390x
Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>


Thanks.

> 
>  tests/qemu-iotests/240        | 228 +++++++++++++++-------------------
>  tests/qemu-iotests/240.out    |  76 +++++++-----
>  tests/qemu-iotests/iotests.py |  10 ++
>  3 files changed, 153 insertions(+), 161 deletions(-)
> 

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

[Max Reitz]

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()

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

[Maxim Levitsky]

On Tue, 2020-11-03 at 13:53 +0100, Max Reitz wrote:
> 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)...
I agree with that, the test can be improved a lot. I on purpose didn't do
this since I wanted 1:1 translation of the bash test.
After that a refactoring, or a rewrite of this test can be very welcome.

> 
> > +
> > +    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.
It is a late change, which I copy&pasta'ed so no wonder it is a bit different.
Fixed!

That code too I would like later to move to a function or so.

> 
> > +        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?

Actually this doesn't work since the whole thing is started with 'qtest' accel,
and there is no real guest there. 

The pcie device removal in 'production' happens only when you place the device on a 
pcie root port or on pci hotplug enabled bridge,
which then signals the removal of the device to the guest which needs to ACK this,
and only then the qemu actually removes the device and sends the DEVICE_DELETED event.

So this device_del is pretty much pointless I think, and it never will be really removed.
Again I tried to just translate the test without doing any more changes, but I will remove this.

> 
> > +        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.)
I just didn't notice it since it didn't seem to be important.
> 
> > +        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.
That is a result of a copy&pasta from some other iotest.
Fixed, thanks!


Thanks for the review!

Best regards,
	Maxim Levitsky

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