[Qemu-devel] [PATCH v3 0/5] blockdev-backup: don't check aio_context too early

[Qemu-devel] [PATCH v3 0/5] blockdev-backup: don't check aio_context too early

[John Snow]

See patch one's commit message for justification.
Patches 2-5 are for testing, because that's always how these things go.

001/5:[----] [--] 'blockdev-backup: don't check aio_context too early'
002/5:[0004] [FC] 'iotests.py: do not use infinite waits'
003/5:[down]      'QEMUMachine: add events_wait method'
004/5:[0022] [FC] 'iotests.py: rewrite run_job to be pickier'
005/5:[0017] [FC] 'iotests: add iotest 250 for testing blockdev-backup
                   across iothread contexts'

v3: Rebased on Max's staging branch:
    Rebase patch 2
    added patch 3, to add events_wait.
    Rework patch 4 to make run_job consume legacy events, too
    Minorly edit patch 5 due to the two above.
v2: added patch 4, with iotest framework adjustments in patches 2/3.

John Snow (5):
  blockdev-backup: don't check aio_context too early
  iotests.py: do not use infinite waits
  QEMUMachine: add events_wait method
  iotests.py: rewrite run_job to be pickier
  iotests: add iotest 250 for testing blockdev-backup across iothread
    contexts

 blockdev.c                    |   4 --
 python/qemu/__init__.py       |  69 +++++++++++++------
 tests/qemu-iotests/250        | 122 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/250.out    | 119 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  60 ++++++++++-------
 6 files changed, 326 insertions(+), 49 deletions(-)
 create mode 100755 tests/qemu-iotests/250
 create mode 100644 tests/qemu-iotests/250.out

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/5] blockdev-backup: don't check aio_context too early

[John Snow]

in blockdev_backup_prepare, we check to make sure that the target is
associated with a compatible aio context. However, do_blockdev_backup is
called later and has some logic to move the target to a compatible
aio_context. The transaction version will fail certain commands
needlessly early as a result.

Allow blockdev_backup_prepare to simply call do_blockdev_backup, which
will ultimately decide if the contexts are compatible or not.

Note: the transaction version has always disallowed this operation since
its initial commit bd8baecd (2014), whereas the version of
qmp_blockdev_backup at the time, from commit c29c1dd312f, tried to
enforce the aio_context switch instead. It's not clear, and I can't see
from the mailing list archives at the time, why the two functions take a
different approach. It wasn't until later in efd7556708b (2016) that the
standalone version tried to determine if it could set the context or
not.

Reported-by: aihua liang <aliang@redhat.com>
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1683498
Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e856ca4be9..01a48a2a5a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1872,10 +1872,6 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
     }
 
     aio_context = bdrv_get_aio_context(bs);
-    if (aio_context != bdrv_get_aio_context(target)) {
-        error_setg(errp, "Backup between two IO threads is not implemented");
-        return;
-    }
     aio_context_acquire(aio_context);
     state->bs = bs;
 
-- 
2.20.1

[Qemu-devel] [PATCH v3 2/5] iotests.py: do not use infinite waits

[John Snow]

Cap waits to 60 seconds so that iotests can fail gracefully if something
goes wrong.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6bcddd8870..6e17c040dc 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -521,7 +521,7 @@ class VM(qtest.QEMUQtestMachine):
             output_list += [key + '=' + obj[key]]
         return ','.join(output_list)
 
-    def get_qmp_events_filtered(self, wait=True):
+    def get_qmp_events_filtered(self, wait=60.0):
         result = []
         for ev in self.get_qmp_events(wait=wait):
             result.append(filter_qmp_event(ev))
@@ -539,10 +539,10 @@ class VM(qtest.QEMUQtestMachine):
 
     # Returns None on success, and an error string on failure
     def run_job(self, job, auto_finalize=True, auto_dismiss=False,
-                pre_finalize=None):
+                pre_finalize=None, wait=60.0):
         error = None
         while True:
-            for ev in self.get_qmp_events_filtered(wait=True):
+            for ev in self.get_qmp_events_filtered(wait=wait):
                 if ev['event'] == 'JOB_STATUS_CHANGE':
                     status = ev['data']['status']
                     if status == 'aborting':
