[Qemu-devel] [PATCH v4 0/5] bitmaps: remove x- prefix from QMP api Part2

[Qemu-devel] [PATCH v4 0/5] bitmaps: remove x- prefix from QMP api Part2

[John Snow]

Add testing to test the bitmap-merge API.

V4:
 - Removed patches 1-5 which have been staged
 - Rewrite qmp_log entirely, split into three patches
 - Pretty-printing has been extended to log() as well as qmp_log()
 - Adjust iotest 236 to be format generic instead of qcow2 [Vladimir]
 - Adjust iotest 236 to not reduplicate serialization work [Vladimir]
 - Many other small touchups

V3:
 - Reworked qmp_log to pretty-print the outgoing command, too [Vladimir]
 - Modified test to log only bitmap information [Vladimir]
 - Test disable/enable transaction toggle [Eric]

John Snow (5):
  iotests: add qmp recursive sorting function
  iotests: remove default filters from qmp_log
  iotests: change qmp_log filters to expect QMP objects only
  iotests: implement pretty-print for log and qmp_log
  iotests: add iotest 236 for testing bitmap merge

 tests/qemu-iotests/206        |   8 +-
 tests/qemu-iotests/236        | 131 ++++++++++++++++++++++
 tests/qemu-iotests/236.out    | 198 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  53 +++++++--
 5 files changed, 381 insertions(+), 10 deletions(-)
 create mode 100755 tests/qemu-iotests/236
 create mode 100644 tests/qemu-iotests/236.out

-- 
2.17.2

[Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function

[John Snow]

Python before 3.6 does not sort kwargs by default.
If we want to print out pretty-printed QMP objects while
preserving the "exec" > "arguments" ordering, we need a custom sort.

We can accomplish this by sorting **kwargs into an OrderedDict,
which does preserve addition order.
---
 tests/qemu-iotests/iotests.py | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9595429fea..9aec03c7a3 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@ import signal
 import logging
 import atexit
 import io
+from collections import OrderedDict
 
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
 import qtest
@@ -75,6 +76,16 @@ def qemu_img(*args):
         sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
     return exitcode
 
+def ordered_kwargs(kwargs):
+    # kwargs prior to 3.6 are not ordered, so:
+    od = OrderedDict()
+    for k in sorted(kwargs.keys()):
+        if isinstance(kwargs[k], dict):
+            od[k] = ordered_kwargs(kwargs[k])
+        else:
+            od[k] = kwargs[k]
+    return od
+
 def qemu_img_create(*args):
     args = list(args)
 
@@ -257,8 +268,9 @@ def filter_img_info(output, filename):
 def log(msg, filters=[]):
     for flt in filters:
         msg = flt(msg)
-    if type(msg) is dict or type(msg) is list:
-        print(json.dumps(msg, sort_keys=True))
+    if isinstance(msg, dict) or isinstance(msg, list):
+        sort_keys = not isinstance(msg, OrderedDict)
+        print(json.dumps(msg, sort_keys=sort_keys))
     else:
         print(msg)
 
@@ -448,8 +460,9 @@ class VM(qtest.QEMUQtestMachine):
         return result
 
     def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
-        logmsg = '{"execute": "%s", "arguments": %s}' % \
-            (cmd, json.dumps(kwargs, sort_keys=True))
+        full_cmd = OrderedDict({"execute": cmd,
+                                "arguments": ordered_kwargs(kwargs)})
+        logmsg = json.dumps(full_cmd)
         log(logmsg, filters)
         result = self.qmp(cmd, **kwargs)
         log(json.dumps(result, sort_keys=True), filters)
-- 
2.17.2

[Qemu-devel] [PATCH v4 2/5] iotests: remove default filters from qmp_log

[John Snow]

Only test 206 uses it, so remove it.
---
 tests/qemu-iotests/206        | 8 ++++++--
 tests/qemu-iotests/iotests.py | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index 128c334c7c..e92550fa59 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -26,7 +26,9 @@ from iotests import imgfmt
 iotests.verify_image_format(supported_fmts=['qcow2'])
 
 def blockdev_create(vm, options):
-    result = vm.qmp_log('blockdev-create', job_id='job0', options=options)
+    result = vm.qmp_log('blockdev-create',
+                        filters=[iotests.filter_testfiles],
+                        job_id='job0', options=options)
 
     if 'return' in result:
         assert result['return'] == {}
@@ -52,7 +54,9 @@ with iotests.FilePath('t.qcow2') as disk_path, \
                           'filename': disk_path,
                           'size': 0 })
 
