bitmaps: remove x- prefix from QMP api

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.

Note that this changes the sort order for "command" and "arguments", so I restrict this reordering to occur only when the indent is specified.

---
  tests/qemu-iotests/iotests.py | 17 +++++++++++++----
  1 file changed, 13 insertions(+), 4 deletions(-)

tests/qemu-iotests/iotests.py +1 -3

index 9595429fea..ab5823c011 100644
@@ -447,12 +447,21 @@ class VM(qtest.QEMUQtestMachine):
              result.append(filter_qmp_event(ev))
          return result
 
-    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
-        logmsg = '{"execute": "%s", "arguments": %s}' % \
-            (cmd, json.dumps(kwargs, sort_keys=True))
+    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):

tests/qemu-iotests/iotests.py +2 -1

          log(logmsg, filters)
          result = self.qmp(cmd, **kwargs)
-        log(json.dumps(result, sort_keys=True), filters)
+        log(json.dumps(result, indent=indent, separators=separators,
+                       sort_keys=True), filters)
          return result
 
      def run_job(self, job, auto_finalize=True, auto_dismiss=False):

Quoted 30 lines · 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.

Note that this changes the sort order for "command" and "arguments", so I restrict this reordering to occur only when the indent is specified.
---
  tests/qemu-iotests/iotests.py | 17 +++++++++++++----

  1 file changed, 13 insertions(+), 4 deletions(-)

tests/qemu-iotests/iotests.py +0 -0

index 9595429fea..ab5823c011 100644
@@ -447,12 +447,21 @@ class VM(qtest.QEMUQtestMachine):
               result.append(filter_qmp_event(ev))

          return result
-    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
-        logmsg = '{"execute": "%s", "arguments": %s}' % \
-            (cmd, json.dumps(kwargs, sort_keys=True))
+    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
+        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
+        separators = (',', ': ') if indent is not None else (',', ': ') +        if indent is not None:
+            fullcmd = { "execute": cmd,
+                        "arguments": kwargs }
+            logmsg = json.dumps(fullcmd, indent=indent, separators=separators,

Quoted 4 lines

+                                sort_keys=True)
+        else:
+            logmsg = '{"execute": "%s", "arguments": %s}' % \
+                (cmd, json.dumps(kwargs, sort_keys=True))

definitely overlogic on None/is not None, but anyway, ',' separator leads to less beautiful output. So, I think it is better to do just "re.sub(r'\s+\n', '\n', longmsg)" on final message, here or in log(), or in both.

Quoted 9 lines

          log(logmsg, filters)
          result = self.qmp(cmd, **kwargs)
-        log(json.dumps(result, sort_keys=True), filters) +        log(json.dumps(result, indent=indent, separators=separators, +                       sort_keys=True), filters)
          return result
      def run_job(self, job, auto_finalize=True, auto_dismiss=False):

Quoted 31 lines · Vladimir Sementsov-Ogievskiy

Quoted 30 lines · 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.

Note that this changes the sort order for "command" and "arguments", so I restrict this reordering to occur only when the indent is specified.
---
  tests/qemu-iotests/iotests.py | 17 +++++++++++++----

  1 file changed, 13 insertions(+), 4 deletions(-)

tests/qemu-iotests/iotests.py +0 -0

index 9595429fea..ab5823c011 100644
@@ -447,12 +447,21 @@ class VM(qtest.QEMUQtestMachine):
               result.append(filter_qmp_event(ev))

          return result
-    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
-        logmsg = '{"execute": "%s", "arguments": %s}' % \
-            (cmd, json.dumps(kwargs, sort_keys=True))
+    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
+        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
+        separators = (',', ': ') if indent is not None else (',', ': ') +        if indent is not None:
+            fullcmd = { "execute": cmd,
+                        "arguments": kwargs }
+            logmsg = json.dumps(fullcmd, indent=indent, separators=separators,

Quoted 8 lines

Quoted 4 lines

+                                sort_keys=True)
+        else:
+            logmsg = '{"execute": "%s", "arguments": %s}' % \
+                (cmd, json.dumps(kwargs, sort_keys=True))