@@ -647,7 +647,7 @@ class QMPTestCase(unittest.TestCase):
         self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
                          self.vm.flatten_qmp_object(reference))
 
-    def cancel_and_wait(self, drive='drive0', force=False, resume=False):
+    def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0):
         '''Cancel a block job and wait for it to finish, returning the event'''
         result = self.vm.qmp('block-job-cancel', device=drive, force=force)
         self.assert_qmp(result, 'return', {})
@@ -658,7 +658,7 @@ class QMPTestCase(unittest.TestCase):
         cancelled = False
         result = None
         while not cancelled:
-            for event in self.vm.get_qmp_events(wait=True):
+            for event in self.vm.get_qmp_events(wait=wait):
                 if event['event'] == 'BLOCK_JOB_COMPLETED' or \
                    event['event'] == 'BLOCK_JOB_CANCELLED':
                     self.assert_qmp(event, 'data/device', drive)
@@ -671,10 +671,10 @@ class QMPTestCase(unittest.TestCase):
         self.assert_no_active_block_jobs()
         return result
 
-    def wait_until_completed(self, drive='drive0', check_offset=True):
+    def wait_until_completed(self, drive='drive0', check_offset=True, wait=60.0):
         '''Wait for a block job to finish, returning the event'''
         while True:
-            for event in self.vm.get_qmp_events(wait=True):
+            for event in self.vm.get_qmp_events(wait=wait):
                 if event['event'] == 'BLOCK_JOB_COMPLETED':
                     self.assert_qmp(event, 'data/device', drive)
                     self.assert_qmp_absent(event, 'data/error')
-- 
2.20.1

[Qemu-devel] [PATCH v3 3/5] QEMUMachine: add events_wait method

[John Snow]

Instead of event_wait which looks for a single event, add an events_wait
which can look for any number of events simultaneously. However, it
will still only return one at a time, whichever happens first.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
index 81d9657ec0..98ed8a2e28 100644
--- a/python/qemu/__init__.py
+++ b/python/qemu/__init__.py
@@ -402,42 +402,71 @@ class QEMUMachine(object):
         self._qmp.clear_events()
         return events
 
-    def event_wait(self, name, timeout=60.0, match=None):
+    @staticmethod
+    def event_match(event, match=None):
         """
-        Wait for specified timeout on named event in QMP; optionally filter
-        results by match.
+        Check if an event matches optional match criteria.
 
-        The 'match' is checked to be a recursive subset of the 'event'; skips
-        branch processing on match's value None
-           {"foo": {"bar": 1}} matches {"foo": None}
-           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
+        The match criteria takes the form of a matching subdict. The event is
+        checked to be a superset of the subdict, recursively, with matching
+        values whenever those values are not None.
+
+        Examples, with the subdict queries on the left:
+         - None matches any object.
+         - {"foo": None} matches {"foo": {"bar": 1}}
+         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
+         - {"foo": {"baz": 2}} matches {"foo": {"bar": 1, "baz": 2}}
         """
-        def event_match(event, match=None):
-            if match is None:
-                return True
+        if match is None:
+            return True
 
-            for key in match:
-                if key in event:
-                    if isinstance(event[key], dict):
-                        if not event_match(event[key], match[key]):
-                            return False
-                    elif event[key] != match[key]:
+        for key in match:
+            if key in event:
+                if isinstance(event[key], dict):
+                    if not QEMUMachine.event_match(event[key], match[key]):
                         return False
-                else:
+                elif event[key] != match[key]:
                     return False
+            else:
+                return False
+        return True
 
-            return True
+    def event_wait(self, name, timeout=60.0, match=None):
+        """
+        event_wait waits for and returns a named event from QMP with a timeout.
+
+        name: The event to wait for.
+        timeout: QEMUMonitorProtocol.pull_event timeout parameter.
+        match: Optional match criteria. See event_match for details.
+        """
+        return self.events_wait([(name, match)], timeout)
+
+    def events_wait(self, events, timeout=60.0):
+        """
+        events_wait waits for and returns a named event from QMP with a timeout.
+
+        events: a sequence of (name, match_criteria) tuples.
+                The match criteria are optional and may be None.
+                See event_match for details.
+        timeout: QEMUMonitorProtocol.pull_event timeout parameter.
+        """
+        def _match(event):
+            for name, match in events:
+                if (event['event'] == name and
+                    self.event_match(event, match)):
+                    return True
+            return False
 
         # Search cached events
         for event in self._events:
-            if (event['event'] == name) and event_match(event, match):
+            if _match(event):
                 self._events.remove(event)
                 return event
 
         # Poll for new events
         while True:
             event = self._qmp.pull_event(wait=timeout)
-            if (event['event'] == name) and event_match(event, match):
+            if _match(event):
                 return event
             self._events.append(event)
 
-- 
2.20.1

[Qemu-devel] [PATCH v3 4/5] iotests.py: rewrite run_job to be pickier

[John Snow]

Don't pull events out of the queue that don't belong to us;
be choosier so that we can use this method to drive jobs that
were launched by transactions that may have more jobs.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 48 +++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 6e17c040dc..dc77d3fba0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -540,27 +540,37 @@ class VM(qtest.QEMUQtestMachine):
     # Returns None on success, and an error string on failure
     def run_job(self, job, auto_finalize=True, auto_dismiss=False,
                 pre_finalize=None, wait=60.0):
+        match_device = {'data': {'device': job}}
+        match_id = {'data': {'id': job}}
+        events = [
+            ('BLOCK_JOB_COMPLETED', match_device),
+            ('BLOCK_JOB_CANCELLED', match_device),
+            ('BLOCK_JOB_ERROR', match_device),
+            ('BLOCK_JOB_READY', match_device),
+            ('BLOCK_JOB_PENDING', match_id),
+            ('JOB_STATUS_CHANGE', match_id)
+        ]
         error = None
         while True:
-            for ev in self.get_qmp_events_filtered(wait=wait):
-                if ev['event'] == 'JOB_STATUS_CHANGE':
-                    status = ev['data']['status']
-                    if status == 'aborting':
-                        result = self.qmp('query-jobs')
-                        for j in result['return']:
-                            if j['id'] == job:
-                                error = j['error']
-                                log('Job failed: %s' % (j['error']))
-                    elif status == 'pending' and not auto_finalize:
-                        if pre_finalize:
-                            pre_finalize()
-                        self.qmp_log('job-finalize', id=job)
-                    elif status == 'concluded' and not auto_dismiss:
-                        self.qmp_log('job-dismiss', id=job)
-                    elif status == 'null':
-                        return error
-                else:
-                    log(ev)
+            ev = filter_qmp_event(self.events_wait(events))
+            if ev['event'] != 'JOB_STATUS_CHANGE':
+                log(ev)
+                continue
+            status = ev['data']['status']
+            if status == 'aborting':
+                result = self.qmp('query-jobs')
+                for j in result['return']:
+                    if j['id'] == job:
+                        error = j['error']
+                        log('Job failed: %s' % (j['error']))
+            elif status == 'pending' and not auto_finalize:
+                if pre_finalize:
+                    pre_finalize()
+                self.qmp_log('job-finalize', id=job)
+            elif status == 'concluded' and not auto_dismiss:
+                self.qmp_log('job-dismiss', id=job)
+            elif status == 'null':
+                return error
 
     def node_info(self, node_name):
         nodes = self.qmp('query-named-block-nodes')
-- 
2.20.1

[Qemu-devel] [PATCH v3 5/5] iotests: add iotest 250 for testing blockdev-backup across iothread contexts

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/250     | 122 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/250.out | 119 ++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 242 insertions(+)
 create mode 100755 tests/qemu-iotests/250
 create mode 100644 tests/qemu-iotests/250.out

