[Qemu-devel] [PATCH v2 0/7] bitmaps: remove x- prefix from QMP api

[Qemu-devel] [PATCH v2 0/7] bitmaps: remove x- prefix from QMP api

[John Snow]

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

V2:
 - Adding R-Bs from Eric and Vladimir.
 - A few trivial patches for iotests...
 - Added quick smoke test.

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        | 123 ++++++++++++++++
 tests/qemu-iotests/236.out    | 265 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |   9 +-
 8 files changed, 489 insertions(+), 77 deletions(-)
 create mode 100755 tests/qemu-iotests/236
 create mode 100644 tests/qemu-iotests/236.out

-- 
2.17.2

[Qemu-devel] [PATCH v2 1/7] blockdev: abort transactions in reverse order

[John Snow]

Presently, we abort transactions in the same order they were processed in.
Bitmap commands, though, attempt to restore backup data structures on abort.
To that end, though, they need to be aborted in reverse chronological order.

Replace the QSIMPLEQ data structure with a QTAILQ one, so we can iterate
in reverse for the abort phase of the transaction.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 blockdev.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 81f95d920b..1ba706df8b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1341,7 +1341,7 @@ struct BlkActionState {
     const BlkActionOps *ops;
     JobTxn *block_job_txn;
     TransactionProperties *txn_props;
-    QSIMPLEQ_ENTRY(BlkActionState) entry;
+    QTAILQ_ENTRY(BlkActionState) entry;
 };
 
 /* internal snapshot private data */
@@ -2269,8 +2269,8 @@ void qmp_transaction(TransactionActionList *dev_list,
     BlkActionState *state, *next;
     Error *local_err = NULL;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
-    QSIMPLEQ_INIT(&snap_bdrv_states);
+    QTAILQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
+    QTAILQ_INIT(&snap_bdrv_states);
 
     /* Does this transaction get canceled as a group on failure?
      * If not, we don't really need to make a JobTxn.
@@ -2301,7 +2301,7 @@ void qmp_transaction(TransactionActionList *dev_list,
         state->action = dev_info;
         state->block_job_txn = block_job_txn;
         state->txn_props = props;
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
+        QTAILQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
 
         state->ops->prepare(state, &local_err);
         if (local_err) {
@@ -2310,7 +2310,7 @@ void qmp_transaction(TransactionActionList *dev_list,
         }
     }
 
-    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+    QTAILQ_FOREACH(state, &snap_bdrv_states, entry) {
         if (state->ops->commit) {
             state->ops->commit(state);
         }
@@ -2321,13 +2321,13 @@ void qmp_transaction(TransactionActionList *dev_list,
 
 delete_and_fail:
     /* failure, and it is all-or-none; roll back all operations */
-    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+    QTAILQ_FOREACH_REVERSE(state, &snap_bdrv_states, snap_bdrv_states, entry) {
         if (state->ops->abort) {
             state->ops->abort(state);
         }
     }
 exit:
-    QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
+    QTAILQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
         if (state->ops->clean) {
             state->ops->clean(state);
         }
-- 
2.17.2

[Qemu-devel] [PATCH v2 2/7] blockdev: n-ary bitmap merge

[John Snow]

Especially outside of transactions, it is helpful to provide
all-or-nothing semantics for bitmap merges. This facilitates
the coalescing of multiple bitmaps into a single target for
the "checkpoint" interpretation when assembling bitmaps that
represent arbitrary points in time from component bitmaps.

This is an incompatible change from the preliminary version
of the API.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c           | 64 +++++++++++++++++++++++++++++---------------
 qapi/block-core.json | 22 +++++++--------
 2 files changed, 53 insertions(+), 33 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1ba706df8b..0f740fd964 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2122,33 +2122,26 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
     }
 }
 
+void do_block_dirty_bitmap_merge(const char *node, const char *target,
+                                 strList *bitmaps, HBitmap **backup,
+                                 Error **errp);
+
 static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
                                              Error **errp)
 {
     BlockDirtyBitmapMerge *action;
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);
-    BdrvDirtyBitmap *merge_source;
 
     if (action_check_completion_mode(common, errp) < 0) {
         return;
     }
 
     action = common->action->u.x_block_dirty_bitmap_merge.data;
-    state->bitmap = block_dirty_bitmap_lookup(action->node,
-                                              action->dst_name,
-                                              &state->bs,
-                                              errp);
-    if (!state->bitmap) {
-        return;
-    }
 
-    merge_source = bdrv_find_dirty_bitmap(state->bs, action->src_name);
-    if (!merge_source) {
-        return;
-    }
-
-    bdrv_merge_dirty_bitmap(state->bitmap, merge_source, &state->backup, errp);
+    do_block_dirty_bitmap_merge(action->node, action->target,
+                                action->bitmaps, &state->backup,
+                                errp);
 }
 
 static void abort_prepare(BlkActionState *common, Error **errp)
@@ -2980,24 +2973,51 @@ void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name,
     bdrv_disable_dirty_bitmap(bitmap);
 }
 