definitely overlogic on None/is not None, but anyway, ',' separator leads to less beautiful output. So, I think it is better to do just "re.sub(r'\s+\n', '\n', longmsg)" on final message, here or in log(), or in both.

find myself composing counter-proposal patch, will send soon...

Quoted 11 lines

Quoted 9 lines

          log(logmsg, filters)
          result = self.qmp(cmd, **kwargs)
-        log(json.dumps(result, sort_keys=True), filters) +        log(json.dumps(result, indent=indent, separators=separators, +                       sort_keys=True), filters)
          return result
      def run_job(self, job, auto_finalize=True, auto_dismiss=False):

Quoted 32 lines · Vladimir Sementsov-Ogievskiy

Quoted 31 lines · Vladimir Sementsov-Ogievskiy

Quoted 30 lines · 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.

Note that this changes the sort order for "command" and "arguments", so I restrict this reordering to occur only when the indent is specified.

---
   tests/qemu-iotests/iotests.py | 17 +++++++++++++----
   1 file changed, 13 insertions(+), 4 deletions(-)

tests/qemu-iotests/iotests.py +7 -3

index 9595429fea..ab5823c011 100644
@@ -447,12 +447,21 @@ class VM(qtest.QEMUQtestMachine):
               result.append(filter_qmp_event(ev))
           return result
-    def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
-        logmsg = '{"execute": "%s", "arguments": %s}' % \
-            (cmd, json.dumps(kwargs, sort_keys=True))
+    def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
+        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
+        separators = (',', ': ') if indent is not None else (',', ': ')
+        if indent is not None:
+            fullcmd = { "execute": cmd,
+                        "arguments": kwargs }
+            logmsg = json.dumps(fullcmd, indent=indent, separators=separators,

Quoted 8 lines

Quoted 8 lines

tests/qemu-iotests/iotests.py class VM(qtest.QEMUQtestMachine): · line 460

Quoted 4 lines

tests/qemu-iotests/iotests.py +4 -0

+                                sort_keys=True)
+        else:
+            logmsg = '{"execute": "%s", "arguments": %s}' % \
+                (cmd, json.dumps(kwargs, sort_keys=True))

definitely overlogic on None/is not None, but anyway, ',' separator leads to less beautiful output. So, I think it is better to do just "re.sub(r'\s+\n', '\n', longmsg)" on final message, here or in log(), or in both.

I can't use "if indent" because 0 is a valid indentation, and I decided to keep the "correct" order for execute/arguments if no indentation is requested.

How is ',' less beautiful? When pretty-printing output ',' only occurs at the end of a line anyway. I don't understand the critique.

Quoted 17 lines

find myself composing counter-proposal patch, will send soon...

Quoted 11 lines

Quoted 9 lines

tests/qemu-iotests/iotests.py +2 -1

           log(logmsg, filters)
           result = self.qmp(cmd, **kwargs)
-        log(json.dumps(result, sort_keys=True), filters)
+        log(json.dumps(result, indent=indent, separators=separators,
+                       sort_keys=True), filters)
           return result
       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
 

New interface, new smoke test.

---
  tests/qemu-iotests/236     | 124 +++++++++++++++++++++++
  tests/qemu-iotests/236.out | 200 +++++++++++++++++++++++++++++++++++++
  tests/qemu-iotests/group   |   1 +
  3 files changed, 325 insertions(+)

create mode 100755 tests/qemu-iotests/236
create mode 100644 tests/qemu-iotests/236.out

tests/qemu-iotests/236 new file +17 -0

new file mode 100755
index 0000000000..edf16c4ab1
@@ -0,0 +1,124 @@
+#!/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

tests/qemu-iotests/236 new file +10 -0

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

tests/qemu-iotests/236 new file +6 -0

+    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")
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB")

Quoted 32 lines · John Snow

New interface, new smoke test.

---
  tests/qemu-iotests/236     | 124 +++++++++++++++++++++++
  tests/qemu-iotests/236.out | 200 +++++++++++++++++++++++++++++++++++++
  tests/qemu-iotests/group   |   1 +
  3 files changed, 325 insertions(+)