-    vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
+    vm.qmp_log('blockdev-add',
+               filters=[iotests.filter_testfiles],
+               driver='file', filename=disk_path,
                node_name='imgfile')
 
     blockdev_create(vm, { 'driver': imgfmt,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9aec03c7a3..55fb60e039 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -459,7 +459,7 @@ class VM(qtest.QEMUQtestMachine):
             result.append(filter_qmp_event(ev))
         return result
 
-    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
+    def qmp_log(self, cmd, filters=[], **kwargs):
         full_cmd = OrderedDict({"execute": cmd,
                                 "arguments": ordered_kwargs(kwargs)})
         logmsg = json.dumps(full_cmd)
-- 
2.17.2

[Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[John Snow]

log() treats filters as if they can always filter its primary argument.
qmp_log treats filters as if they're always text.

Change qmp_log to treat filters as if they're always qmp object filters,
then change the logging call to rely on log()'s ability to serialize QMP
objects, so we're not duplicating that effort.

Because kwargs have been sorted already, the order is preserved.

Edit the only caller who uses filters on qmp_log to use a qmp version,
also added in this patch.
---
 tests/qemu-iotests/206        |  4 ++--
 tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index e92550fa59..5bb738bf23 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -27,7 +27,7 @@ iotests.verify_image_format(supported_fmts=['qcow2'])
 
 def blockdev_create(vm, options):
     result = vm.qmp_log('blockdev-create',
-                        filters=[iotests.filter_testfiles],
+                        filters=[iotests.filter_qmp_testfiles],
                         job_id='job0', options=options)
 
     if 'return' in result:
@@ -55,7 +55,7 @@ with iotests.FilePath('t.qcow2') as disk_path, \
                           'size': 0 })
 
     vm.qmp_log('blockdev-add',
-               filters=[iotests.filter_testfiles],
+               filters=[iotests.filter_qmp_testfiles],
                driver='file', filename=disk_path,
                node_name='imgfile')
 
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 55fb60e039..812302538d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -246,10 +246,29 @@ def filter_qmp_event(event):
         event['timestamp']['microseconds'] = 'USECS'
     return event
 
+def filter_qmp(qmsg, filter_fn):
+    '''Given a string filter, filter a QMP object's values.
+    filter_fn takes a (key, value) pair.'''
+    for key in qmsg:
+        if isinstance(qmsg[key], list):
+            qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
+        elif isinstance(qmsg[key], dict):
+            qmsg[key] = filter_qmp(qmsg[key], filter_fn)
+        else:
+            qmsg[key] = filter_fn(key, qmsg[key])
+    return qmsg
+
 def filter_testfiles(msg):
     prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
     return msg.replace(prefix, 'TEST_DIR/PID-')
 
+def filter_qmp_testfiles(qmsg):
+    def _filter(key, value):
+        if key == 'filename' or key == 'backing-file':
+            return filter_testfiles(value)
+        return value
+    return filter_qmp(qmsg, _filter)
+
 def filter_generated_node_ids(msg):
     return re.sub("#block[0-9]+", "NODE_NAME", msg)
 
@@ -462,10 +481,9 @@ class VM(qtest.QEMUQtestMachine):
     def qmp_log(self, cmd, filters=[], **kwargs):
         full_cmd = OrderedDict({"execute": cmd,
                                 "arguments": ordered_kwargs(kwargs)})
-        logmsg = json.dumps(full_cmd)
-        log(logmsg, filters)
+        log(full_cmd, filters)
         result = self.qmp(cmd, **kwargs)
-        log(json.dumps(result, sort_keys=True), filters)
+        log(result, filters)
         return result
 
     def run_job(self, job, auto_finalize=True, auto_dismiss=False):
-- 
2.17.2

[Qemu-devel] [PATCH v4 4/5] iotests: implement pretty-print for log and qmp_log

[John Snow]

If iotests have lines exceeding >998 characters long, git doesn't
want to send it plaintext to the list. We can solve this by allowing
the iotests to use pretty printed QMP output that we can match against
instead.

As a bonus, it's much nicer for human eyes too.
---
 tests/qemu-iotests/iotests.py | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 812302538d..d3c57e266f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -284,12 +284,18 @@ def filter_img_info(output, filename):
         lines.append(line)
     return '\n'.join(lines)
 
-def log(msg, filters=[]):
+def log(msg, filters=[], indent=None):
+    '''Logs either a string message or a JSON serializable message (like QMP).
+    If indent is provided, JSON serializable messages are pretty-printed.'''
     for flt in filters:
         msg = flt(msg)
     if isinstance(msg, dict) or isinstance(msg, list):
-        sort_keys = not isinstance(msg, OrderedDict)
-        print(json.dumps(msg, sort_keys=sort_keys))
+        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
+        separators = (', ', ': ') if indent is None else (',', ': ')
+        # Don't sort if it's already sorted
+        do_sort = not isinstance(msg, OrderedDict)
+        print(json.dumps(msg, sort_keys=do_sort,
+                         indent=indent, separators=separators))
     else:
         print(msg)
 
@@ -478,12 +484,12 @@ class VM(qtest.QEMUQtestMachine):
             result.append(filter_qmp_event(ev))
         return result
 
-    def qmp_log(self, cmd, filters=[], **kwargs):
+    def qmp_log(self, cmd, filters=[], indent=None, **kwargs):
         full_cmd = OrderedDict({"execute": cmd,
                                 "arguments": ordered_kwargs(kwargs)})
-        log(full_cmd, filters)
+        log(full_cmd, filters, indent=indent)
         result = self.qmp(cmd, **kwargs)
-        log(result, filters)
+        log(result, filters, indent=indent)
         return result
 
     def run_job(self, job, auto_finalize=True, auto_dismiss=False):
-- 
2.17.2

[Qemu-devel] [PATCH v4 5/5] iotests: add iotest 236 for testing bitmap merge

[John Snow]

New interface, new smoke test.
---
 tests/qemu-iotests/236     | 131 ++++++++++++++++++++++++
 tests/qemu-iotests/236.out | 198 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 330 insertions(+)
 create mode 100755 tests/qemu-iotests/236
 create mode 100644 tests/qemu-iotests/236.out

diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
new file mode 100755
index 0000000000..bf8d6882d0
--- /dev/null
+++ b/tests/qemu-iotests/236
@@ -0,0 +1,131 @@
+#!/usr/bin/env python
+#
+# Test bitmap merges.
+#
+# Copyright (c) 2018 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 iotests
+from iotests import log
+
+iotests.verify_image_format(supported_fmts=['generic'])
+size = 64 * 1024 * 1024
+granularity = 64 * 1024
+
+patterns = [("0x5d", "0",         "64k"),
+            ("0xd5", "1M",        "64k"),
+            ("0xdc", "32M",       "64k"),
+            ("0xcd", "0x3ff0000", "64k")]  # 64M - 64K
+
+overwrite = [("0xab", "0",         "64k"), # Full overwrite
+             ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K)
+             ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K)
+             ("0xea", "0x3fe0000", "64k")] # Adjacent-left (64M - 128K)
+
+def query_bitmaps(vm):
+    res = vm.qmp("query-block")
+    return {device['device']: device['dirty-bitmaps'] for
+            device in res['return']}
+
+with iotests.FilePath('img') as img_path, \
+     iotests.VM() as vm:
+
+    log('--- Preparing image & VM ---\n')
+    iotests.qemu_img_create('-f', iotests.imgfmt, img_path, str(size))
+    vm.add_drive(img_path)
+    vm.launch()
+
+    log('--- Adding preliminary bitmaps A & B ---\n')
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="bitmapA", granularity=granularity)
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+               name="bitmapB", granularity=granularity)
+
+    # Dirties 4 clusters. count=262144
+    log('')
+    log('--- Emulating writes ---\n')
+    for p in patterns:
+        cmd = "write -P%s %s %s" % p
+        log(cmd)
+        log(vm.hmp_qemu_io("drive0", cmd))
+
+    log(query_bitmaps(vm), indent=2)
+
+    log('')
+    log('--- Disabling B & Adding C ---\n')
+    vm.qmp_log("transaction", indent=2, actions=[
+        { "type": "block-dirty-bitmap-disable",
+          "data": { "node": "drive0", "name": "bitmapB" }},
+        { "type": "block-dirty-bitmap-add",
+          "data": { "node": "drive0", "name": "bitmapC",
+                    "granularity": granularity }},
+        # Purely extraneous, but test that it works:
+        { "type": "block-dirty-bitmap-disable",
+          "data": { "node": "drive0", "name": "bitmapC" }},
+        { "type": "block-dirty-bitmap-enable",
+          "data": { "node": "drive0", "name": "bitmapC" }},
+    ])
+
+    log('')
+    log('--- Emulating further writes ---\n')
+    # Dirties 6 clusters, 3 of which are new in contrast to "A".
+    # A = 64 * 1024 * (4 + 3) = 458752
+    # C = 64 * 1024 * 6       = 393216
+    for p in overwrite:
+        cmd = "write -P%s %s %s" % p
+        log(cmd)
+        log(vm.hmp_qemu_io("drive0", cmd))
+
+    log('')
+    log('--- Disabling A & C ---\n')
+    vm.qmp_log("transaction", indent=2, actions=[
+        { "type": "block-dirty-bitmap-disable",
+          "data": { "node": "drive0", "name": "bitmapA" }},
+        { "type": "block-dirty-bitmap-disable",
+          "data": { "node": "drive0", "name": "bitmapC" }}
+    ])
+
+    # A: 7 clusters
+    # B: 4 clusters
+    # C: 6 clusters
+    log(query_bitmaps(vm), indent=2)
+
+    log('')
+    log('--- Creating D as a merge of B & C ---\n')
+    # Good hygiene: create a disabled bitmap as a merge target.
+    vm.qmp_log("transaction", indent=2, actions=[
+        { "type": "block-dirty-bitmap-add",
+          "data": { "node": "drive0", "name": "bitmapD",
+                    "disabled": True, "granularity": granularity }},
+        { "type": "block-dirty-bitmap-merge",
+          "data": { "node": "drive0", "target": "bitmapD",
+                    "bitmaps": ["bitmapB", "bitmapC"] }}
+    ])
+
+    # A and D should now both have 7 clusters apiece.
+    # B and C remain unchanged with 4 and 6 respectively.
+    log(query_bitmaps(vm), indent=2)
+
+    # A and D should be equivalent.
+    # Some formats round the size of the disk, so don't print the checksums.
+    check_a = vm.qmp('x-debug-block-dirty-bitmap-sha256',
+                     node="drive0", name="bitmapA")['return']['sha256']
+    check_b = vm.qmp('x-debug-block-dirty-bitmap-sha256',
+                     node="drive0", name="bitmapD")['return']['sha256']
+    assert(check_a == check_b)
+
+    vm.shutdown()
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
new file mode 100644
index 0000000000..b5f8300816
--- /dev/null
+++ b/tests/qemu-iotests/236.out
@@ -0,0 +1,198 @@
+--- Preparing image & VM ---
+
+--- Adding preliminary bitmaps A & B ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmapA", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmapB", "node": "drive0"}}
+{"return": {}}
+
+--- Emulating writes ---
+
+write -P0x5d 0 64k
+{"return": ""}
+write -P0xd5 1M 64k
+{"return": ""}
+write -P0xdc 32M 64k
+{"return": ""}
+write -P0xcd 0x3ff0000 64k
+{"return": ""}
+{
+  "drive0": [
+    {
+      "count": 262144,
+      "granularity": 65536,
+      "name": "bitmapB",
+      "status": "active"
+    },
+    {
+      "count": 262144,
+      "granularity": 65536,
+      "name": "bitmapA",
+      "status": "active"
+    }
+  ]
+}
+
+--- Disabling B & Adding C ---
+
+{
+  "execute": "transaction",
+  "arguments": {
+    "actions": [
+      {
+        "data": {
+          "node": "drive0",
+          "name": "bitmapB"
+        },
+        "type": "block-dirty-bitmap-disable"
+      },
+      {
+        "data": {
+          "node": "drive0",
+          "name": "bitmapC",
+          "granularity": 65536
+        },
+        "type": "block-dirty-bitmap-add"
+      },
+      {
+        "data": {
+          "node": "drive0",
+          "name": "bitmapC"
+        },
+        "type": "block-dirty-bitmap-disable"
+      },
+      {
+        "data": {
+          "node": "drive0",
+          "name": "bitmapC"
+        },
+        "type": "block-dirty-bitmap-enable"
+      }
+    ]
+  }
+}
+{
+  "return": {}
+}
+
+--- Emulating further writes ---
+
+write -P0xab 0 64k
+{"return": ""}
+write -P0xad 0x00f8000 64k
+{"return": ""}
+write -P0x1d 0x2008000 64k
+{"return": ""}
+write -P0xea 0x3fe0000 64k
+{"return": ""}
+
+--- Disabling A & C ---
+
+{
+  "execute": "transaction",
+  "arguments": {
+    "actions": [
+      {
+        "data": {
+          "node": "drive0",
+          "name": "bitmapA"
+        },
+        "type": "block-dirty-bitmap-disable"
+      },
+      {
+        "data": {
+          "node": "drive0",
+          "name": "bitmapC"
+        },
+        "type": "block-dirty-bitmap-disable"
+      }
+    ]
+  }
+}
+{
+  "return": {}
+}
+{
+  "drive0": [
+    {
+      "count": 393216,
+      "granularity": 65536,
+      "name": "bitmapC",
+      "status": "disabled"
+    },
+    {
+      "count": 262144,
+      "granularity": 65536,
+      "name": "bitmapB",
+      "status": "disabled"
+    },
+    {
+      "count": 458752,
+      "granularity": 65536,
+      "name": "bitmapA",
+      "status": "disabled"
+    }
+  ]
+}
+
+--- Creating D as a merge of B & C ---
+
+{
+  "execute": "transaction",
+  "arguments": {
+    "actions": [
+      {
+        "data": {
+          "node": "drive0",
+          "disabled": true,
+          "name": "bitmapD",
+          "granularity": 65536
+        },
+        "type": "block-dirty-bitmap-add"
+      },
+      {
+        "data": {
+          "node": "drive0",
+          "target": "bitmapD",
+          "bitmaps": [
+            "bitmapB",
+            "bitmapC"
+          ]
+        },
+        "type": "block-dirty-bitmap-merge"
+      }
+    ]
+  }
+}
+{
+  "return": {}
+}
+{
+  "drive0": [
+    {
+      "count": 458752,
+      "granularity": 65536,
+      "name": "bitmapD",
+      "status": "disabled"
+    },
+    {
+      "count": 393216,
+      "granularity": 65536,
+      "name": "bitmapC",
+      "status": "disabled"
+    },
+    {
+      "count": 262144,
+      "granularity": 65536,
+      "name": "bitmapB",
+      "status": "disabled"
+    },
+    {
+      "count": 458752,
+      "granularity": 65536,
+      "name": "bitmapA",
+      "status": "disabled"
+    }
+  ]
+}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 61a6d98ebd..a61f9e83d6 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -233,3 +233,4 @@
 233 auto quick
 234 auto quick migration
 235 auto quick