-void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name,
-                                    const char *src_name, Error **errp)
+void do_block_dirty_bitmap_merge(const char *node, const char *target,
+                                 strList *bitmaps, HBitmap **backup,
+                                 Error **errp)
 {
     BlockDriverState *bs;
-    BdrvDirtyBitmap *dst, *src;
+    BdrvDirtyBitmap *dst, *src, *anon;
+    strList *lst;
+    Error *local_err = NULL;
 
-    dst = block_dirty_bitmap_lookup(node, dst_name, &bs, errp);
+    dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
     if (!dst) {
         return;
     }
 
-    src = bdrv_find_dirty_bitmap(bs, src_name);
-    if (!src) {
-        error_setg(errp, "Dirty bitmap '%s' not found", src_name);
+    anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
+                                    NULL, errp);
+    if (!anon) {
         return;
     }
 
-    bdrv_merge_dirty_bitmap(dst, src, NULL, errp);
+    for (lst = bitmaps; lst; lst = lst->next) {
+        src = bdrv_find_dirty_bitmap(bs, lst->value);
+        if (!src) {
+            error_setg(errp, "Dirty bitmap '%s' not found", lst->value);
+            goto out;
+        }
+
+        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            goto out;
+        }
+    }
+
+    /* Merge into dst; dst is unchanged on failure. */
+    bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
+
+ out:
+    bdrv_release_dirty_bitmap(bs, anon);
+}
+
+void qmp_x_block_dirty_bitmap_merge(const char *node, const char *target,
+                                    strList *bitmaps, Error **errp)
+{
+    do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
 }
 
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d4fe710836..320d74ef34 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1818,14 +1818,14 @@
 #
 # @node: name of device/node which the bitmap is tracking
 #
-# @dst_name: name of the destination dirty bitmap
+# @target: name of the destination dirty bitmap
 #
-# @src_name: name of the source dirty bitmap
+# @bitmaps: name(s) of the source dirty bitmap(s)
 #
 # Since: 3.0
 ##
 { 'struct': 'BlockDirtyBitmapMerge',
-  'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } }
+  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
 
 ##
 # @block-dirty-bitmap-add:
@@ -1940,23 +1940,23 @@
 ##
 # @x-block-dirty-bitmap-merge:
 #
-# FIXME: Rename @src_name and @dst_name to src-name and dst-name.
-#
-# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty
-# bitmap is unchanged. On error, @dst_name is unchanged.
+# Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
+# The @bitmaps dirty bitmaps are unchanged.
+# On error, @target is unchanged.
 #
 # Returns: nothing on success
 #          If @node is not a valid block device, DeviceNotFound
-#          If @dst_name or @src_name is not found, GenericError
-#          If bitmaps has different sizes or granularities, GenericError
+#          If any bitmap in @bitmaps or @target is not found, GenericError
+#          If any of the bitmaps have different sizes or granularities,
+#              GenericError
 #
 # Since: 3.0
 #
 # Example:
 #
 # -> { "execute": "x-block-dirty-bitmap-merge",
-#      "arguments": { "node": "drive0", "dst_name": "bitmap0",
-#                     "src_name": "bitmap1" } }
+#      "arguments": { "node": "drive0", "target": "bitmap0",
+#                     "bitmaps": ["bitmap1"] } }
 # <- { "return": {} }
 #
 ##
-- 
2.17.2

[Qemu-devel] [PATCH v2 3/7] block: remove 'x' prefix from experimental bitmap APIs

[John Snow]

The 'x' prefix was added because I was uncertain of the direction we'd
take for the libvirt API. With the general approach solidified, I feel
comfortable committing to this API for 4.0.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c             | 22 +++++++++++-----------
 qapi/block-core.json   | 34 +++++++++++++++++-----------------
 qapi/transaction.json  | 12 ++++++------
 tests/qemu-iotests/223 |  4 ++--
 4 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0f740fd964..da87aae5cf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1966,7 +1966,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common,
                                action->has_granularity, action->granularity,
                                action->has_persistent, action->persistent,
                                action->has_autoload, action->autoload,
-                               action->has_x_disabled, action->x_disabled,
+                               action->has_disabled, action->disabled,
                                &local_err);
 
     if (!local_err) {
@@ -2051,7 +2051,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
         return;
     }
 
-    action = common->action->u.x_block_dirty_bitmap_enable.data;
+    action = common->action->u.block_dirty_bitmap_enable.data;
     state->bitmap = block_dirty_bitmap_lookup(action->node,
                                               action->name,
                                               NULL,
@@ -2092,7 +2092,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,
         return;
     }
 
-    action = common->action->u.x_block_dirty_bitmap_disable.data;
+    action = common->action->u.block_dirty_bitmap_disable.data;
     state->bitmap = block_dirty_bitmap_lookup(action->node,
                                               action->name,
                                               NULL,
@@ -2137,7 +2137,7 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
         return;
     }
 
-    action = common->action->u.x_block_dirty_bitmap_merge.data;
+    action = common->action->u.block_dirty_bitmap_merge.data;
 
     do_block_dirty_bitmap_merge(action->node, action->target,
                                 action->bitmaps, &state->backup,
@@ -2205,17 +2205,17 @@ static const BlkActionOps actions[] = {
         .commit = block_dirty_bitmap_free_backup,
         .abort = block_dirty_bitmap_restore,
     },
-    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_ENABLE] = {
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_enable_prepare,
         .abort = block_dirty_bitmap_enable_abort,
     },
-    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_DISABLE] = {
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_disable_prepare,
         .abort = block_dirty_bitmap_disable_abort,
     },