create mode 100755 tests/qemu-iotests/236
create mode 100644 tests/qemu-iotests/236.out

tests/qemu-iotests/236 new file +17 -0

new file mode 100755
index 0000000000..edf16c4ab1
@@ -0,0 +1,124 @@
+#!/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

tests/qemu-iotests/236 line 27

Quoted 10 lines

tests/qemu-iotests/236 new file +10 -0

+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# [email protected]
+
+import json
+import iotests
+from iotests import log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+

we hardcode patterns, so it's better to hardcode size here too:

size = 64 * 1024 * 1024

Quoted 16 lines

tests/qemu-iotests/236 new file +15 -0

+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']}
+

tests/qemu-iotests/236 line 47

Quoted 5 lines

tests/qemu-iotests/236 new file +5 -0

+with iotests.FilePath('img') as img_path, \
+     iotests.VM() as vm:
+
+    log('--- Preparing image & VM ---\n')
+    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')

no needs for qemu_img_pipe, as you don't use its output, qemu_img() is enough and qemu_img_create() is better, and the best I think is just:

vm.add_drive_raw('id=drive0,driver=null-co,size=' + str(size))

as qcow2 (and real disk in general) is actually unrelated to the test.

Quoted 11 lines

tests/qemu-iotests/236 new file +10 -0

+    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")
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB")
+
+    # Dirties 4 clusters. count=262144
+    log('')
+    log('--- Emulating writes ---\n')

tests/qemu-iotests/236 line 63

Quoted 6 lines

tests/qemu-iotests/236 new file +6 -0

+    for p in patterns:
+        cmd = "write -P%s %s %s" % p
+        log(cmd)
+        log(vm.hmp_qemu_io("drive0", cmd))
+
+    log(json.dumps(query_bitmaps(vm), indent=2, separators=(',', ': ')))

log can do json.dumps, so it's strange to do it here, and I don't like ',' separator, as I described

Quoted 46 lines · Vladimir Sementsov-Ogievskiy

Quoted 32 lines · John Snow

New interface, new smoke test.

---
   tests/qemu-iotests/236     | 124 +++++++++++++++++++++++
   tests/qemu-iotests/236.out | 200 +++++++++++++++++++++++++++++++++++++
   tests/qemu-iotests/group   |   1 +
   3 files changed, 325 insertions(+)

create mode 100755 tests/qemu-iotests/236
create mode 100644 tests/qemu-iotests/236.out

tests/qemu-iotests/236 new file +17 -0

new file mode 100755
index 0000000000..edf16c4ab1
@@ -0,0 +1,124 @@
+#!/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

tests/qemu-iotests/236 line 27

Quoted 10 lines

tests/qemu-iotests/236 new file +10 -0

+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# [email protected]
+
+import json
+import iotests
+from iotests import log
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+

we hardcode patterns, so it's better to hardcode size here too:

Quoted 1 line

size = 64 * 1024 * 1024

If you insist.

Quoted 16 lines

Quoted 16 lines

tests/qemu-iotests/236 new file +15 -0

+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']}
+

Quoted 9 lines

tests/qemu-iotests/236 line 47

Quoted 5 lines

tests/qemu-iotests/236 new file +5 -0

+with iotests.FilePath('img') as img_path, \
+     iotests.VM() as vm:
+
+    log('--- Preparing image & VM ---\n')
+    iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')

no needs for qemu_img_pipe, as you don't use its output, qemu_img() is enough and qemu_img_create() is better, and the best I think is just:

More cargo cult copy and paste.

Quoted 4 lines

vm.add_drive_raw('id=drive0,driver=null-co,size=' + str(size))

as qcow2 (and real disk in general) is actually unrelated to the test.

I think I'll leave the image creation in just so that if you run the test under different formats it'll actually do something vaguely different, just in case.

Actually, it did highlight how weird VPC is again and I changed the rest as a result to accommodate image formats that round their disk sizes.

Quoted 11 lines

Quoted 11 lines

tests/qemu-iotests/236 new file +10 -0

+    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")
+    vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB")
+
+    # Dirties 4 clusters. count=262144
+    log('')
+    log('--- Emulating writes ---\n')