diff --git a/tests/qemu-iotests/250 b/tests/qemu-iotests/250
new file mode 100755
index 0000000000..c594a43205
--- /dev/null
+++ b/tests/qemu-iotests/250
@@ -0,0 +1,122 @@
+#!/usr/bin/env python
+#
+# Test incremental/backup across iothread contexts
+#
+# Copyright (c) 2019 John Snow for Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# owner=jsnow@redhat.com
+
+import os
+import iotests
+from iotests import log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+size = 64 * 1024 * 1024
+
+with iotests.FilePath('img0') as img0_path, \
+     iotests.FilePath('img1') as img1_path, \
+     iotests.FilePath('img0-full') as img0_full_path, \
+     iotests.FilePath('img1-full') as img1_full_path, \
+     iotests.FilePath('img0-incr') as img0_incr_path, \
+     iotests.FilePath('img1-incr') as img1_incr_path, \
+     iotests.VM() as vm:
+
+    def create_target(filepath, name, size):
+        basename = os.path.basename(filepath)
+        nodename = "file_{}".format(basename)
+        log(vm.command('blockdev-create', job_id='job1',
+                       options={
+                           'driver': 'file',
+                           'filename': filepath,
+                           'size': 0,
+                       }))
+        vm.run_job('job1')
+        log(vm.command('blockdev-add', driver='file',
+                       node_name=nodename, filename=filepath))
+        log(vm.command('blockdev-create', job_id='job2',
+                       options={
+                           'driver': iotests.imgfmt,
+                           'file': nodename,
+                           'size': size,
+                       }))
+        vm.run_job('job2')
+        log(vm.command('blockdev-add', driver=iotests.imgfmt,
+                       node_name=name,
+                       file=nodename))
+
+    log('--- Preparing images & VM ---\n')
+    vm.add_object('iothread,id=iothread0')
+    vm.add_object('iothread,id=iothread1')
+    vm.add_device('virtio-scsi-pci,id=scsi0,iothread=iothread0')
+    vm.add_device('virtio-scsi-pci,id=scsi1,iothread=iothread1')
+    iotests.qemu_img_create('-f', iotests.imgfmt, img0_path, str(size))
+    iotests.qemu_img_create('-f', iotests.imgfmt, img1_path, str(size))
+    vm.add_drive(img0_path, interface='none')
+    vm.add_device('scsi-hd,id=device0,drive=drive0,bus=scsi0.0')
+    vm.add_drive(img1_path, interface='none')
+    vm.add_device('scsi-hd,id=device1,drive=drive1,bus=scsi1.0')
+
+    log('--- Starting VM ---\n')
+    vm.launch()
+
+    log('--- Create Targets & Full Backups ---\n')
+    create_target(img0_full_path, 'img0-full', size)
+    create_target(img1_full_path, 'img1-full', size)
+    ret = vm.qmp_log('transaction', indent=2, actions=[
+        { 'type': 'block-dirty-bitmap-add',
+          'data': { 'node': 'drive0', 'name': 'bitmap0' }},
+        { 'type': 'block-dirty-bitmap-add',
+          'data': { 'node': 'drive1', 'name': 'bitmap1' }},
+        { 'type': 'blockdev-backup',
+          'data': { 'device': 'drive0',
+                    'target': 'img0-full',
+                    'sync': 'full',
+                    'job-id': 'j0' }},
+        { 'type': 'blockdev-backup',
+          'data': { 'device': 'drive1',
+                    'target': 'img1-full',
+                    'sync': 'full',
+                    'job-id': 'j1' }}
+    ])
+    if "error" in ret:
+        raise Exception(ret['error']['desc'])
+    vm.run_job('j0', auto_dismiss=True)
+    vm.run_job('j1', auto_dismiss=True)
+
+    log('\n--- Create Targets & Incremental Backups ---\n')
+    create_target(img0_incr_path, 'img0-incr', size)
+    create_target(img1_incr_path, 'img1-incr', size)
+    ret = vm.qmp_log('transaction', indent=2, actions=[
+        { 'type': 'blockdev-backup',
+          'data': { 'device': 'drive0',
+                    'target': 'img0-incr',
+                    'sync': 'incremental',
+                    'bitmap': 'bitmap0',
+                    'job-id': 'j2' }},
+        { 'type': 'blockdev-backup',
+          'data': { 'device': 'drive1',
+                    'target': 'img1-incr',
+                    'sync': 'incremental',
+                    'bitmap': 'bitmap1',
+                    'job-id': 'j3' }}
+    ])
+    if "error" in ret:
+        raise Exception(ret['error']['desc'])
+    vm.run_job('j2', auto_dismiss=True)
+    vm.run_job('j3', auto_dismiss=True)
+
+    log('\n--- Done ---')
+    vm.shutdown()
diff --git a/tests/qemu-iotests/250.out b/tests/qemu-iotests/250.out
new file mode 100644
index 0000000000..eec38614ec
--- /dev/null
+++ b/tests/qemu-iotests/250.out
@@ -0,0 +1,119 @@
+--- Preparing images & VM ---
+
+--- Starting VM ---
+
+--- Create Targets & Full Backups ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "job1"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "job2"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "job1"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "job2"}}
+{"return": {}}
+{}
+{
+  "execute": "transaction",
+  "arguments": {
+    "actions": [
+      {
+        "data": {
+          "name": "bitmap0",
+          "node": "drive0"
+        },
+        "type": "block-dirty-bitmap-add"
+      },
+      {
+        "data": {
+          "name": "bitmap1",
+          "node": "drive1"
+        },
+        "type": "block-dirty-bitmap-add"
+      },
+      {
+        "data": {
+          "device": "drive0",
+          "job-id": "j0",
+          "sync": "full",
+          "target": "img0-full"
+        },
+        "type": "blockdev-backup"
+      },
+      {
+        "data": {
+          "device": "drive1",
+          "job-id": "j1",
+          "sync": "full",
+          "target": "img1-full"
+        },
+        "type": "blockdev-backup"
+      }
+    ]
+  }
+}
+{
+  "return": {}
+}
+{"data": {"device": "j0", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "j1", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Create Targets & Incremental Backups ---
+
+{}
+{"execute": "job-dismiss", "arguments": {"id": "job1"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "job2"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "job1"}}
+{"return": {}}
+{}
+{}
+{"execute": "job-dismiss", "arguments": {"id": "job2"}}
+{"return": {}}
+{}
+{
+  "execute": "transaction",
+  "arguments": {
+    "actions": [
+      {
+        "data": {
+          "bitmap": "bitmap0",
+          "device": "drive0",
+          "job-id": "j2",
+          "sync": "incremental",
+          "target": "img0-incr"
+        },
+        "type": "blockdev-backup"
+      },
+      {
+        "data": {
+          "bitmap": "bitmap1",
+          "device": "drive1",
+          "job-id": "j3",
+          "sync": "incremental",
+          "target": "img1-incr"
+        },
+        "type": "blockdev-backup"
+      }
+    ]
+  }
+}
+{
+  "return": {}
+}
+{"data": {"device": "j2", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "j3", "len": 67108864, "offset": 67108864, "speed": 0, "type": "backup"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+--- Done ---
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 2758f48143..ea52e8660d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -249,6 +249,7 @@
 247 rw auto quick
 248 rw auto quick
 249 rw auto quick
+250 rw auto quick
 252 rw auto backing quick
 253 rw auto quick
 255 rw auto quick
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 3/5] QEMUMachine: add events_wait method

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

On 23.05.19 19:06, John Snow wrote:
> Instead of event_wait which looks for a single event, add an events_wait
> which can look for any number of events simultaneously. However, it
> will still only return one at a time, whichever happens first.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
> index 81d9657ec0..98ed8a2e28 100644
> --- a/python/qemu/__init__.py
> +++ b/python/qemu/__init__.py
> @@ -402,42 +402,71 @@ class QEMUMachine(object):
>          self._qmp.clear_events()
>          return events
>  
> -    def event_wait(self, name, timeout=60.0, match=None):
> +    @staticmethod
> +    def event_match(event, match=None):
>          """
> -        Wait for specified timeout on named event in QMP; optionally filter
> -        results by match.
> +        Check if an event matches optional match criteria.
>  
> -        The 'match' is checked to be a recursive subset of the 'event'; skips
> -        branch processing on match's value None
> -           {"foo": {"bar": 1}} matches {"foo": None}
> -           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
> +        The match criteria takes the form of a matching subdict. The event is
> +        checked to be a superset of the subdict, recursively, with matching
> +        values whenever those values are not None.
> +
> +        Examples, with the subdict queries on the left:
> +         - None matches any object.
> +         - {"foo": None} matches {"foo": {"bar": 1}}
> +         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}

Pre-existing, but the difference between “bar” and “baz” confused me
quite a bit.

Also, I wonder...  {"foo": None} would not match {"foo": 1}, right?
Does that make sense?  Shouldn’t None be the wildcard here in general?
(Also pre-existing of course.)

But this patch doesn’t make things worse, so:

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

(I’d still like your opinion.)


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

Re: [Qemu-devel] [PATCH v3 0/5] blockdev-backup: don't check aio_context too early

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1797 bytes --]

On 23.05.19 19:06, John Snow wrote:
> See patch one's commit message for justification.
> Patches 2-5 are for testing, because that's always how these things go.
> 
> 001/5:[----] [--] 'blockdev-backup: don't check aio_context too early'
> 002/5:[0004] [FC] 'iotests.py: do not use infinite waits'
> 003/5:[down]      'QEMUMachine: add events_wait method'
> 004/5:[0022] [FC] 'iotests.py: rewrite run_job to be pickier'
> 005/5:[0017] [FC] 'iotests: add iotest 250 for testing blockdev-backup
>                    across iothread contexts'
> 
> v3: Rebased on Max's staging branch:
>     Rebase patch 2
>     added patch 3, to add events_wait.
>     Rework patch 4 to make run_job consume legacy events, too
>     Minorly edit patch 5 due to the two above.
> v2: added patch 4, with iotest framework adjustments in patches 2/3.
> 
> John Snow (5):
>   blockdev-backup: don't check aio_context too early
>   iotests.py: do not use infinite waits
>   QEMUMachine: add events_wait method
>   iotests.py: rewrite run_job to be pickier
>   iotests: add iotest 250 for testing blockdev-backup across iothread
>     contexts
> 
>  blockdev.c                    |   4 --
>  python/qemu/__init__.py       |  69 +++++++++++++------
>  tests/qemu-iotests/250        | 122 ++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/250.out    | 119 +++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group      |   1 +
>  tests/qemu-iotests/iotests.py |  60 ++++++++++-------
>  6 files changed, 326 insertions(+), 49 deletions(-)
>  create mode 100755 tests/qemu-iotests/250
>  create mode 100644 tests/qemu-iotests/250.out

Looks good to me (if it helps:

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

), just a question on patch 3 on pre-existing quirks.

Max


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

Re: [Qemu-devel] [PATCH v3 3/5] QEMUMachine: add events_wait method

[John Snow]

On 5/23/19 1:49 PM, Max Reitz wrote:
> On 23.05.19 19:06, John Snow wrote:
>> Instead of event_wait which looks for a single event, add an events_wait
>> which can look for any number of events simultaneously. However, it
>> will still only return one at a time, whichever happens first.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>
>> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
>> index 81d9657ec0..98ed8a2e28 100644
>> --- a/python/qemu/__init__.py
>> +++ b/python/qemu/__init__.py
>> @@ -402,42 +402,71 @@ class QEMUMachine(object):
>>          self._qmp.clear_events()
>>          return events
>>  
>> -    def event_wait(self, name, timeout=60.0, match=None):
>> +    @staticmethod
>> +    def event_match(event, match=None):
>>          """
>> -        Wait for specified timeout on named event in QMP; optionally filter
>> -        results by match.
>> +        Check if an event matches optional match criteria.
>>  
>> -        The 'match' is checked to be a recursive subset of the 'event'; skips
>> -        branch processing on match's value None
>> -           {"foo": {"bar": 1}} matches {"foo": None}
>> -           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
>> +        The match criteria takes the form of a matching subdict. The event is
>> +        checked to be a superset of the subdict, recursively, with matching
>> +        values whenever those values are not None.
>> +
>> +        Examples, with the subdict queries on the left:
>> +         - None matches any object.
>> +         - {"foo": None} matches {"foo": {"bar": 1}}
>> +         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
> 
> Pre-existing, but the difference between “bar” and “baz” confused me
> quite a bit.
> 
> Also, I wonder...  {"foo": None} would not match {"foo": 1}, right?
> Does that make sense?  Shouldn’t None be the wildcard here in general?
> (Also pre-existing of course.)
> 
> But this patch doesn’t make things worse, so:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> (I’d still like your opinion.)
> 

I knew I was inviting trouble by trying to re-document this.

The intention I had when writing the docs, which I think are wrong now,
was for {"foo": None} to match {"foo": 1}, but I think you're right that
it won't because '1' isn't a dict, so it tests for equality instead.

So I need to fix this one up a little bit, but I'll take the review as a
sign that this approach seems workable.

--js

Re: [Qemu-devel] [PATCH v3 3/5] QEMUMachine: add events_wait method

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3169 bytes --]

On 23.05.19 20:03, John Snow wrote:
> 
> 
> On 5/23/19 1:49 PM, Max Reitz wrote:
>> On 23.05.19 19:06, John Snow wrote:
>>> Instead of event_wait which looks for a single event, add an events_wait
>>> which can look for any number of events simultaneously. However, it
>>> will still only return one at a time, whichever happens first.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
>>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
>>> index 81d9657ec0..98ed8a2e28 100644
>>> --- a/python/qemu/__init__.py
>>> +++ b/python/qemu/__init__.py
>>> @@ -402,42 +402,71 @@ class QEMUMachine(object):
>>>          self._qmp.clear_events()
>>>          return events
>>>  
>>> -    def event_wait(self, name, timeout=60.0, match=None):
>>> +    @staticmethod
>>> +    def event_match(event, match=None):
>>>          """
>>> -        Wait for specified timeout on named event in QMP; optionally filter
>>> -        results by match.
>>> +        Check if an event matches optional match criteria.
>>>  
>>> -        The 'match' is checked to be a recursive subset of the 'event'; skips
>>> -        branch processing on match's value None
>>> -           {"foo": {"bar": 1}} matches {"foo": None}
>>> -           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
>>> +        The match criteria takes the form of a matching subdict. The event is
>>> +        checked to be a superset of the subdict, recursively, with matching
>>> +        values whenever those values are not None.
>>> +
>>> +        Examples, with the subdict queries on the left:
>>> +         - None matches any object.
>>> +         - {"foo": None} matches {"foo": {"bar": 1}}
>>> +         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
>>
>> Pre-existing, but the difference between “bar” and “baz” confused me
>> quite a bit.
>>
>> Also, I wonder...  {"foo": None} would not match {"foo": 1}, right?
>> Does that make sense?  Shouldn’t None be the wildcard here in general?
>> (Also pre-existing of course.)
>>
>> But this patch doesn’t make things worse, so:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> (I’d still like your opinion.)
>>
> 
> I knew I was inviting trouble by trying to re-document this.
> 
> The intention I had when writing the docs, which I think are wrong now,
> was for {"foo": None} to match {"foo": 1}, but I think you're right that
> it won't because '1' isn't a dict, so it tests for equality instead.
> 
> So I need to fix this one up a little bit, but I'll take the review as a
> sign that this approach seems workable.

I think the comment is technically completely correct.  It’s just that
(1) it’s hard to discern “bar” from “baz”, and (2) if {"foo": None}
intentionally does not match {"foo": 1}, we may want to document that,
because it isn’t intuitively clear from the description.  If it’s a bug,
the code should be fixed (and it should still be documented).

Max


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

Re: [Qemu-devel] [PATCH v3 3/5] QEMUMachine: add events_wait method

[John Snow]

On 5/24/19 8:38 AM, Max Reitz wrote:
> On 23.05.19 20:03, John Snow wrote:
>>
>>
>> On 5/23/19 1:49 PM, Max Reitz wrote:
>>> On 23.05.19 19:06, John Snow wrote:
>>>> Instead of event_wait which looks for a single event, add an events_wait
>>>> which can look for any number of events simultaneously. However, it
>>>> will still only return one at a time, whichever happens first.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  python/qemu/__init__.py | 69 +++++++++++++++++++++++++++++------------
>>>>  1 file changed, 49 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/python/qemu/__init__.py b/python/qemu/__init__.py
>>>> index 81d9657ec0..98ed8a2e28 100644
>>>> --- a/python/qemu/__init__.py
>>>> +++ b/python/qemu/__init__.py
>>>> @@ -402,42 +402,71 @@ class QEMUMachine(object):
>>>>          self._qmp.clear_events()
>>>>          return events
>>>>  
>>>> -    def event_wait(self, name, timeout=60.0, match=None):
>>>> +    @staticmethod
>>>> +    def event_match(event, match=None):
>>>>          """
>>>> -        Wait for specified timeout on named event in QMP; optionally filter
>>>> -        results by match.
>>>> +        Check if an event matches optional match criteria.
>>>>  
>>>> -        The 'match' is checked to be a recursive subset of the 'event'; skips
>>>> -        branch processing on match's value None
>>>> -           {"foo": {"bar": 1}} matches {"foo": None}
>>>> -           {"foo": {"bar": 1}} does not matches {"foo": {"baz": None}}
>>>> +        The match criteria takes the form of a matching subdict. The event is
>>>> +        checked to be a superset of the subdict, recursively, with matching
>>>> +        values whenever those values are not None.
>>>> +
>>>> +        Examples, with the subdict queries on the left:
>>>> +         - None matches any object.
>>>> +         - {"foo": None} matches {"foo": {"bar": 1}}
>>>> +         - {"foo": {"baz": None}} does not match {"foo": {"bar": 1}}
>>>
>>> Pre-existing, but the difference between “bar” and “baz” confused me
>>> quite a bit.
>>>
>>> Also, I wonder...  {"foo": None} would not match {"foo": 1}, right?
>>> Does that make sense?  Shouldn’t None be the wildcard here in general?
>>> (Also pre-existing of course.)
>>>
>>> But this patch doesn’t make things worse, so:
>>>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>
>>> (I’d still like your opinion.)
>>>
>>
>> I knew I was inviting trouble by trying to re-document this.
>>
>> The intention I had when writing the docs, which I think are wrong now,
>> was for {"foo": None} to match {"foo": 1}, but I think you're right that
>> it won't because '1' isn't a dict, so it tests for equality instead.
>>
>> So I need to fix this one up a little bit, but I'll take the review as a
>> sign that this approach seems workable.
> 
> I think the comment is technically completely correct.  It’s just that
> (1) it’s hard to discern “bar” from “baz”, and (2) if {"foo": None}
> intentionally does not match {"foo": 1}, we may want to document that,
> because it isn’t intuitively clear from the description.  If it’s a bug,
> the code should be fixed (and it should still be documented).
> 
> Max
> 

Yeah, I see. OK; I have prepared a patch that can be applied
independently after this series, if you'd like to stage this as-is. The
new patch changes behavior of this function a little, but is backwards
compatible with no changes because nobody was using these edge cases.

--js

Re: [Qemu-devel] [PATCH v3 0/5] blockdev-backup: don't check aio_context too early

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

On 23.05.19 19:06, John Snow wrote:
> See patch one's commit message for justification.
> Patches 2-5 are for testing, because that's always how these things go.
> 
> 001/5:[----] [--] 'blockdev-backup: don't check aio_context too early'
> 002/5:[0004] [FC] 'iotests.py: do not use infinite waits'
> 003/5:[down]      'QEMUMachine: add events_wait method'
> 004/5:[0022] [FC] 'iotests.py: rewrite run_job to be pickier'
> 005/5:[0017] [FC] 'iotests: add iotest 250 for testing blockdev-backup
>                    across iothread contexts'
> 
> v3: Rebased on Max's staging branch:
>     Rebase patch 2
>     added patch 3, to add events_wait.
>     Rework patch 4 to make run_job consume legacy events, too
>     Minorly edit patch 5 due to the two above.
> v2: added patch 4, with iotest framework adjustments in patches 2/3.

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block
https://github.com/XanClic/qemu/commits/block

(:-P)

Max


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