-    [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = {
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_MERGE] = {
         .instance_size = sizeof(BlockDirtyBitmapState),
         .prepare = block_dirty_bitmap_merge_prepare,
         .commit = block_dirty_bitmap_free_backup,
@@ -2931,7 +2931,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,
     bdrv_clear_dirty_bitmap(bitmap, NULL);
 }
 
-void qmp_x_block_dirty_bitmap_enable(const char *node, const char *name,
+void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
                                    Error **errp)
 {
     BlockDriverState *bs;
@@ -2952,7 +2952,7 @@ void qmp_x_block_dirty_bitmap_enable(const char *node, const char *name,
     bdrv_enable_dirty_bitmap(bitmap);
 }
 
-void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name,
+void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
                                     Error **errp)
 {
     BlockDriverState *bs;
@@ -3014,8 +3014,8 @@ void do_block_dirty_bitmap_merge(const char *node, const char *target,
     bdrv_release_dirty_bitmap(bs, anon);
 }
 
-void qmp_x_block_dirty_bitmap_merge(const char *node, const char *target,
-                                    strList *bitmaps, Error **errp)
+void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
+                                  strList *bitmaps, Error **errp)
 {
     do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 320d74ef34..fde96fdb50 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1803,15 +1803,15 @@
 #            Currently, all dirty tracking bitmaps are loaded from Qcow2 on
 #            open.
 #
-# @x-disabled: the bitmap is created in the disabled state, which means that
-#              it will not track drive changes. The bitmap may be enabled with
-#              x-block-dirty-bitmap-enable. Default is false. (Since: 3.0)
+# @disabled: the bitmap is created in the disabled state, which means that
+#            it will not track drive changes. The bitmap may be enabled with
+#            block-dirty-bitmap-enable. Default is false. (Since: 4.0)
 #
 # Since: 2.4
 ##
 { 'struct': 'BlockDirtyBitmapAdd',
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
-            '*persistent': 'bool', '*autoload': 'bool', '*x-disabled': 'bool' } }
+            '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } }
 
 ##
 # @BlockDirtyBitmapMerge:
@@ -1822,7 +1822,7 @@
 #
 # @bitmaps: name(s) of the source dirty bitmap(s)
 #
-# Since: 3.0
+# Since: 4.0
 ##
 { 'struct': 'BlockDirtyBitmapMerge',
   'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
@@ -1896,7 +1896,7 @@
   'data': 'BlockDirtyBitmap' }
 
 ##
-# @x-block-dirty-bitmap-enable:
+# @block-dirty-bitmap-enable:
 #
 # Enables a dirty bitmap so that it will begin tracking disk changes.
 #
@@ -1904,20 +1904,20 @@
 #          If @node is not a valid block device, DeviceNotFound
 #          If @name is not found, GenericError with an explanation
 #
-# Since: 3.0
+# Since: 4.0
 #
 # Example:
 #
-# -> { "execute": "x-block-dirty-bitmap-enable",
+# -> { "execute": "block-dirty-bitmap-enable",
 #      "arguments": { "node": "drive0", "name": "bitmap0" } }
 # <- { "return": {} }
 #
 ##
-  { 'command': 'x-block-dirty-bitmap-enable',
+  { 'command': 'block-dirty-bitmap-enable',
     'data': 'BlockDirtyBitmap' }
 
 ##
-# @x-block-dirty-bitmap-disable:
+# @block-dirty-bitmap-disable:
 #
 # Disables a dirty bitmap so that it will stop tracking disk changes.
 #
@@ -1925,20 +1925,20 @@
 #          If @node is not a valid block device, DeviceNotFound
 #          If @name is not found, GenericError with an explanation
 #
-# Since: 3.0
+# Since: 4.0
 #
 # Example:
 #
-# -> { "execute": "x-block-dirty-bitmap-disable",
+# -> { "execute": "block-dirty-bitmap-disable",
 #      "arguments": { "node": "drive0", "name": "bitmap0" } }
 # <- { "return": {} }
 #
 ##
-    { 'command': 'x-block-dirty-bitmap-disable',
+    { 'command': 'block-dirty-bitmap-disable',
       'data': 'BlockDirtyBitmap' }
 
 ##
-# @x-block-dirty-bitmap-merge:
+# @block-dirty-bitmap-merge:
 #
 # Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
 # The @bitmaps dirty bitmaps are unchanged.
@@ -1950,17 +1950,17 @@
 #          If any of the bitmaps have different sizes or granularities,
 #              GenericError
 #
-# Since: 3.0
+# Since: 4.0
 #
 # Example:
 #
-# -> { "execute": "x-block-dirty-bitmap-merge",
+# -> { "execute": "block-dirty-bitmap-merge",
 #      "arguments": { "node": "drive0", "target": "bitmap0",
 #                     "bitmaps": ["bitmap1"] } }
 # <- { "return": {} }
 #
 ##
-      { 'command': 'x-block-dirty-bitmap-merge',
+      { 'command': 'block-dirty-bitmap-merge',
         'data': 'BlockDirtyBitmapMerge' }
 
 ##
diff --git a/qapi/transaction.json b/qapi/transaction.json
index 5875cdb16c..95edb78227 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -46,9 +46,9 @@
 # - @abort: since 1.6
 # - @block-dirty-bitmap-add: since 2.5
 # - @block-dirty-bitmap-clear: since 2.5
-# - @x-block-dirty-bitmap-enable: since 3.0
-# - @x-block-dirty-bitmap-disable: since 3.0
-# - @x-block-dirty-bitmap-merge: since 3.1
+# - @block-dirty-bitmap-enable: since 4.0
+# - @block-dirty-bitmap-disable: since 4.0
+# - @block-dirty-bitmap-merge: since 4.0
 # - @blockdev-backup: since 2.3
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
@@ -62,9 +62,9 @@
        'abort': 'Abort',
        'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
        'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
-       'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap',
-       'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap',
-       'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
+       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
        'blockdev-backup': 'BlockdevBackup',
        'blockdev-snapshot': 'BlockdevSnapshot',
        'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223
index 397b865d34..5513dc6215 100755
--- a/tests/qemu-iotests/223
+++ b/tests/qemu-iotests/223
@@ -112,9 +112,9 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add",
   "arguments":{"driver":"qcow2", "node-name":"n",
     "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
   "arguments":{"node":"n", "name":"b"}}' "return"
-_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-block-dirty-bitmap-disable",
+_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable",
   "arguments":{"node":"n", "name":"b2"}}' "return"
 _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start",
   "arguments":{"addr":{"type":"unix",
-- 
2.17.2

[Qemu-devel] [PATCH v2 4/7] iotests.py: don't abort if IMGKEYSECRET is undefined

[John Snow]

Instead of using os.environ[], use .get with a default of empty string
to match the setup in check to allow us to import the iotests module
(for debugging, say) without needing a crafted environment just to
import the module.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index d537538ba0..a34e66813a 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -63,7 +63,7 @@ socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
 debug = False
 
 luks_default_secret_object = 'secret,id=keysec0,data=' + \
-                             os.environ['IMGKEYSECRET']
+                             os.environ.get('IMGKEYSECRET', '')
 luks_default_key_secret_opt = 'key-secret=keysec0'
 
 
-- 
2.17.2

[Qemu-devel] [PATCH v2 5/7] iotests: add filter_generated_node_ids

[John Snow]

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

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index a34e66813a..9595429fea 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -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'):
-- 
2.17.2

[Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for 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.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9595429fea..dbbef4bad3 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -447,12 +447,12 @@ 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, indent=None, filters=[filter_testfiles], **kwargs):
         logmsg = '{"execute": "%s", "arguments": %s}' % \
             (cmd, json.dumps(kwargs, sort_keys=True))
         log(logmsg, filters)
         result = self.qmp(cmd, **kwargs)
-        log(json.dumps(result, sort_keys=True), filters)
+        log(json.dumps(result, indent=indent, sort_keys=True), filters)
         return result
 
     def run_job(self, job, auto_finalize=True, auto_dismiss=False):
-- 
2.17.2

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

[John Snow]

New interface, new smoke test.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/236     | 123 +++++++++++++++++
 tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 389 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..3d162e967b
--- /dev/null
+++ b/tests/qemu-iotests/236
@@ -0,0 +1,123 @@
+#!/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 sys
+import os
+import iotests
+from iotests import log
+sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
+from qemu import QEMUMachine
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+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)
+
+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')
+    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')
+    for p in patterns:
+        cmd = "write -P%s %s %s" % p
+        log(cmd)
+        log(vm.hmp_qemu_io("drive0", cmd))
+
+    vm.qmp_log("query-block", indent=2,
+               filters=[iotests.filter_generated_node_ids,
+                        iotests.filter_testfiles])
+
+    log('')
+    log('--- Disabling B & Adding C ---\n')
+    vm.qmp_log("transaction", actions=[
+        { "type": "block-dirty-bitmap-disable",
+          "data": { "node": "drive0", "name": "bitmapB" }},
+        { "type": "block-dirty-bitmap-add",
+          "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", 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
+    vm.qmp_log("query-block", indent=2,
+               filters=[iotests.filter_generated_node_ids,
+                        iotests.filter_testfiles])
+
+    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", actions=[
+        { "type": "block-dirty-bitmap-add",
+          "data": { "node": "drive0", "name": "bitmapD", "disabled": True }},
+        { "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.
+    vm.qmp_log("query-block", indent=2,
+               filters=[iotests.filter_generated_node_ids,
+                        iotests.filter_testfiles])
+
+    # A and D should be equivalent.
+    vm.qmp_log('x-debug-block-dirty-bitmap-sha256',
+               node="drive0", name="bitmapA")
+    vm.qmp_log('x-debug-block-dirty-bitmap-sha256',
+               node="drive0", name="bitmapD")
+
+    vm.shutdown()
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
new file mode 100644
index 0000000000..5e4d1fe6d5
--- /dev/null
+++ b/tests/qemu-iotests/236.out
@@ -0,0 +1,265 @@
+--- Preparing image & VM ---
+
+--- Adding preliminary bitmaps A & B ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmapA", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"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": ""}
+{"execute": "query-block", "arguments": {}}
+{
+  "return": [
+    {
+      "device": "drive0",
+      "dirty-bitmaps": [
+        {
+          "count": 262144,
+          "granularity": 65536,
+          "name": "bitmapB",
+          "status": "active"
+        },
+        {
+          "count": 262144,
+          "granularity": 65536,
+          "name": "bitmapA",
+          "status": "active"
+        }
+      ],
+      "inserted": {
+        "backing_file_depth": 0,
+        "bps": 0,
+        "bps_rd": 0,
+        "bps_wr": 0,
+        "cache": {
+          "direct": false,
+          "no-flush": false,
+          "writeback": true
+        },
+        "detect_zeroes": "off",
+        "drv": "qcow2",
+        "encrypted": false,
+        "encryption_key_missing": false,
+        "file": "TEST_DIR/PID-img",
+        "image": {
+          "actual-size": 528384,
+          "cluster-size": 65536,
+          "dirty-flag": false,
+          "filename": "TEST_DIR/PID-img",
+          "format": "qcow2",
+          "format-specific": {
+            "data": {
+              "compat": "1.1",
+              "corrupt": false,
+              "lazy-refcounts": false,
+              "refcount-bits": 16
+            },
+            "type": "qcow2"
+          },
+          "virtual-size": 67108864
+        },
+        "iops": 0,
+        "iops_rd": 0,
+        "iops_wr": 0,
+        "node-name": "NODE_NAME",
+        "ro": false,
+        "write_threshold": 0
+      },
+      "io-status": "ok",
+      "locked": false,
+      "qdev": "/machine/peripheral-anon/device[0]/virtio-backend",
+      "removable": false,
+      "type": "unknown"
+    }
+  ]
+}
+
+--- Disabling B & Adding C ---
+
+{"execute": "transaction", "arguments": {"actions": [{"data": {"name": "bitmapB", "node": "drive0"}, "type": "block-dirty-bitmap-disable"}, {"data": {"name": "bitmapC", "node": "drive0"}, "type": "block-dirty-bitmap-add"}]}}
+{"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": {"name": "bitmapA", "node": "drive0"}, "type": "block-dirty-bitmap-disable"}, {"data": {"name": "bitmapC", "node": "drive0"}, "type": "block-dirty-bitmap-disable"}]}}
+{"return": {}}
+{"execute": "query-block", "arguments": {}}
+{
+  "return": [
+    {
+      "device": "drive0",
+      "dirty-bitmaps": [
+        {
+          "count": 393216,
+          "granularity": 65536,
+          "name": "bitmapC",
+          "status": "disabled"
+        },
+        {
+          "count": 262144,
+          "granularity": 65536,
+          "name": "bitmapB",
+          "status": "disabled"
+        },
+        {
+          "count": 458752,
+          "granularity": 65536,
+          "name": "bitmapA",
+          "status": "disabled"
+        }
+      ],
+      "inserted": {
+        "backing_file_depth": 0,
+        "bps": 0,
+        "bps_rd": 0,
+        "bps_wr": 0,
+        "cache": {
+          "direct": false,
+          "no-flush": false,
+          "writeback": true
+        },
+        "detect_zeroes": "off",
+        "drv": "qcow2",
+        "encrypted": false,
+        "encryption_key_missing": false,
+        "file": "TEST_DIR/PID-img",
+        "image": {
+          "actual-size": 724992,
+          "cluster-size": 65536,
+          "dirty-flag": false,
+          "filename": "TEST_DIR/PID-img",
+          "format": "qcow2",
+          "format-specific": {
+            "data": {
+              "compat": "1.1",
+              "corrupt": false,
+              "lazy-refcounts": false,
+              "refcount-bits": 16
+            },
+            "type": "qcow2"
+          },
+          "virtual-size": 67108864
+        },
+        "iops": 0,
+        "iops_rd": 0,
+        "iops_wr": 0,
+        "node-name": "NODE_NAME",
+        "ro": false,
+        "write_threshold": 0
+      },
+      "io-status": "ok",
+      "locked": false,
+      "qdev": "/machine/peripheral-anon/device[0]/virtio-backend",
+      "removable": false,
+      "type": "unknown"
+    }
+  ]
+}
+
+--- Creating D as a merge of B & C ---
+
+{"execute": "transaction", "arguments": {"actions": [{"data": {"disabled": true, "name": "bitmapD", "node": "drive0"}, "type": "block-dirty-bitmap-add"}, {"data": {"bitmaps": ["bitmapB", "bitmapC"], "node": "drive0", "target": "bitmapD"}, "type": "block-dirty-bitmap-merge"}]}}
+{"return": {}}
+{"execute": "query-block", "arguments": {}}
+{
+  "return": [
+    {
+      "device": "drive0",
+      "dirty-bitmaps": [
+        {
+          "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"
+        }
+      ],
+      "inserted": {
+        "backing_file_depth": 0,
+        "bps": 0,
+        "bps_rd": 0,
+        "bps_wr": 0,
+        "cache": {
+          "direct": false,
+          "no-flush": false,
+          "writeback": true
+        },
+        "detect_zeroes": "off",
+        "drv": "qcow2",
+        "encrypted": false,
+        "encryption_key_missing": false,
+        "file": "TEST_DIR/PID-img",
+        "image": {
+          "actual-size": 724992,
+          "cluster-size": 65536,
+          "dirty-flag": false,
+          "filename": "TEST_DIR/PID-img",
+          "format": "qcow2",
+          "format-specific": {
+            "data": {
+              "compat": "1.1",
+              "corrupt": false,
+              "lazy-refcounts": false,
+              "refcount-bits": 16
+            },
+            "type": "qcow2"
+          },
+          "virtual-size": 67108864
+        },
+        "iops": 0,
+        "iops_rd": 0,
+        "iops_wr": 0,
+        "node-name": "NODE_NAME",
+        "ro": false,
+        "write_threshold": 0
+      },
+      "io-status": "ok",
+      "locked": false,
+      "qdev": "/machine/peripheral-anon/device[0]/virtio-backend",
+      "removable": false,
+      "type": "unknown"
+    }
+  ]
+}
+{"execute": "x-debug-block-dirty-bitmap-sha256", "arguments": {"name": "bitmapA", "node": "drive0"}}
+{"return": {"sha256": "7abe3d7e3c794cfaf9b08bc9ce599192c96a2206f07b42d9997ff78fdd7af744"}}
+{"execute": "x-debug-block-dirty-bitmap-sha256", "arguments": {"name": "bitmapD", "node": "drive0"}}
+{"return": {"sha256": "7abe3d7e3c794cfaf9b08bc9ce599192c96a2206f07b42d9997ff78fdd7af744"}}
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 v2 4/7] iotests.py: don't abort if IMGKEYSECRET is undefined

[Eric Blake]

On 12/12/18 7:50 PM, John Snow wrote:
> Instead of using os.environ[], use .get with a default of empty string
> to match the setup in check to allow us to import the iotests module
> (for debugging, say) without needing a crafted environment just to
> import the module.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index d537538ba0..a34e66813a 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -63,7 +63,7 @@ socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
>   debug = False
>   
>   luks_default_secret_object = 'secret,id=keysec0,data=' + \
> -                             os.environ['IMGKEYSECRET']
> +                             os.environ.get('IMGKEYSECRET', '')
>   luks_default_key_secret_opt = 'key-secret=keysec0'
>   
>   
> 

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

Re: [Qemu-devel] [PATCH v2 5/7] iotests: add filter_generated_node_ids

[Eric Blake]

On 12/12/18 7:50 PM, John Snow wrote:
> To mimic the common filter of the same name, but for the python tests.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 3 +++
>   1 file changed, 3 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index a34e66813a..9595429fea 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -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'):
> 

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

Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log

[Eric Blake]

On 12/12/18 7:50 PM, John Snow wrote:
> 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 9595429fea..dbbef4bad3 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -447,12 +447,12 @@ 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, indent=None, filters=[filter_testfiles], **kwargs):

Why None instead of False?

>           logmsg = '{"execute": "%s", "arguments": %s}' % \
>               (cmd, json.dumps(kwargs, sort_keys=True))
>           log(logmsg, filters)
>           result = self.qmp(cmd, **kwargs)
> -        log(json.dumps(result, sort_keys=True), filters)
> +        log(json.dumps(result, indent=indent, sort_keys=True), filters)

But I'd actually have to read the contract to json.dumps() to learn what 
is expected.

/me goes and does that...
https://docs.python.org/2/library/json.html

If indent is a non-negative integer, then JSON array elements and object 
members will be pretty-printed with that indent level. An indent level 
of 0, or negative, will only insert newlines. None (the default) selects 
the most compact representation.

Okay, makes sense.
Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

[Eric Blake]

On 12/12/18 7:50 PM, John Snow wrote:
> New interface, new smoke test.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/236     | 123 +++++++++++++++++
>   tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 389 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

> +    log('')
> +    log('--- Disabling B & Adding C ---\n')
> +    vm.qmp_log("transaction", actions=[
> +        { "type": "block-dirty-bitmap-disable",
> +          "data": { "node": "drive0", "name": "bitmapB" }},
> +        { "type": "block-dirty-bitmap-add",
> +          "data": { "node": "drive0", "name": "bitmapC" }}
> +    ])

Just for grins, what happens if we extend that transaction to 
additionally call:

        { "type": "block-dirty-bitmap-disable",
          "data": { "node": "drive0", "name": "bitmapC" }},
        { "type": "block-dirty-bitmap-enable",
          "data": { "node": "drive0", "name": "bitmapC" }}

Yes, we have a redundancy where plain '-add' coupled with '-disable' in 
a transaction does the same as '-add' with 'enabled':false.  And I'd 
rather keep 'enabled':false as part of '-add', as it's handy to do that 
without having to code up a transaction.  But the specific act of 
toggling the enabled bit twice on a brand new bitmap as part of the same 
transaction, while unlikely to be done by libvirt, may still prove to be 
a useful validation of our transaction semantics.

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

You'll want to fix that.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 0/7] bitmaps: remove x- prefix from QMP api

[no-reply]

Patchew URL: https://patchew.org/QEMU/20181213015013.15350-1-jsnow@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20181213015013.15350-1-jsnow@redhat.com
Subject: [Qemu-devel] [PATCH v2 0/7] bitmaps: remove x- prefix from QMP api

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
7edca32 iotests: add iotest 236 for testing bitmap merge
4760f12 iotests: allow pretty-print for qmp_log
18479b6 iotests: add filter_generated_node_ids
e74d8c5 iotests.py: don't abort if IMGKEYSECRET is undefined
32d68bd4 block: remove 'x' prefix from experimental bitmap APIs
82059d7 blockdev: n-ary bitmap merge
51b6f48 blockdev: abort transactions in reverse order

=== OUTPUT BEGIN ===
Checking PATCH 1/7: blockdev: abort transactions in reverse order...
Checking PATCH 2/7: blockdev: n-ary bitmap merge...
ERROR: externs should be avoided in .c files
#27: FILE: blockdev.c:2125:
+void do_block_dirty_bitmap_merge(const char *node, const char *target,

total: 1 errors, 0 warnings, 147 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/7: block: remove 'x' prefix from experimental bitmap APIs...
Checking PATCH 4/7: iotests.py: don't abort if IMGKEYSECRET is undefined...
Checking PATCH 5/7: iotests: add filter_generated_node_ids...
Checking PATCH 6/7: iotests: allow pretty-print for qmp_log...
Checking PATCH 7/7: iotests: add iotest 236 for testing bitmap merge...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#12: 
new file mode 100755

total: 0 errors, 1 warnings, 392 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20181213015013.15350-1-jsnow@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH v2 2/7] blockdev: n-ary bitmap merge

[Vladimir Sementsov-Ogievskiy]

13.12.2018 4:50, John Snow wrote:
> Especially outside of transactions, it is helpful to provide
> all-or-nothing semantics for bitmap merges. This facilitates
> the coalescing of multiple bitmaps into a single target for
> the "checkpoint" interpretation when assembling bitmaps that
> represent arbitrary points in time from component bitmaps.
> 
> This is an incompatible change from the preliminary version
> of the API.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

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



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/7] block: remove 'x' prefix from experimental bitmap APIs

[Vladimir Sementsov-Ogievskiy]

13.12.2018 4:50, John Snow wrote:
> The 'x' prefix was added because I was uncertain of the direction we'd
> take for the libvirt API. With the general approach solidified, I feel
> comfortable committing to this API for 4.0.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>
> Reviewed-by: Eric Blake<eblake@redhat.com>


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

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 4/7] iotests.py: don't abort if IMGKEYSECRET is undefined

[Vladimir Sementsov-Ogievskiy]

13.12.2018 4:50, John Snow wrote:
> Instead of using os.environ[], use .get with a default of empty string
> to match the setup in check to allow us to import the iotests module
> (for debugging, say) without needing a crafted environment just to
> import the module.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>


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

> ---
>   tests/qemu-iotests/iotests.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index d537538ba0..a34e66813a 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -63,7 +63,7 @@ socket_scm_helper = os.environ.get('SOCKET_SCM_HELPER', 'socket_scm_helper')
>   debug = False
>   
>   luks_default_secret_object = 'secret,id=keysec0,data=' + \
> -                             os.environ['IMGKEYSECRET']
> +                             os.environ.get('IMGKEYSECRET', '')
>   luks_default_key_secret_opt = 'key-secret=keysec0'
>   
>   
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 5/7] iotests: add filter_generated_node_ids