Quoted 8 lines

tests/qemu-iotests/236 line 63

Quoted 6 lines

tests/qemu-iotests/236 new file +6 -0

+    for p in patterns:
+        cmd = "write -P%s %s %s" % p
+        log(cmd)
+        log(vm.hmp_qemu_io("drive0", cmd))
+
+    log(json.dumps(query_bitmaps(vm), indent=2, separators=(',', ': ')))

log can do json.dumps, so it's strange to do it here, and I don't like ',' separator, as I described

Because it tries to filter the rich object before it converts, as you've noticed. I think I'll take a page from your book and try to fix log() to be better.

As for not liking the ',' separator, it should be identical to your approach because it only removes the space when pretty printing is enabled. Can you show me what this approach does that you find to be ugly and how it's different from your regex approach?

--js

New interface, new smoke test.

To mimic the common filter of the same name, but for the python tests.

Signed-off-by John Snow <[email protected].

Reviewed-by Eric Blake <[email protected]>

Reviewed-by Vladimir Sementsov-Ogievskiy <[email protected]>

---
  tests/qemu-iotests/iotests.py | 3 +++
  1 file changed, 3 insertions(+)

tests/qemu-iotests/iotests.py +3 -0

index a34e66813a..9595429fea 100644
@@ -239,6 +239,9 @@ def filter_testfiles(msg):
      prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
      return msg.replace(prefix, 'TEST_DIR/PID-')
 
+def filter_generated_node_ids(msg):
+    return re.sub("#block[0-9]+", "NODE_NAME", msg)
+
  def filter_img_info(output, filename):
      lines = []
      for line in output.split('\n'):

Touch up a few last things and remove the x- prefix.

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]

Note that the filter I added is now unused, but I think we will want it and it's small enough, so I'm going to check it in anyway. If you disagree, I'll just drop the patch instead.

--js

John Snow (7):
blockdev: abort transactions in reverse order
blockdev: n-ary bitmap merge
block: remove 'x' prefix from experimental bitmap APIs iotests.py: don't abort if IMGKEYSECRET is undefined
iotests: add filter_generated_node_ids
iotests: allow pretty-print for qmp_log
iotests: add iotest 236 for testing bitmap merge

 blockdev.c                    |  96 +++++++++-------
 qapi/block-core.json          |  56 +++++-----
 qapi/transaction.json         |  12 +-
 tests/qemu-iotests/223        |   4 +-
 tests/qemu-iotests/236        | 124 +++++++++++++++++++++
 tests/qemu-iotests/236.out    | 200 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  22 +++-
 8 files changed, 436 insertions(+), 79 deletions(-)
 create mode 100755 tests/qemu-iotests/236
 create mode 100644 tests/qemu-iotests/236.out

Quoted 22 lines · John Snow

Touch up a few last things and remove the x- prefix.

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]

Note that the filter I added is now unused, but I think we will want it and it's small enough, so I'm going to check it in anyway. If you disagree, I'll just drop the patch instead.

--js

John Snow (7):
blockdev: abort transactions in reverse order
blockdev: n-ary bitmap merge
block: remove 'x' prefix from experimental bitmap APIs iotests.py: don't abort if IMGKEYSECRET is undefined
iotests: add filter_generated_node_ids
iotests: allow pretty-print for qmp_log
iotests: add iotest 236 for testing bitmap merge

Quoted 12 lines

 blockdev.c                    |  96 +++++++++-------
 qapi/block-core.json          |  56 +++++-----
 qapi/transaction.json         |  12 +-
 tests/qemu-iotests/223        |   4 +-
 tests/qemu-iotests/236        | 124 +++++++++++++++++++++
 tests/qemu-iotests/236.out    | 200 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  22 +++-
 8 files changed, 436 insertions(+), 79 deletions(-)
 create mode 100755 tests/qemu-iotests/236
 create mode 100644 tests/qemu-iotests/236.out

Thanks, I'm staging patches 1-5 and I'll send the PR once we get to the bottom of patches 6 and 7, just to keep volume on the list down.