+236 auto quick
\ No newline at end of file
-- 
2.17.2

Re: [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function

[Vladimir Sementsov-Ogievskiy]

19.12.2018 4:52, John Snow wrote:
> Python before 3.6 does not sort kwargs by default.
> If we want to print out pretty-printed QMP objects while
> preserving the "exec" > "arguments" ordering, we need a custom sort.
> 
> We can accomplish this by sorting **kwargs into an OrderedDict,
> which does preserve addition order.
> ---
>   tests/qemu-iotests/iotests.py | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 9595429fea..9aec03c7a3 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -30,6 +30,7 @@ import signal
>   import logging
>   import atexit
>   import io
> +from collections import OrderedDict
>   
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
>   import qtest
> @@ -75,6 +76,16 @@ def qemu_img(*args):
>           sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
>       return exitcode
>   
> +def ordered_kwargs(kwargs):
> +    # kwargs prior to 3.6 are not ordered, so:
> +    od = OrderedDict()
> +    for k in sorted(kwargs.keys()):

you can use for k, v in sorted(kwargs.items()):
and use then v instead of kwargs[k]

> +        if isinstance(kwargs[k], dict):
> +            od[k] = ordered_kwargs(kwargs[k])
> +        else:
> +            od[k] = kwargs[k]
> +    return od
> +
>   def qemu_img_create(*args):
>       args = list(args)
>   
> @@ -257,8 +268,9 @@ def filter_img_info(output, filename):
>   def log(msg, filters=[]):
>       for flt in filters:
>           msg = flt(msg)

I think that trying to apply text filters to object should be fixed first.

> -    if type(msg) is dict or type(msg) is list:
> -        print(json.dumps(msg, sort_keys=True))
> +    if isinstance(msg, dict) or isinstance(msg, list):
> +        sort_keys = not isinstance(msg, OrderedDict)
> +        print(json.dumps(msg, sort_keys=sort_keys))
>       else:
>           print(msg)
>   
> @@ -448,8 +460,9 @@ class VM(qtest.QEMUQtestMachine):
>           return result
>   
>       def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
> -        logmsg = '{"execute": "%s", "arguments": %s}' % \
> -            (cmd, json.dumps(kwargs, sort_keys=True))
> +        full_cmd = OrderedDict({"execute": cmd,
> +                                "arguments": ordered_kwargs(kwargs)})

no, you can't use dict as a parameter to constructor, as dict is not ordered,
use tuple of tuples, like OrderedDict((('execute': cmd), ('execute': ...)))



> +        logmsg = json.dumps(full_cmd)
>           log(logmsg, filters)

and I prefere fixing the thing, that we do json.dumps both in log() and qmp_log() before
this patch.

Also: so, we move all qmp_log callers to new logic (through sorting by hand with ordered_kwargs),
and it works? Then, maybe, move all log callers to new logic, and get rid of json.dumps at all,
to have one path instead of two?

>           result = self.qmp(cmd, **kwargs)
>           log(json.dumps(result, sort_keys=True), filters)
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 2/5] iotests: remove default filters from qmp_log

[Vladimir Sementsov-Ogievskiy]

19.12.2018 4:52, John Snow wrote:
> Only test 206 uses it, so remove it.

s-o-b

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[Vladimir Sementsov-Ogievskiy]

19.12.2018 4:52, John Snow wrote:
> log() treats filters as if they can always filter its primary argument.
> qmp_log treats filters as if they're always text.
> 
> Change qmp_log to treat filters as if they're always qmp object filters,
> then change the logging call to rely on log()'s ability to serialize QMP
> objects, so we're not duplicating that effort.

As I understand, there still no use for qmp-object based filters (even after the
series), do we really need them? I'm afraid it's premature complication.

Why not to keep all filters text based? If we need to filter some concrete fields,
not the whole thing, I doubt that recursion and defining functions inside
functions is a true way for it. Instead in concrete case, like in you test, it's
better to select fields that should be in output, and output only them.

> 
> Because kwargs have been sorted already, the order is preserved.
> 
> Edit the only caller who uses filters on qmp_log to use a qmp version,
> also added in this patch.
> ---
>   tests/qemu-iotests/206        |  4 ++--
>   tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
>   2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
> index e92550fa59..5bb738bf23 100755
> --- a/tests/qemu-iotests/206
> +++ b/tests/qemu-iotests/206
> @@ -27,7 +27,7 @@ iotests.verify_image_format(supported_fmts=['qcow2'])
>   
>   def blockdev_create(vm, options):
>       result = vm.qmp_log('blockdev-create',
> -                        filters=[iotests.filter_testfiles],
> +                        filters=[iotests.filter_qmp_testfiles],
>                           job_id='job0', options=options)
>   
>       if 'return' in result:
> @@ -55,7 +55,7 @@ with iotests.FilePath('t.qcow2') as disk_path, \
>                             'size': 0 })
>   
>       vm.qmp_log('blockdev-add',
> -               filters=[iotests.filter_testfiles],
> +               filters=[iotests.filter_qmp_testfiles],
>                  driver='file', filename=disk_path,
>                  node_name='imgfile')
>   
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 55fb60e039..812302538d 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -246,10 +246,29 @@ def filter_qmp_event(event):
>           event['timestamp']['microseconds'] = 'USECS'
>       return event
>   
> +def filter_qmp(qmsg, filter_fn):
> +    '''Given a string filter, filter a QMP object's values.
> +    filter_fn takes a (key, value) pair.'''
> +    for key in qmsg:
> +        if isinstance(qmsg[key], list):
> +            qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
> +        elif isinstance(qmsg[key], dict):
> +            qmsg[key] = filter_qmp(qmsg[key], filter_fn)
> +        else:
> +            qmsg[key] = filter_fn(key, qmsg[key])
> +    return qmsg
> +
>   def filter_testfiles(msg):
>       prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>       return msg.replace(prefix, 'TEST_DIR/PID-')
>   
> +def filter_qmp_testfiles(qmsg):
> +    def _filter(key, value):
> +        if key == 'filename' or key == 'backing-file':
> +            return filter_testfiles(value)
> +        return value
> +    return filter_qmp(qmsg, _filter)
> +
>   def filter_generated_node_ids(msg):
>       return re.sub("#block[0-9]+", "NODE_NAME", msg)
>   
> @@ -462,10 +481,9 @@ class VM(qtest.QEMUQtestMachine):
>       def qmp_log(self, cmd, filters=[], **kwargs):
>           full_cmd = OrderedDict({"execute": cmd,
>                                   "arguments": ordered_kwargs(kwargs)})
> -        logmsg = json.dumps(full_cmd)
> -        log(logmsg, filters)
> +        log(full_cmd, filters)
>           result = self.qmp(cmd, **kwargs)
> -        log(json.dumps(result, sort_keys=True), filters)
> +        log(result, filters)
>           return result
>   
>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[Vladimir Sementsov-Ogievskiy]