[Vladimir Sementsov-Ogievskiy]

13.12.2018 4:50, John Snow wrote:
> To mimic the common filter of the same name, but for the python tests.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

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

> ---
>   tests/qemu-iotests/iotests.py | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index a34e66813a..9595429fea 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -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'):
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log

[Vladimir Sementsov-Ogievskiy]

13.12.2018 4:50, John Snow wrote:
> 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 9595429fea..dbbef4bad3 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -447,12 +447,12 @@ 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, indent=None, filters=[filter_testfiles], **kwargs):
>           logmsg = '{"execute": "%s", "arguments": %s}' % \
>               (cmd, json.dumps(kwargs, sort_keys=True))
>           log(logmsg, filters)

why not to prettify cmd json too? Just make fullobj = {"execute": cmd, "arguments": kwargs}, and prettify it.
(hmm, unfortunately, "execute" < "arguments", and they will be rearranged)

with or without (as the patch don't aim to prettify everything):
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

hmm, doing the same thing as Eric (check specs), I see the following note:

 > Note: Since the default item separator is ', ', the output might include trailing whitespace when indent is specified.

And I remember a pain of trailing whitespaces in iotests on, may be, backporting, or something like this some time ago.
It's of course not about this patch, but I think it is a good idea to avoid trailing whitespaces in test output, at least
in common helpers. May be best place to fix is iotests.log() function

>   
>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

13.12.2018 4:50, John Snow wrote:
> New interface, new smoke test.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/236     | 123 +++++++++++++++++
>   tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 389 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..3d162e967b
> --- /dev/null
> +++ b/tests/qemu-iotests/236
> @@ -0,0 +1,123 @@
> +#!/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 sys
> +import os
> +import iotests
> +from iotests import log
> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
> +from qemu import QEMUMachine

unused, with previous line and, therefore, os and sys modules)

> +
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +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)
> +
> +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')

hm, actually null device is enough here.

> +    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')
> +    for p in patterns:
> +        cmd = "write -P%s %s %s" % p
> +        log(cmd)
> +        log(vm.hmp_qemu_io("drive0", cmd))
> +
> +    vm.qmp_log("query-block", indent=2,
> +               filters=[iotests.filter_generated_node_ids,
> +                        iotests.filter_testfiles])

I'm against. query-block prints a lot of unrelated things, which may change from
version to version (for example, Andrey is now adding bitmap information to qcow2
format-specific info), then, backported test may fail for previous versions (or just
different config) because of something absolutely unrelated to bitmaps.

I think, it should be shortened to result[0]['device'], result[0]['dirty-bitmaps']

And, I think, any way it would be good to create a separate helper for printing
current state of bitmaps (which may output their sha256 too)

> +
> +    log('')
> +    log('--- Disabling B & Adding C ---\n')

why not just
log('\n--- Disabling B & Adding C ---\n')

[...]

Otherwise, the test looks fine for me


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log

[John Snow]

On 12/12/18 9:20 PM, Eric Blake wrote:
> On 12/12/18 7:50 PM, John Snow wrote:
>> 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/iotests.py | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py
>> b/tests/qemu-iotests/iotests.py
>> index 9595429fea..dbbef4bad3 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -447,12 +447,12 @@ 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, indent=None, filters=[filter_testfiles],
>> **kwargs):
> 
> Why None instead of False?
> 
>>           logmsg = '{"execute": "%s", "arguments": %s}' % \
>>               (cmd, json.dumps(kwargs, sort_keys=True))
>>           log(logmsg, filters)
>>           result = self.qmp(cmd, **kwargs)
>> -        log(json.dumps(result, sort_keys=True), filters)
>> +        log(json.dumps(result, indent=indent, sort_keys=True), filters)
> 
> But I'd actually have to read the contract to json.dumps() to learn what
> is expected.
> 
> /me goes and does that...
> https://docs.python.org/2/library/json.html
> 
> If indent is a non-negative integer, then JSON array elements and object
> members will be pretty-printed with that indent level. An indent level
> of 0, or negative, will only insert newlines. None (the default) selects
> the most compact representation.
> 
> Okay, makes sense.
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Yeah, it's a bubbling up of API in this case and a little messy, but it
was the quickest route to making the output look pretty.

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