19.12.2018 14:07, Vladimir Sementsov-Ogievskiy wrote:
> 19.12.2018 4:52, John Snow wrote:
>> log() treats filters as if they can always filter its primary argument.
>> qmp_log treats filters as if they're always text.
>>
>> Change qmp_log to treat filters as if they're always qmp object filters,
>> then change the logging call to rely on log()'s ability to serialize QMP
>> objects, so we're not duplicating that effort.
> 
> As I understand, there still no use for qmp-object based filters (even after the
> series), do we really need them? I'm afraid it's premature complication.

aha, sorry, missed that you use it in 206.
But still not sure that it worth it. Isn't it better to just remove fields from dict,
which are unpredictable, instead of substituting them..

The other idea here: if we want
automatically logged qmp commands (qmp_log(), actually), it should filter unpredictable
things from output automatically, just by command, which is the first argument. Caller
should not care about it, as it may be derived from command, how to filter it's output.
And then, we just need a kind of dict of functions, which do not do something like generic
recursion, but specifically prepares common-test-output for the concrete command...

> 
> Why not to keep all filters text based? If we need to filter some concrete fields,
> not the whole thing, I doubt that recursion and defining functions inside
> functions is a true way for it. Instead in concrete case, like in you test, it's
> better to select fields that should be in output, and output only them.
> 
>>
>> Because kwargs have been sorted already, the order is preserved.
>>
>> Edit the only caller who uses filters on qmp_log to use a qmp version,
>> also added in this patch.
>> ---
>>   tests/qemu-iotests/206        |  4 ++--
>>   tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
>>   2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
>> index e92550fa59..5bb738bf23 100755
>> --- a/tests/qemu-iotests/206
>> +++ b/tests/qemu-iotests/206
>> @@ -27,7 +27,7 @@ iotests.verify_image_format(supported_fmts=['qcow2'])
>>   def blockdev_create(vm, options):
>>       result = vm.qmp_log('blockdev-create',
>> -                        filters=[iotests.filter_testfiles],
>> +                        filters=[iotests.filter_qmp_testfiles],
>>                           job_id='job0', options=options)
>>       if 'return' in result:
>> @@ -55,7 +55,7 @@ with iotests.FilePath('t.qcow2') as disk_path, \
>>                             'size': 0 })
>>       vm.qmp_log('blockdev-add',
>> -               filters=[iotests.filter_testfiles],
>> +               filters=[iotests.filter_qmp_testfiles],
>>                  driver='file', filename=disk_path,
>>                  node_name='imgfile')
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 55fb60e039..812302538d 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -246,10 +246,29 @@ def filter_qmp_event(event):
>>           event['timestamp']['microseconds'] = 'USECS'
>>       return event
>> +def filter_qmp(qmsg, filter_fn):
>> +    '''Given a string filter, filter a QMP object's values.
>> +    filter_fn takes a (key, value) pair.'''
>> +    for key in qmsg:
>> +        if isinstance(qmsg[key], list):
>> +            qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
>> +        elif isinstance(qmsg[key], dict):
>> +            qmsg[key] = filter_qmp(qmsg[key], filter_fn)
>> +        else:
>> +            qmsg[key] = filter_fn(key, qmsg[key])
>> +    return qmsg
>> +
>>   def filter_testfiles(msg):
>>       prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>>       return msg.replace(prefix, 'TEST_DIR/PID-')
>> +def filter_qmp_testfiles(qmsg):
>> +    def _filter(key, value):
>> +        if key == 'filename' or key == 'backing-file':
>> +            return filter_testfiles(value)
>> +        return value
>> +    return filter_qmp(qmsg, _filter)
>> +
>>   def filter_generated_node_ids(msg):
>>       return re.sub("#block[0-9]+", "NODE_NAME", msg)
>> @@ -462,10 +481,9 @@ class VM(qtest.QEMUQtestMachine):
>>       def qmp_log(self, cmd, filters=[], **kwargs):
>>           full_cmd = OrderedDict({"execute": cmd,
>>                                   "arguments": ordered_kwargs(kwargs)})
>> -        logmsg = json.dumps(full_cmd)
>> -        log(logmsg, filters)
>> +        log(full_cmd, filters)
>>           result = self.qmp(cmd, **kwargs)
>> -        log(json.dumps(result, sort_keys=True), filters)
>> +        log(result, filters)
>>           return result
>>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[John Snow]

On 12/19/18 6:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 19.12.2018 14:07, Vladimir Sementsov-Ogievskiy wrote:
>> 19.12.2018 4:52, John Snow wrote:
>>> log() treats filters as if they can always filter its primary argument.
>>> qmp_log treats filters as if they're always text.
>>>
>>> Change qmp_log to treat filters as if they're always qmp object filters,
>>> then change the logging call to rely on log()'s ability to serialize QMP
>>> objects, so we're not duplicating that effort.
>>
>> As I understand, there still no use for qmp-object based filters (even after the
>> series), do we really need them? I'm afraid it's premature complication.
> 
> aha, sorry, missed that you use it in 206.
> But still not sure that it worth it. Isn't it better to just remove fields from dict,
> which are unpredictable, instead of substituting them..
> 

I'd like to keep the QMP output a prettified version of the plaintext
output, and not have it omit things. You can make the case for changing
that behavior in a separate patch.

> The other idea here: if we want
> automatically logged qmp commands (qmp_log(), actually), it should filter unpredictable
> things from output automatically, just by command, which is the first argument. Caller
> should not care about it, as it may be derived from command, how to filter it's output.
> And then, we just need a kind of dict of functions, which do not do something like generic
> recursion, but specifically prepares common-test-output for the concrete command...
> 

Feel free to enhance the test suite later to understand all the types of
commands and replies and scrub them automatically. For now, specifying
the filters matches behavior in much of the rest of the test suite and I
am not motivated to fix it.