[John Snow]

On 12/12/18 9:27 PM, Eric Blake wrote:
> On 12/12/18 7:50 PM, John Snow wrote:
>> New interface, new smoke test.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/236     | 123 +++++++++++++++++
>>   tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 389 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
> 
>> +    log('')
>> +    log('--- Disabling B & Adding C ---\n')
>> +    vm.qmp_log("transaction", actions=[
>> +        { "type": "block-dirty-bitmap-disable",
>> +          "data": { "node": "drive0", "name": "bitmapB" }},
>> +        { "type": "block-dirty-bitmap-add",
>> +          "data": { "node": "drive0", "name": "bitmapC" }}
>> +    ])
> 
> Just for grins, what happens if we extend that transaction to
> additionally call:
> 
>        { "type": "block-dirty-bitmap-disable",
>          "data": { "node": "drive0", "name": "bitmapC" }},
>        { "type": "block-dirty-bitmap-enable",
>          "data": { "node": "drive0", "name": "bitmapC" }}
> 
> Yes, we have a redundancy where plain '-add' coupled with '-disable' in
> a transaction does the same as '-add' with 'enabled':false.  And I'd
> rather keep 'enabled':false as part of '-add', as it's handy to do that
> without having to code up a transaction.  But the specific act of
> toggling the enabled bit twice on a brand new bitmap as part of the same
> transaction, while unlikely to be done by libvirt, may still prove to be
> a useful validation of our transaction semantics.
> 
>> +++ 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
> 
> You'll want to fix that.
> 

Now that you've pointed it out, I have to. :)

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

OK. I want more tests, too; mostly around malformed requests (bad
target, single bad source, multiple bad sources, mixed bad sources).
your suggestion to merge self into self,
and maybe anything else I've forgotten by now.

--js

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

[John Snow]

On 12/13/18 8:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.12.2018 4:50, John Snow wrote:
>> New interface, new smoke test.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/236     | 123 +++++++++++++++++
>>   tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/group   |   1 +
>>   3 files changed, 389 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..3d162e967b
>> --- /dev/null
>> +++ b/tests/qemu-iotests/236
>> @@ -0,0 +1,123 @@
>> +#!/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 sys
>> +import os
>> +import iotests
>> +from iotests import log
>> +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
>> +from qemu import QEMUMachine
> 
> unused, with previous line and, therefore, os and sys modules)
> 

Forgive the copy and paste. I'll trim it down.

>> +
>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>> +
>> +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)
>> +
>> +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')
> 
> hm, actually null device is enough here.
> 

Sure.

>> +    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')
>> +    for p in patterns:
>> +        cmd = "write -P%s %s %s" % p
>> +        log(cmd)
>> +        log(vm.hmp_qemu_io("drive0", cmd))
>> +
>> +    vm.qmp_log("query-block", indent=2,
>> +               filters=[iotests.filter_generated_node_ids,
>> +                        iotests.filter_testfiles])
> 
> I'm against. query-block prints a lot of unrelated things, which may change from
> version to version (for example, Andrey is now adding bitmap information to qcow2
> format-specific info), then, backported test may fail for previous versions (or just
> different config) because of something absolutely unrelated to bitmaps.
> 