Re: [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function

[John Snow]

On 12/19/18 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 19.12.2018 4:52, John Snow wrote:
>> Python before 3.6 does not sort kwargs by default.
>> If we want to print out pretty-printed QMP objects while
>> preserving the "exec" > "arguments" ordering, we need a custom sort.
>>
>> We can accomplish this by sorting **kwargs into an OrderedDict,
>> which does preserve addition order.
>> ---
>>   tests/qemu-iotests/iotests.py | 21 +++++++++++++++++----
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>

Hm, my patch sending script broke and it's not adding my S-o-B lines.
I'll fix that.

>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 9595429fea..9aec03c7a3 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -30,6 +30,7 @@ import signal
>>   import logging
>>   import atexit
>>   import io
>> +from collections import OrderedDict
>>   
>>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
>>   import qtest
>> @@ -75,6 +76,16 @@ def qemu_img(*args):
>>           sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
>>       return exitcode
>>   
>> +def ordered_kwargs(kwargs):
>> +    # kwargs prior to 3.6 are not ordered, so:
>> +    od = OrderedDict()
>> +    for k in sorted(kwargs.keys()):
> 
> you can use for k, v in sorted(kwargs.items()):
> and use then v instead of kwargs[k]
> 

I don't need to sort the tuples, though, Only the keys -- which are not
duplicated. Is it really worth changing? ...

>> +        if isinstance(kwargs[k], dict):
>> +            od[k] = ordered_kwargs(kwargs[k])
>> +        else:
>> +            od[k] = kwargs[k]
>> +    return od
>> +
>>   def qemu_img_create(*args):
>>       args = list(args)
>>   
>> @@ -257,8 +268,9 @@ def filter_img_info(output, filename):
>>   def log(msg, filters=[]):
>>       for flt in filters:
>>           msg = flt(msg)
> 
> I think that trying to apply text filters to object should be fixed first.
> 

Text filters *aren't* applied to objects before this patch.

I think log should apply the filters you give it to the object you give
it, whether or not that's text or QMP objects. If you give it the wrong
filters that's your fault.

That's the way it works now, and I don't think it's broken.

>> -    if type(msg) is dict or type(msg) is list:
>> -        print(json.dumps(msg, sort_keys=True))
>> +    if isinstance(msg, dict) or isinstance(msg, list):
>> +        sort_keys = not isinstance(msg, OrderedDict)
>> +        print(json.dumps(msg, sort_keys=sort_keys))
>>       else:
>>           print(msg)
>>   
>> @@ -448,8 +460,9 @@ class VM(qtest.QEMUQtestMachine):
>>           return result
>>   
>>       def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>> -        logmsg = '{"execute": "%s", "arguments": %s}' % \
>> -            (cmd, json.dumps(kwargs, sort_keys=True))
>> +        full_cmd = OrderedDict({"execute": cmd,
>> +                                "arguments": ordered_kwargs(kwargs)})
> 
> no, you can't use dict as a parameter to constructor, as dict is not ordered,
> use tuple of tuples, like OrderedDict((('execute': cmd), ('execute': ...)))
> 

Ah, I see the problem...

> 
> 
>> +        logmsg = json.dumps(full_cmd)
>>           log(logmsg, filters)
> 
> and I prefere fixing the thing, that we do json.dumps both in log() and qmp_log() before
> this patch.
> 
> Also: so, we move all qmp_log callers to new logic (through sorting by hand with ordered_kwargs),
> and it works? Then, maybe, move all log callers to new logic, and get rid of json.dumps at all,
> to have one path instead of two?

(There's only one call to dumps by the end of this series, so we're
heading in that direction. I don't want to make callers need to learn
that they need to call ordered_kwargs or build an OrderedDict, I'd
rather let qmp_log handle that.)

> 

The motivation is that log() will log whatever you give it and apply
filters that work on that kind of object. Some callers need to filter
rich QMP objects and some callers need to filter text -- this is the way
log() behaves right now and I simply didn't change it.

What qmp_log currently does is convert both the outgoing and incoming
QMP objects to text, and then filters them as text. However, only
precisely one test (206) uses this functionality.

So... I need some way for test 206 to do what it does. One way is to
make a rich QMP filter, which is what I do later in this series under
the pretense that other tests will likely want to filter QMP output, too.

The other approach involves teaching qmp_log to accept two kinds of
filters (qmp and text) and then passing both along to log(), which will
then filter the object before pretty-printing and then apply the text
filters after pretty-printing, and then logging the result.

As it stands now, though, log() just applies all filters to the first
argument without the caller needing to disambiguate. If I teach log() to
use two types of filters, I need to go back through all of the iotests
and teach all callers to specify e.g. qfilters= or tfilters=.

I opted for an approach that let me just edit test 206 instead -- and
one that added a recursive QMP filter that might be useful in the future
for other purposes.

>>           result = self.qmp(cmd, **kwargs)
>>           log(json.dumps(result, sort_keys=True), filters)
>>
> 
> 

Re: [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[John Snow]

On 12/19/18 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> 19.12.2018 4:52, John Snow wrote:
>> log() treats filters as if they can always filter its primary argument.
>> qmp_log treats filters as if they're always text.
>>
>> Change qmp_log to treat filters as if they're always qmp object filters,
>> then change the logging call to rely on log()'s ability to serialize QMP
>> objects, so we're not duplicating that effort.
> 
> As I understand, there still no use for qmp-object based filters (even after the
> series), do we really need them? I'm afraid it's premature complication.
> 

There are callers of log() that use qmp filters. Those callers can now
migrate over to qmp_log to get both the call and response.

There ARE users of QMP filters.

Look at `git grep 'log(vm'` for callers that are passing rich QMP
objects. The ones that pass filters are usually passing filter_qmp_event.

Now, if we choose, we can move them over to using qmp_log and amend the
logging output to get both the outgoing and returning message.

-- hmm, maybe, if you want, and I am NOT suggesting I will do this
before the holiday break (and therefore not in this series) -- what we
can do is this:

log(txt, filters=[]) -- Takes text and text filters only.

log_qmp(obj, tfilters=[], qfilters=[]) -- Logs a QMP object, takes QMP
filters for pre-filtering and tfilters for post-filtering. Contains the
json.dumps call. Simply passes tfilters down to log().

vm.qmp(log=1, tfilters=[], qfilters=[], ...) -- Perform the actual QMP
call and response, logging the outgoing and incoming objects via log_qmp.

I can use this patchset as a starting point to do that. It will involve
amending a lot of existing tests and test outputs, so I won't do this
unless there appears to be some support for that API in advance.

> Why not to keep all filters text based? If we need to filter some concrete fields,
> not the whole thing, I doubt that recursion and defining functions inside
> functions is a true way for it. Instead in concrete case, like in you test, it's
> better to select fields that should be in output, and output only them.
> 
>>
>> Because kwargs have been sorted already, the order is preserved.
>>
>> Edit the only caller who uses filters on qmp_log to use a qmp version,
>> also added in this patch.
>> ---
>>   tests/qemu-iotests/206        |  4 ++--
>>   tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
>>   2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
>> index e92550fa59..5bb738bf23 100755
>> --- a/tests/qemu-iotests/206
>> +++ b/tests/qemu-iotests/206
>> @@ -27,7 +27,7 @@ iotests.verify_image_format(supported_fmts=['qcow2'])
>>   
>>   def blockdev_create(vm, options):
>>       result = vm.qmp_log('blockdev-create',
>> -                        filters=[iotests.filter_testfiles],
>> +                        filters=[iotests.filter_qmp_testfiles],
>>                           job_id='job0', options=options)
>>   
>>       if 'return' in result:
>> @@ -55,7 +55,7 @@ with iotests.FilePath('t.qcow2') as disk_path, \
>>                             'size': 0 })
>>   
>>       vm.qmp_log('blockdev-add',
>> -               filters=[iotests.filter_testfiles],
>> +               filters=[iotests.filter_qmp_testfiles],
>>                  driver='file', filename=disk_path,
>>                  node_name='imgfile')
>>   
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 55fb60e039..812302538d 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -246,10 +246,29 @@ def filter_qmp_event(event):
>>           event['timestamp']['microseconds'] = 'USECS'
>>       return event
>>   
>> +def filter_qmp(qmsg, filter_fn):
>> +    '''Given a string filter, filter a QMP object's values.
>> +    filter_fn takes a (key, value) pair.'''
>> +    for key in qmsg:
>> +        if isinstance(qmsg[key], list):
>> +            qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
>> +        elif isinstance(qmsg[key], dict):
>> +            qmsg[key] = filter_qmp(qmsg[key], filter_fn)
>> +        else:
>> +            qmsg[key] = filter_fn(key, qmsg[key])
>> +    return qmsg
>> +
>>   def filter_testfiles(msg):
>>       prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>>       return msg.replace(prefix, 'TEST_DIR/PID-')
>>   
>> +def filter_qmp_testfiles(qmsg):
>> +    def _filter(key, value):
>> +        if key == 'filename' or key == 'backing-file':
>> +            return filter_testfiles(value)
>> +        return value
>> +    return filter_qmp(qmsg, _filter)
>> +
>>   def filter_generated_node_ids(msg):
>>       return re.sub("#block[0-9]+", "NODE_NAME", msg)
>>   
>> @@ -462,10 +481,9 @@ class VM(qtest.QEMUQtestMachine):
>>       def qmp_log(self, cmd, filters=[], **kwargs):
>>           full_cmd = OrderedDict({"execute": cmd,
>>                                   "arguments": ordered_kwargs(kwargs)})
>> -        logmsg = json.dumps(full_cmd)
>> -        log(logmsg, filters)
>> +        log(full_cmd, filters)
>>           result = self.qmp(cmd, **kwargs)
>> -        log(json.dumps(result, sort_keys=True), filters)
>> +        log(result, filters)
>>           return result
>>   
>>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>
> 
> 

-- 
—js