You have a point. I'm only interested in the bitmap info structures, here.

> I think, it should be shortened to result[0]['device'], result[0]['dirty-bitmaps']
> 

OK, let me look at printing something like

{
  "device0": bitmapInfo0,
  "device1": bitmapInfo1,
  ...
}

to the log.

> And, I think, any way it would be good to create a separate helper for printing
> current state of bitmaps (which may output their sha256 too)
> 

Yeah, I was working on a command to do just this but I ran into troubles
with caching the bitmap info for computing it and wound up shelving the
series and I got sidetracked on other issues...

>> +
>> +    log('')
>> +    log('--- Disabling B & Adding C ---\n')
> 
> why not just
> log('\n--- Disabling B & Adding C ---\n')
> 

Just preference, there's no reason.

I have a habit of avoiding pre-pending control characters to string
lines, but don't have an aversion to appending them.

I can change it if it sticks out as too unusual.

> [...]
> 
> Otherwise, the test looks fine for me
> 
> 

Thanks for looking!

Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log

[John Snow]

On 12/13/18 8:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> 13.12.2018 4:50, John Snow wrote:
>> 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   tests/qemu-iotests/iotests.py | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 9595429fea..dbbef4bad3 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -447,12 +447,12 @@ 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, indent=None, filters=[filter_testfiles], **kwargs):
>>           logmsg = '{"execute": "%s", "arguments": %s}' % \
>>               (cmd, json.dumps(kwargs, sort_keys=True))
>>           log(logmsg, filters)
> 
> why not to prettify cmd json too? Just make fullobj = {"execute": cmd, "arguments": kwargs}, and prettify it.
> (hmm, unfortunately, "execute" < "arguments", and they will be rearranged)
> 

It wasn't long enough to irritate me :)
but we're here, so I'll do that too.

> with or without (as the patch don't aim to prettify everything):
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
>>           result = self.qmp(cmd, **kwargs)
>> -        log(json.dumps(result, sort_keys=True), filters)
>> +        log(json.dumps(result, indent=indent, sort_keys=True), filters)
>>           return result
> 
> hmm, doing the same thing as Eric (check specs), I see the following note:
> 
>  > Note: Since the default item separator is ', ', the output might include trailing whitespace when indent is specified.
> 
> And I remember a pain of trailing whitespaces in iotests on, may be, backporting, or something like this some time ago.
> It's of course not about this patch, but I think it is a good idea to avoid trailing whitespaces in test output, at least
> in common helpers. May be best place to fix is iotests.log() function
> 

Oh, good spot. I actually did run into this and wasn't aware of what
caused it! I will change the default separator.

>>   
>>       def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>
> 
> 

Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log

[Vladimir Sementsov-Ogievskiy]

14.12.2018 23:51, John Snow wrote:
> 
> 
> On 12/13/18 8:09 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 13.12.2018 4:50, John Snow wrote:
>>> 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.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>    tests/qemu-iotests/iotests.py | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 9595429fea..dbbef4bad3 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -447,12 +447,12 @@ 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, indent=None, filters=[filter_testfiles], **kwargs):
>>>            logmsg = '{"execute": "%s", "arguments": %s}' % \
>>>                (cmd, json.dumps(kwargs, sort_keys=True))
>>>            log(logmsg, filters)
>>
>> why not to prettify cmd json too? Just make fullobj = {"execute": cmd, "arguments": kwargs}, and prettify it.
>> (hmm, unfortunately, "execute" < "arguments", and they will be rearranged)
>>
> 
> It wasn't long enough to irritate me :)
> but we're here, so I'll do that too.
> 
>> with or without (as the patch don't aim to prettify everything):
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>>            result = self.qmp(cmd, **kwargs)
>>> -        log(json.dumps(result, sort_keys=True), filters)
>>> +        log(json.dumps(result, indent=indent, sort_keys=True), filters)
>>>            return result
>>
>> hmm, doing the same thing as Eric (check specs), I see the following note:
>>
>>   > Note: Since the default item separator is ', ', the output might include trailing whitespace when indent is specified.
>>
>> And I remember a pain of trailing whitespaces in iotests on, may be, backporting, or something like this some time ago.
>> It's of course not about this patch, but I think it is a good idea to avoid trailing whitespaces in test output, at least
>> in common helpers. May be best place to fix is iotests.log() function
>>
> 
> Oh, good spot. I actually did run into this and wasn't aware of what
> caused it! I will change the default separator.

In this case output would be less pretty. I'd prefer just remove trailing whitespace as part of filtering process in log().

> 
>>>    
>>>        def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>>
>>
>>


-- 
Best regards,
Vladimir