Re: [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function

[Eric Blake]

On 12/19/18 11:55 AM, John Snow wrote:

>>> +def ordered_kwargs(kwargs):
>>> +    # kwargs prior to 3.6 are not ordered, so:
>>> +    od = OrderedDict()
>>> +    for k in sorted(kwargs.keys()):
>>
>> you can use for k, v in sorted(kwargs.items()):
>> and use then v instead of kwargs[k]
>>
> 
> I don't need to sort the tuples, though, Only the keys -- which are not
> duplicated. Is it really worth changing? ...

If I'm reading this correctly:
https://www.pythoncentral.io/how-to-sort-python-dictionaries-by-key-or-value/

sorting tuples with unique keys is the same as sorting by the keys, but 
gives you the value (as part of the tuple) for free.  So the benefit is 
that:

> 
>>> +        if isinstance(kwargs[k], dict):
>>> +            od[k] = ordered_kwargs(kwargs[k])

here, you'd write:

if isinstance(v, dict):
   od[k] = ordered_kwargs(v)

instead of having to repeat the value lookup.

> 
> The motivation is that log() will log whatever you give it and apply
> filters that work on that kind of object. Some callers need to filter
> rich QMP objects and some callers need to filter text -- this is the way
> log() behaves right now and I simply didn't change it.
> 
> What qmp_log currently does is convert both the outgoing and incoming
> QMP objects to text, and then filters them as text. However, only
> precisely one test (206) uses this functionality.
> 
> So... I need some way for test 206 to do what it does. One way is to
> make a rich QMP filter, which is what I do later in this series under
> the pretense that other tests will likely want to filter QMP output, too.
> 
> The other approach involves teaching qmp_log to accept two kinds of
> filters (qmp and text) and then passing both along to log(), which will
> then filter the object before pretty-printing and then apply the text
> filters after pretty-printing, and then logging the result.
> 
> As it stands now, though, log() just applies all filters to the first
> argument without the caller needing to disambiguate. If I teach log() to
> use two types of filters, I need to go back through all of the iotests
> and teach all callers to specify e.g. qfilters= or tfilters=.
> 
> I opted for an approach that let me just edit test 206 instead -- and
> one that added a recursive QMP filter that might be useful in the future
> for other purposes.

The reasoning here makes sense to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function

[Eric Blake]

On 12/18/18 7:52 PM, John Snow wrote:
> Python before 3.6 does not sort kwargs by default.
> If we want to print out pretty-printed QMP objects while
> preserving the "exec" > "arguments" ordering, we need a custom sort.

Naive question - why do we need the sorting? Is it so that the output is 
deterministic?  Surely it can't be because the ordering otherwise makes 
a difference to execution.

Here's my review from a non-Python export viewpoint:

> 
> We can accomplish this by sorting **kwargs into an OrderedDict,
> which does preserve addition order.
> ---
>   tests/qemu-iotests/iotests.py | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 deletions(-)
> 

> @@ -448,8 +460,9 @@ class VM(qtest.QEMUQtestMachine):
>           return result
>   
>       def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
> -        logmsg = '{"execute": "%s", "arguments": %s}' % \
> -            (cmd, json.dumps(kwargs, sort_keys=True))
> +        full_cmd = OrderedDict({"execute": cmd,
> +                                "arguments": ordered_kwargs(kwargs)})
> +        logmsg = json.dumps(full_cmd)

Vladimir knows Python better than me, but once you fix this OrderedDict 
construction up to actually work, the patch looks reasonable to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function

[John Snow]

On 12/19/18 1:52 PM, Eric Blake wrote:
> On 12/18/18 7:52 PM, John Snow wrote:
>> Python before 3.6 does not sort kwargs by default.
>> If we want to print out pretty-printed QMP objects while
>> preserving the "exec" > "arguments" ordering, we need a custom sort.
> 
> Naive question - why do we need the sorting? Is it so that the output is
> deterministic?  Surely it can't be because the ordering otherwise makes
> a difference to execution.
> 

Yeah, it's because dicts are unordered and prior to 3.6 they may shuffle
around arbitrarily based on internal hashes.

kwargs in particular are unordered- the order we send over the wire may
or may not reflect the order the test author wrote in their iotest.

Therefore, it's a way to get consistent ordering.

Now, we CAN just rely on sort_keys=True to be done with it, however,
this puts arguments before execute, and it's less nice to read -- and
I'd have to change a LOT of test output.

This way keeps the order you expect to see while maintaining a strict
order for the arguments. I think that's the nicest compromise until we
can stipulate 3.6+ in the year 2034 where kwargs *are* strictly ordered.

> Here's my review from a non-Python export viewpoint:
> 
>>
>> We can accomplish this by sorting **kwargs into an OrderedDict,
>> which does preserve addition order.
>> ---
>>   tests/qemu-iotests/iotests.py | 21 +++++++++++++++++----
>>   1 file changed, 17 insertions(+), 4 deletions(-)
>>
> 
>> @@ -448,8 +460,9 @@ class VM(qtest.QEMUQtestMachine):
>>           return result
>>         def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>> -        logmsg = '{"execute": "%s", "arguments": %s}' % \
>> -            (cmd, json.dumps(kwargs, sort_keys=True))
>> +        full_cmd = OrderedDict({"execute": cmd,
>> +                                "arguments": ordered_kwargs(kwargs)})
>> +        logmsg = json.dumps(full_cmd)
> 
> Vladimir knows Python better than me, but once you fix this OrderedDict
> construction up to actually work, the patch looks reasonable to me.
> 

Yeah, it only worked by accident of implementation details :(
I've patched it up locally.

--js

Re: [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[Eric Blake]

On 12/19/18 5:27 AM, Vladimir Sementsov-Ogievskiy wrote:

> But still not sure that it worth it. Isn't it better to just remove fields from dict,
> which are unpredictable, instead of substituting them..

For getting the test to pass when we have a key:unpredictable value in 
the dict, you are right that both changing it to key:SUBST or removing 
key work at producing reproducible output. But when it comes to 
debugging test failure, having key:SUBST in the logs gives you a hint at 
what else to look at, whereas omitting key altogether may make the 
reason for the failure completely disappear from the logs.

Thus, I would argue that even though it is more complex to write a 
filter that can recursively substitute, the resulting output is easier 
to debug if a test starts failing - and that if the work in doing the 
more complex filtering has already been submitted and is not too much of 
a burden to maintain, then we might as well use it rather than going 
with the simpler case of just eliding the problematic keys or using just 
textual filtering.

However, I'm not in a good position to argue whether there is a 
reasonable maintenance burden with the patches in this series, vs. what 
it would take to rewrite 206 to do just textual filtering instead of QMP 
dict substitution filtering.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function

[Eric Blake]

On 12/19/18 12:57 PM, John Snow wrote:
> 
> 
> On 12/19/18 1:52 PM, Eric Blake wrote:
>> On 12/18/18 7:52 PM, John Snow wrote:
>>> Python before 3.6 does not sort kwargs by default.
>>> If we want to print out pretty-printed QMP objects while
>>> preserving the "exec" > "arguments" ordering, we need a custom sort.
>>
>> Naive question - why do we need the sorting? Is it so that the output is
>> deterministic?  Surely it can't be because the ordering otherwise makes
>> a difference to execution.
>>
> 
> Yeah, it's because dicts are unordered and prior to 3.6 they may shuffle
> around arbitrarily based on internal hashes.
> 
> kwargs in particular are unordered- the order we send over the wire may
> or may not reflect the order the test author wrote in their iotest.

Which shouldn't matter to what QMP executes, but MIGHT matter in getting 
reproducible log output.

> 
> Therefore, it's a way to get consistent ordering.
> 
> Now, we CAN just rely on sort_keys=True to be done with it, however,
> this puts arguments before execute, and it's less nice to read -- and
> I'd have to change a LOT of test output.

Okay, I'm finally seeing it; the existing code has:

     def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
         logmsg = '{"execute": "%s", "arguments": %s}' % \
             (cmd, json.dumps(kwargs, sort_keys=True))

where our log is outputting a message that resembles a dict where 
"execute": is the first key, and the user's input dict is then sorted 
(the top-most output of {} is unsorted, but the nested ones are sorted, 
and possibly in a different order than the user typed them, but at least 
deterministic). But when you change to the user passing a full QMP 
command (rather than just a dict for the arguments of the QMP command), 
using sort_keys=True will sort everything _including_ putting 
"arguments" before "execute" (which is deterministic but changes log 
output); while using sort_keys=False will not affect output in newer 
python where kwargs remains ordered, but risks nondeterministic output 
on older python.

> 
> This way keeps the order you expect to see while maintaining a strict
> order for the arguments. I think that's the nicest compromise until we
> can stipulate 3.6+ in the year 2034 where kwargs *are* strictly ordered.

But kwargs strict ordering is only at the top-level, not recursive, 
right? That is, even if you can rely on:

foo(b=1, a=2)

providing kwargs where 'b' always outputs before 'a', I don't see how 
that would help:

foo(b={'d':3, 'c':4})

not reorder the keys within 'b' without your recursive ordering.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v4 5/5] iotests: add iotest 236 for testing bitmap merge

[Eric Blake]

On 12/18/18 7:52 PM, John Snow wrote:
> New interface, new smoke test.
> ---
>   tests/qemu-iotests/236     | 131 ++++++++++++++++++++++++
>   tests/qemu-iotests/236.out | 198 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 330 insertions(+)
>   create mode 100755 tests/qemu-iotests/236
>   create mode 100644 tests/qemu-iotests/236.out
> 

> +
> +    log('')
> +    log('--- Disabling B & Adding C ---\n')
> +    vm.qmp_log("transaction", indent=2, actions=[
> +        { "type": "block-dirty-bitmap-disable",
> +          "data": { "node": "drive0", "name": "bitmapB" }},
> +        { "type": "block-dirty-bitmap-add",
> +          "data": { "node": "drive0", "name": "bitmapC",
> +                    "granularity": granularity }},
> +        # Purely extraneous, but test that it works:
> +        { "type": "block-dirty-bitmap-disable",
> +          "data": { "node": "drive0", "name": "bitmapC" }},
> +        { "type": "block-dirty-bitmap-enable",
> +          "data": { "node": "drive0", "name": "bitmapC" }},
> +    ])

One other possible addition just before this point:

qmp_log("transaction", indent=2, actions=[
         { "type": "block-dirty-bitmap-disable",
           "data": { "node": "drive0", "name": "bitmapB" }},
         { "type": "block-dirty-bitmap-add",
           "data": { "node": "drive0", "name": "bitmapC",
                     "granularity": granularity }},
         { "type": "block-dirty-bitmap-remove",
           "data": { "node": "drive0", "name": "bitmapA" }},
         { "type": "abort", "data": {}}
   ])

to check that we properly undo things on an aborted transaction (B 
should still be enabled, C should not exist, and A should not be damaged).

> +    # A and D should be equivalent.
> +    # Some formats round the size of the disk, so don't print the checksums.
> +    check_a = vm.qmp('x-debug-block-dirty-bitmap-sha256',
> +                     node="drive0", name="bitmapA")['return']['sha256']
> +    check_b = vm.qmp('x-debug-block-dirty-bitmap-sha256',
> +                     node="drive0", name="bitmapD")['return']['sha256']
> +    assert(check_a == check_b)

Image agnostic also means that you avoid an 32- vs. 64-bit platform 
discrepancies (we've had issues in the past where the sum is different 
for some image sizes, because the bitmap is always in terms of 'long's, 
but there is one less 'long' in a 32-bit bitmap for the disk size). 
Makes sense.

Also, I don't see any tests of block-dirty-bitmap-remove - this would be 
a good point in the test to insert a final cleanup.


> +++ b/tests/qemu-iotests/group
> @@ -233,3 +233,4 @@
>   233 auto quick
>   234 auto quick migration
>   235 auto quick
> +236 auto quick
> \ No newline at end of file

Umm - what's that still doing here?


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v4 1/5] iotests: add qmp recursive sorting function

[John Snow]

On 12/19/18 2:19 PM, Eric Blake wrote:
> On 12/19/18 12:57 PM, John Snow wrote:
>>
>>
>> On 12/19/18 1:52 PM, Eric Blake wrote:
>>> On 12/18/18 7:52 PM, John Snow wrote:
>>>> Python before 3.6 does not sort kwargs by default.
>>>> If we want to print out pretty-printed QMP objects while
>>>> preserving the "exec" > "arguments" ordering, we need a custom sort.
>>>
>>> Naive question - why do we need the sorting? Is it so that the output is
>>> deterministic?  Surely it can't be because the ordering otherwise makes
>>> a difference to execution.
>>>
>>
>> Yeah, it's because dicts are unordered and prior to 3.6 they may shuffle
>> around arbitrarily based on internal hashes.
>>
>> kwargs in particular are unordered- the order we send over the wire may
>> or may not reflect the order the test author wrote in their iotest.
> 
> Which shouldn't matter to what QMP executes, but MIGHT matter in getting
> reproducible log output.
> 
>>
>> Therefore, it's a way to get consistent ordering.
>>
>> Now, we CAN just rely on sort_keys=True to be done with it, however,
>> this puts arguments before execute, and it's less nice to read -- and
>> I'd have to change a LOT of test output.
> 
> Okay, I'm finally seeing it; the existing code has:
> 
>     def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
>         logmsg = '{"execute": "%s", "arguments": %s}' % \
>             (cmd, json.dumps(kwargs, sort_keys=True))
> 
> where our log is outputting a message that resembles a dict where
> "execute": is the first key, and the user's input dict is then sorted
> (the top-most output of {} is unsorted, but the nested ones are sorted,
> and possibly in a different order than the user typed them, but at least
> deterministic). But when you change to the user passing a full QMP
> command (rather than just a dict for the arguments of the QMP command),
> using sort_keys=True will sort everything _including_ putting
> "arguments" before "execute" (which is deterministic but changes log
> output); while using sort_keys=False will not affect output in newer
> python where kwargs remains ordered, but risks nondeterministic output
> on older python.
> 

Yes, and pretty-printing requires the full object, otherwise it's too
hard to manually re-indent the buffered subdict, and you have to indent
the outer dict, and...

...

It's easier to just build the full dictionary and then pretty-print it.

The motivation here is that pretty-printing a call to qmp_log should
indent both outgoing and incoming because that's semantically the most
obvious and right thing to do. Principle of least surprise.

>>
>> This way keeps the order you expect to see while maintaining a strict
>> order for the arguments. I think that's the nicest compromise until we
>> can stipulate 3.6+ in the year 2034 where kwargs *are* strictly ordered.
> 
> But kwargs strict ordering is only at the top-level, not recursive,
> right? That is, even if you can rely on:
> 
> foo(b=1, a=2)
> 
> providing kwargs where 'b' always outputs before 'a', I don't see how
> that would help:
> 
> foo(b={'d':3, 'c':4})
> 
> not reorder the keys within 'b' without your recursive ordering.
> 

Well, kwargs *are* dictionaries, and both become sorted in 3.6. You can
rely on the ordering at any level, but only in 3.6 and after.

We cannot rely on that behavior, though, so in our current reality:

(1) kwargs can be arbitrarily reordered
(2) subdictionaries in kwargs can also be arbitrarily reordered

And so my ordered_kwargs() function recursively orders any dictionaries
it finds, assuming a typical JSON structure -- which will suffice for
QMP objects.

In practice, it appears to correctly re-implement the behavior of
json.dumps(..., sort_keys=True).

--js

Re: [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[John Snow]

On 12/19/18 2:01 PM, Eric Blake wrote:
> On 12/19/18 5:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>> But still not sure that it worth it. Isn't it better to just remove
>> fields from dict,
>> which are unpredictable, instead of substituting them..
> 
> For getting the test to pass when we have a key:unpredictable value in
> the dict, you are right that both changing it to key:SUBST or removing
> key work at producing reproducible output. But when it comes to
> debugging test failure, having key:SUBST in the logs gives you a hint at
> what else to look at, whereas omitting key altogether may make the
> reason for the failure completely disappear from the logs.
> 
> Thus, I would argue that even though it is more complex to write a
> filter that can recursively substitute, the resulting output is easier
> to debug if a test starts failing - and that if the work in doing the
> more complex filtering has already been submitted and is not too much of
> a burden to maintain, then we might as well use it rather than going
> with the simpler case of just eliding the problematic keys or using just
> textual filtering.
> 
> However, I'm not in a good position to argue whether there is a
> reasonable maintenance burden with the patches in this series, vs. what
> it would take to rewrite 206 to do just textual filtering instead of QMP
> dict substitution filtering.
> 

My reasoning is this:

(1) It would be good if QMP filters behaved similarly to their plaintext
companions, as this is the least surprising behavior, and

(2) It would be best to log the keys provided in responses in case we
wish to test for their presence (and that their values match something
we were able to predict via a pattern), and

(3) Not arbitrarily changing the nature of the response invisibly in a
way that obscures it from log files to aid debugging, like you say.



I offer some ideas for how to add text filtering for QMP objects in
iotests in some of my replies, but it's not going to happen in 2018,
IMO. I want pretty-printing of QMP commands more than I want text
filtering of serialized QMP objects.

Re: [Qemu-devel] [PATCH v4 5/5] iotests: add iotest 236 for testing bitmap merge

[John Snow]

On 12/19/18 2:34 PM, Eric Blake wrote:
> On 12/18/18 7:52 PM, John Snow wrote:
>> New interface, new smoke test.
>> ---
>>   tests/qemu-iotests/236     | 131 ++++++++++++++++++++++++
>>   tests/qemu-iotests/236.out | 198 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 330 insertions(+)
>>   create mode 100755 tests/qemu-iotests/236
>>   create mode 100644 tests/qemu-iotests/236.out
>>
> 
>> +
>> +    log('')
>> +    log('--- Disabling B & Adding C ---\n')
>> +    vm.qmp_log("transaction", indent=2, actions=[
>> +        { "type": "block-dirty-bitmap-disable",
>> +          "data": { "node": "drive0", "name": "bitmapB" }},
>> +        { "type": "block-dirty-bitmap-add",
>> +          "data": { "node": "drive0", "name": "bitmapC",
>> +                    "granularity": granularity }},
>> +        # Purely extraneous, but test that it works:
>> +        { "type": "block-dirty-bitmap-disable",
>> +          "data": { "node": "drive0", "name": "bitmapC" }},
>> +        { "type": "block-dirty-bitmap-enable",
>> +          "data": { "node": "drive0", "name": "bitmapC" }},
>> +    ])
> 
> One other possible addition just before this point:
> 
> qmp_log("transaction", indent=2, actions=[
>         { "type": "block-dirty-bitmap-disable",
>           "data": { "node": "drive0", "name": "bitmapB" }},
>         { "type": "block-dirty-bitmap-add",
>           "data": { "node": "drive0", "name": "bitmapC",
>                     "granularity": granularity }},
>         { "type": "block-dirty-bitmap-remove",
>           "data": { "node": "drive0", "name": "bitmapA" }},
>         { "type": "abort", "data": {}}
>   ])
> 
> to check that we properly undo things on an aborted transaction (B
> should still be enabled, C should not exist, and A should not be damaged).
> 

Good suggestion, but remove is not a transaction item. I'll substitute
for clear, which should showcase the same effects.

>> +    # A and D should be equivalent>> +    # Some formats round the size of the disk, so don't print the
>> checksums.
>> +    check_a = vm.qmp('x-debug-block-dirty-bitmap-sha256',
>> +                     node="drive0", name="bitmapA")['return']['sha256']
>> +    check_b = vm.qmp('x-debug-block-dirty-bitmap-sha256',
>> +                     node="drive0", name="bitmapD")['return']['sha256']
>> +    assert(check_a == check_b)
> 
> Image agnostic also means that you avoid an 32- vs. 64-bit platform
> discrepancies (we've had issues in the past where the sum is different
> for some image sizes, because the bitmap is always in terms of 'long's,
> but there is one less 'long' in a 32-bit bitmap for the disk size).
> Makes sense.
> 
> Also, I don't see any tests of block-dirty-bitmap-remove - this would be
> a good point in the test to insert a final cleanup.
> 

OK, done.

> 
>> +++ b/tests/qemu-iotests/group
>> @@ -233,3 +233,4 @@
>>   233 auto quick
>>   234 auto quick migration
>>   235 auto quick
>> +236 auto quick
>> \ No newline at end of file
> 
> Umm - what's that still doing here?
> 
> 
:whistles:

Re: [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[Vladimir Sementsov-Ogievskiy]

19.12.2018 21:35, John Snow wrote:
> 
> 
> On 12/19/18 6:07 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 19.12.2018 4:52, John Snow wrote:
>>> log() treats filters as if they can always filter its primary argument.
>>> qmp_log treats filters as if they're always text.
>>>
>>> Change qmp_log to treat filters as if they're always qmp object filters,
>>> then change the logging call to rely on log()'s ability to serialize QMP
>>> objects, so we're not duplicating that effort.
>>
>> As I understand, there still no use for qmp-object based filters (even after the
>> series), do we really need them? I'm afraid it's premature complication.
>>
> 
> There are callers of log() that use qmp filters. Those callers can now
> migrate over to qmp_log to get both the call and response.
> 
> There ARE users of QMP filters.
> 
> Look at `git grep 'log(vm'` for callers that are passing rich QMP
> objects. The ones that pass filters are usually passing filter_qmp_event.

oops, I'm totally ashamed)

Interesting, about least surprising behavior, for me it's a surprise. I thought,
that assumed behavior is filtering final text..

> 
> Now, if we choose, we can move them over to using qmp_log and amend the
> logging output to get both the outgoing and returning message.
> 
> -- hmm, maybe, if you want, and I am NOT suggesting I will do this
> before the holiday break (and therefore not in this series) -- what we
> can do is this:
> 
> log(txt, filters=[]) -- Takes text and text filters only.
> 
> log_qmp(obj, tfilters=[], qfilters=[]) -- Logs a QMP object, takes QMP
> filters for pre-filtering and tfilters for post-filtering. Contains the
> json.dumps call. Simply passes tfilters down to log().
> 
> vm.qmp(log=1, tfilters=[], qfilters=[], ...) -- Perform the actual QMP
> call and response, logging the outgoing and incoming objects via log_qmp.
> 
> I can use this patchset as a starting point to do that. It will involve
> amending a lot of existing tests and test outputs, so I won't do this
> unless there appears to be some support for that API in advance.

Ohm, stop listening me, I had false assumptions. Ok, at this point, I'll move to
reviewing your next series, hope it didn't complicated due to my false criticism.

> 
>> Why not to keep all filters text based? If we need to filter some concrete fields,
>> not the whole thing, I doubt that recursion and defining functions inside
>> functions is a true way for it. Instead in concrete case, like in you test, it's
>> better to select fields that should be in output, and output only them.
>>
>>>
>>> Because kwargs have been sorted already, the order is preserved.
>>>
>>> Edit the only caller who uses filters on qmp_log to use a qmp version,
>>> also added in this patch.
>>> ---
>>>    tests/qemu-iotests/206        |  4 ++--
>>>    tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
>>>    2 files changed, 23 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
>>> index e92550fa59..5bb738bf23 100755
>>> --- a/tests/qemu-iotests/206
>>> +++ b/tests/qemu-iotests/206
>>> @@ -27,7 +27,7 @@ iotests.verify_image_format(supported_fmts=['qcow2'])
>>>    
>>>    def blockdev_create(vm, options):
>>>        result = vm.qmp_log('blockdev-create',
>>> -                        filters=[iotests.filter_testfiles],
>>> +                        filters=[iotests.filter_qmp_testfiles],
>>>                            job_id='job0', options=options)
>>>    
>>>        if 'return' in result:
>>> @@ -55,7 +55,7 @@ with iotests.FilePath('t.qcow2') as disk_path, \
>>>                              'size': 0 })
>>>    
>>>        vm.qmp_log('blockdev-add',
>>> -               filters=[iotests.filter_testfiles],
>>> +               filters=[iotests.filter_qmp_testfiles],
>>>                   driver='file', filename=disk_path,
>>>                   node_name='imgfile')
>>>    
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 55fb60e039..812302538d 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -246,10 +246,29 @@ def filter_qmp_event(event):
>>>            event['timestamp']['microseconds'] = 'USECS'
>>>        return event
>>>    
>>> +def filter_qmp(qmsg, filter_fn):
>>> +    '''Given a string filter, filter a QMP object's values.
>>> +    filter_fn takes a (key, value) pair.'''
>>> +    for key in qmsg:
>>> +        if isinstance(qmsg[key], list):
>>> +            qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
>>> +        elif isinstance(qmsg[key], dict):
>>> +            qmsg[key] = filter_qmp(qmsg[key], filter_fn)
>>> +        else:
>>> +            qmsg[key] = filter_fn(key, qmsg[key])
>>> +    return qmsg
>>> +
>>>    def filter_testfiles(msg):
>>>        prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>>>        return msg.replace(prefix, 'TEST_DIR/PID-')
>>>    
>>> +def filter_qmp_testfiles(qmsg):
>>> +    def _filter(key, value):
>>> +        if key == 'filename' or key == 'backing-file':
>>> +            return filter_testfiles(value)
>>> +        return value
>>> +    return filter_qmp(qmsg, _filter)
>>> +
>>>    def filter_generated_node_ids(msg):
>>>        return re.sub("#block[0-9]+", "NODE_NAME", msg)
>>>    
>>> @@ -462,10 +481,9 @@ class VM(qtest.QEMUQtestMachine):
>>>        def qmp_log(self, cmd, filters=[], **kwargs):
>>>            full_cmd = OrderedDict({"execute": cmd,
>>>                                    "arguments": ordered_kwargs(kwargs)})
>>> -        logmsg = json.dumps(full_cmd)
>>> -        log(logmsg, filters)
>>> +        log(full_cmd, filters)
>>>            result = self.qmp(cmd, **kwargs)
>>> -        log(json.dumps(result, sort_keys=True), filters)
>>> +        log(result, filters)
>>>            return result
>>>    
>>>        def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>>
>>
>>
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v4 3/5] iotests: change qmp_log filters to expect QMP objects only

[Vladimir Sementsov-Ogievskiy]

19.12.2018 22:52, John Snow wrote:
> 
> 
> On 12/19/18 2:01 PM, Eric Blake wrote:
>> On 12/19/18 5:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>>> But still not sure that it worth it. Isn't it better to just remove
>>> fields from dict,
>>> which are unpredictable, instead of substituting them..
>>
>> For getting the test to pass when we have a key:unpredictable value in
>> the dict, you are right that both changing it to key:SUBST or removing
>> key work at producing reproducible output. But when it comes to
>> debugging test failure, having key:SUBST in the logs gives you a hint at
>> what else to look at, whereas omitting key altogether may make the
>> reason for the failure completely disappear from the logs.
>>
>> Thus, I would argue that even though it is more complex to write a
>> filter that can recursively substitute, the resulting output is easier
>> to debug if a test starts failing - and that if the work in doing the
>> more complex filtering has already been submitted and is not too much of
>> a burden to maintain, then we might as well use it rather than going
>> with the simpler case of just eliding the problematic keys or using just
>> textual filtering.
>>
>> However, I'm not in a good position to argue whether there is a
>> reasonable maintenance burden with the patches in this series, vs. what
>> it would take to rewrite 206 to do just textual filtering instead of QMP
>> dict substitution filtering.
>>
> 
> My reasoning is this:
> 
> (1) It would be good if QMP filters behaved similarly to their plaintext
> companions, as this is the least surprising behavior, and
> 
> (2) It would be best to log the keys provided in responses in case we
> wish to test for their presence (and that their values match something
> we were able to predict via a pattern), and
> 
> (3) Not arbitrarily changing the nature of the response invisibly in a
> way that obscures it from log files to aid debugging, like you say.

Yes, at least (2-3) makes sense for me.

> 
> 
> 
> I offer some ideas for how to add text filtering for QMP objects in
> iotests in some of my replies, but it's not going to happen in 2018,
> IMO. I want pretty-printing of QMP commands more than I want text
> filtering of serialized QMP objects.
> 


-- 
Best regards,
Vladimir