* [Qemu-devel] [PATCH v5 01/11] blockdev: abort transactions in reverse order
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 02/11] block/dirty-bitmap: remove assertion from restore John Snow
` (9 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
Presently, we abort transactions in the same order they were processed in.
Bitmap commands, though, attempt to restore backup data structures on abort.
That's not valid, 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 a6f71f9d83..43e4c22da5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1339,7 +1339,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 */
@@ -2266,8 +2266,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.
@@ -2298,7 +2298,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) {
@@ -2307,7 +2307,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);
}
@@ -2318,13 +2318,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
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v5 02/11] block/dirty-bitmap: remove assertion from restore
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 01/11] blockdev: abort transactions in reverse order John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:41 ` Eric Blake
2018-12-20 8:33 ` Vladimir Sementsov-Ogievskiy
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge John Snow
` (8 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
When making a backup of a dirty bitmap (for transactions), we want to
restore that backup whether or not the bitmap is enabled or not.
It is perfectly valid to write into bitmaps that are disabled. It is
only illegitimate for the guest to have done so.
Remove this assertion.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/dirty-bitmap.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 89fd1d7f8b..6b688394e4 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -625,7 +625,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
{
HBitmap *tmp = bitmap->bitmap;
- assert(bdrv_dirty_bitmap_enabled(bitmap));
assert(!bdrv_dirty_bitmap_readonly(bitmap));
bitmap->bitmap = backup;
hbitmap_free(tmp);
--
2.17.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 02/11] block/dirty-bitmap: remove assertion from restore
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 02/11] block/dirty-bitmap: remove assertion from restore John Snow
@ 2018-12-20 2:41 ` Eric Blake
2018-12-20 8:33 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Eric Blake @ 2018-12-20 2:41 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block
Cc: vsementsov, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
On 12/19/18 8:29 PM, John Snow wrote:
> When making a backup of a dirty bitmap (for transactions), we want to
> restore that backup whether or not the bitmap is enabled or not.
drop one of the two 'or not'
>
> It is perfectly valid to write into bitmaps that are disabled. It is
> only illegitimate for the guest to have done so.
>
> Remove this assertion.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/dirty-bitmap.c | 1 -
> 1 file changed, 1 deletion(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v5 02/11] block/dirty-bitmap: remove assertion from restore
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 02/11] block/dirty-bitmap: remove assertion from restore John Snow
2018-12-20 2:41 ` Eric Blake
@ 2018-12-20 8:33 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-20 8:33 UTC (permalink / raw)
To: John Snow, qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Eric Blake, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
20.12.2018 5:29, John Snow wrote:
> When making a backup of a dirty bitmap (for transactions), we want to
> restore that backup whether or not the bitmap is enabled or not.
>
> It is perfectly valid to write into bitmaps that are disabled. It is
> only illegitimate for the guest to have done so.
>
> Remove this assertion.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/dirty-bitmap.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 89fd1d7f8b..6b688394e4 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -625,7 +625,6 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
> void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup)
> {
> HBitmap *tmp = bitmap->bitmap;
> - assert(bdrv_dirty_bitmap_enabled(bitmap));
> assert(!bdrv_dirty_bitmap_readonly(bitmap));
> bitmap->bitmap = backup;
> hbitmap_free(tmp);
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 01/11] blockdev: abort transactions in reverse order John Snow
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 02/11] block/dirty-bitmap: remove assertion from restore John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:48 ` Eric Blake
2018-12-20 9:23 ` Vladimir Sementsov-Ogievskiy
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 04/11] block: remove 'x' prefix from experimental bitmap APIs John Snow
` (7 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
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>
---
blockdev.c | 75 ++++++++++++++++++++++++++++++--------------
qapi/block-core.json | 22 ++++++-------
2 files changed, 62 insertions(+), 35 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 43e4c22da5..6031c94121 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2119,33 +2119,28 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
}
}
+static BdrvDirtyBitmap *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);
+ state->bitmap = do_block_dirty_bitmap_merge(action->node, action->target,
+ action->bitmaps, &state->backup,
+ errp);
}
static void abort_prepare(BlkActionState *common, Error **errp)
@@ -2977,24 +2972,56 @@ 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)
+static BdrvDirtyBitmap *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;
+ return NULL;
}
- src = bdrv_find_dirty_bitmap(bs, src_name);
- if (!src) {
- error_setg(errp, "Dirty bitmap '%s' not found", src_name);
- return;
+ anon = bdrv_create_dirty_bitmap(bs, bdrv_dirty_bitmap_granularity(dst),
+ NULL, errp);
+ if (!anon) {
+ return NULL;
}
- 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);
+ dst = NULL;
+ goto out;
+ }
+
+ bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ dst = NULL;
+ 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);
+ return dst;
+}
+
+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 762000f31f..a153ea4420 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1821,14 +1821,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:
@@ -1943,23 +1943,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
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge John Snow
@ 2018-12-20 2:48 ` Eric Blake
2018-12-20 21:03 ` John Snow
2018-12-20 9:23 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 31+ messages in thread
From: Eric Blake @ 2018-12-20 2:48 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block
Cc: vsementsov, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
On 12/19/18 8:29 PM, 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.
but that doesn't matter because it was in the x- namespace, and we're
about to rename it anyway.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> blockdev.c | 75 ++++++++++++++++++++++++++++++--------------
> qapi/block-core.json | 22 ++++++-------
> 2 files changed, 62 insertions(+), 35 deletions(-)
>
> +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
> + const char *target,
> + strList *bitmaps,
> + HBitmap **backup,
> + Error **errp)
> {
> - 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);
> + dst = NULL;
> + goto out;
> + }
> +
> + bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + dst = NULL;
> + goto out;
> + }
> + }
Appears to be a silent no-op when given "bitmaps":[] as the source. An
alternative would be requiring at least one source in the list, but I
don't see it as worth changing the patch to special-case an empty list
differently from a no-op.
> @@ -1943,23 +1943,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
Could do s/3.0/4.0/ to match the incompatible change here, but you do it
in the later patch where your remove the x-.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge
2018-12-20 2:48 ` Eric Blake
@ 2018-12-20 21:03 ` John Snow
0 siblings, 0 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 21:03 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block
Cc: Kevin Wolf, Fam Zheng, vsementsov, Markus Armbruster, Max Reitz
On 12/19/18 9:48 PM, Eric Blake wrote:
> On 12/19/18 8:29 PM, 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.
>
> but that doesn't matter because it was in the x- namespace, and we're
> about to rename it anyway.
>
Yes, just an "FYI".
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> blockdev.c | 75 ++++++++++++++++++++++++++++++--------------
>> qapi/block-core.json | 22 ++++++-------
>> 2 files changed, 62 insertions(+), 35 deletions(-)
>>
>
>> +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
>> + const char *target,
>> + strList *bitmaps,
>> + HBitmap **backup,
>> + Error **errp)
>> {
>
>> - 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);
>> + dst = NULL;
>> + goto out;
>> + }
>> +
>> + bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + dst = NULL;
>> + goto out;
>> + }
>> + }
>
> Appears to be a silent no-op when given "bitmaps":[] as the source. An
> alternative would be requiring at least one source in the list, but I
> don't see it as worth changing the patch to special-case an empty list
> differently from a no-op.
> >> @@ -1943,23 +1943,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
>
> Could do s/3.0/4.0/ to match the incompatible change here, but you do it
> in the later patch where your remove the x-.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
Yeah, I think I'll just leave it this way, so all the version
graduations are in the same patch.
Thank you!
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge John Snow
2018-12-20 2:48 ` Eric Blake
@ 2018-12-20 9:23 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-20 9:23 UTC (permalink / raw)
To: John Snow, qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Eric Blake, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
20.12.2018 5:29, 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: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v5 04/11] block: remove 'x' prefix from experimental bitmap APIs
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
` (2 preceding siblings ...)
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 03/11] blockdev: n-ary bitmap merge John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 05/11] iotests.py: don't abort if IMGKEYSECRET is undefined John Snow
` (6 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
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>
---
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 6031c94121..8e37dd659e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1963,7 +1963,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) {
@@ -2048,7 +2048,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,
@@ -2089,7 +2089,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,
@@ -2136,7 +2136,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;
state->bitmap = do_block_dirty_bitmap_merge(action->node, action->target,
action->bitmaps, &state->backup,
@@ -2204,17 +2204,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,
@@ -2930,7 +2930,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;
@@ -2951,7 +2951,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;
@@ -3018,8 +3018,8 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
return dst;
}
-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 a153ea4420..91685be6c2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1806,15 +1806,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:
@@ -1825,7 +1825,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'] } }
@@ -1899,7 +1899,7 @@
'data': 'BlockDirtyBitmap' }
##
-# @x-block-dirty-bitmap-enable:
+# @block-dirty-bitmap-enable:
#
# Enables a dirty bitmap so that it will begin tracking disk changes.
#
@@ -1907,20 +1907,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.
#
@@ -1928,20 +1928,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.
@@ -1953,17 +1953,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
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v5 05/11] iotests.py: don't abort if IMGKEYSECRET is undefined
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
` (3 preceding siblings ...)
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 04/11] block: remove 'x' prefix from experimental bitmap APIs John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 06/11] iotests: add filter_generated_node_ids John Snow
` (5 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
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: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-id: 20181214231512.5295-5-jsnow@redhat.com
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
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v5 06/11] iotests: add filter_generated_node_ids
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
` (4 preceding siblings ...)
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 05/11] iotests.py: don't abort if IMGKEYSECRET is undefined John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 07/11] iotests: add qmp recursive sorting function John Snow
` (4 subsequent siblings)
10 siblings, 0 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
To mimic the common filter of the same name, but for the python tests.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@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'):
--
2.17.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PATCH v5 07/11] iotests: add qmp recursive sorting function
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
` (5 preceding siblings ...)
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 06/11] iotests: add filter_generated_node_ids John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:40 ` Eric Blake
2018-12-20 9:42 ` Vladimir Sementsov-Ogievskiy
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 08/11] iotests: remove default filters from qmp_log John Snow
` (3 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
Python before 3.6 does not sort dictionaries (including kwargs).
Therefore, printing QMP objects involves sorting the keys to have
a predictable ordering in the iotests output.
However, if we want to pretty-print QMP objects being sent to the
QEMU process, we need to build the entire command before logging it.
Ordinarily, this would then involve "arguments" being sorted above
"execute", which would necessitate a rather ugly and harder-to-read
change to many iotests outputs.
To facilitate pretty-printing AND maintaining predictable output AND
having "arguments" sort before "execute", add a custom sort function
that takes a dictionary and recursively builds an OrderedDict that
maintains the specific key order we wish to see in iotests output.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/iotests.py | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 9595429fea..565eebb1ab 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -30,6 +30,7 @@ import signal
import logging
import atexit
import io
+from collections import OrderedDict
sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts'))
import qtest
@@ -75,6 +76,16 @@ def qemu_img(*args):
sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
return exitcode
+def ordered_kwargs(kwargs):
+ # kwargs prior to 3.6 are not ordered, so:
+ od = OrderedDict()
+ for k, v in sorted(kwargs.items()):
+ if isinstance(v, dict):
+ od[k] = ordered_kwargs(v)
+ else:
+ od[k] = v
+ return od
+
def qemu_img_create(*args):
args = list(args)
@@ -257,8 +268,10 @@ def filter_img_info(output, filename):
def log(msg, filters=[]):
for flt in filters:
msg = flt(msg)
- if type(msg) is dict or type(msg) is list:
- print(json.dumps(msg, sort_keys=True))
+ if isinstance(msg, dict) or isinstance(msg, list):
+ # Don't sort if it's already sorted
+ do_sort = not isinstance(msg, OrderedDict)
+ print(json.dumps(msg, sort_keys=do_sort))
else:
print(msg)
@@ -448,8 +461,11 @@ class VM(qtest.QEMUQtestMachine):
return result
def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
- logmsg = '{"execute": "%s", "arguments": %s}' % \
- (cmd, json.dumps(kwargs, sort_keys=True))
+ full_cmd = OrderedDict((
+ ("execute", cmd),
+ ("arguments", ordered_kwargs(kwargs))
+ ))
+ logmsg = json.dumps(full_cmd)
log(logmsg, filters)
result = self.qmp(cmd, **kwargs)
log(json.dumps(result, sort_keys=True), filters)
--
2.17.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 07/11] iotests: add qmp recursive sorting function
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 07/11] iotests: add qmp recursive sorting function John Snow
@ 2018-12-20 2:40 ` Eric Blake
2018-12-20 9:42 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Eric Blake @ 2018-12-20 2:40 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block
Cc: vsementsov, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
On 12/19/18 8:29 PM, John Snow wrote:
> Python before 3.6 does not sort dictionaries (including kwargs).
> Therefore, printing QMP objects involves sorting the keys to have
> a predictable ordering in the iotests output.
It may be worth also mentioning that sometimes this sorting results in
the log showing things in a different order than the source command
(with no ill effect, as long as the output order is deterministic).
>
> However, if we want to pretty-print QMP objects being sent to the
> QEMU process, we need to build the entire command before logging it.
> Ordinarily, this would then involve "arguments" being sorted above
> "execute", which would necessitate a rather ugly and harder-to-read
> change to many iotests outputs.
>
> To facilitate pretty-printing AND maintaining predictable output AND
> having "arguments" sort before "execute", add a custom sort function
s/before/after/
> that takes a dictionary and recursively builds an OrderedDict that
> maintains the specific key order we wish to see in iotests output.
namely, keys within subdicts are sorted by key name (even if that is not
the order they were input), but the top-level struct with "execute" and
"arguments" stays the way we want it.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/iotests.py | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v5 07/11] iotests: add qmp recursive sorting function
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 07/11] iotests: add qmp recursive sorting function John Snow
2018-12-20 2:40 ` Eric Blake
@ 2018-12-20 9:42 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-20 9:42 UTC (permalink / raw)
To: John Snow, qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Eric Blake, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
20.12.2018 5:29, John Snow wrote:
> Python before 3.6 does not sort dictionaries (including kwargs).
> Therefore, printing QMP objects involves sorting the keys to have
> a predictable ordering in the iotests output.
>
> However, if we want to pretty-print QMP objects being sent to the
> QEMU process, we need to build the entire command before logging it.
> Ordinarily, this would then involve "arguments" being sorted above
> "execute", which would necessitate a rather ugly and harder-to-read
> change to many iotests outputs.
I'm unsure about what it means 'build the entire command before logging'.
[upd, after a second]
aha, it's about '{"execute":...' -> {'execute': ...}
may be, build the entire command object to be passed to json.dumps, or
like this would be better, if you want.
>
> To facilitate pretty-printing AND maintaining predictable output AND
> having "arguments" sort before "execute", add a custom sort function
> that takes a dictionary and recursively builds an OrderedDict that
> maintains the specific key order we wish to see in iotests output.
>
> Signed-off-by: John Snow<jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v5 08/11] iotests: remove default filters from qmp_log
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
` (6 preceding siblings ...)
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 07/11] iotests: add qmp recursive sorting function John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:50 ` Eric Blake
2018-12-20 9:48 ` Vladimir Sementsov-Ogievskiy
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 09/11] iotests: change qmp_log filters to expect QMP objects only John Snow
` (2 subsequent siblings)
10 siblings, 2 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
Several places in iotests deal with serializing objects into JSON
strings, but to add pretty-printing it seems desireable to localize
all of those cases.
log() seems like a good candidate for that centralized behavior.
log() can already serialize json objects, but when it does so,
it assumes filters=[] operates on QMP objects, not strings.
qmp_log currently operates by dumping outgoing and incoming QMP
objects into strings and filtering them assuming that filters=[]
are string filters.
To have qmp_log use log's serialization, qmp_log will need to
accept only qmp filters, not text filters.
However, only a single caller of qmp_log actually requires any
filters at all. I remove the default filter and add it explicitly
to the caller in preparation for refactoring qmp_log to use rich
filters instead.
test 206 is amended to name the filter explicitly and the default
is removed.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/206 | 8 ++++++--
tests/qemu-iotests/iotests.py | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index 128c334c7c..e92550fa59 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -26,7 +26,9 @@ from iotests import imgfmt
iotests.verify_image_format(supported_fmts=['qcow2'])
def blockdev_create(vm, options):
- result = vm.qmp_log('blockdev-create', job_id='job0', options=options)
+ result = vm.qmp_log('blockdev-create',
+ filters=[iotests.filter_testfiles],
+ job_id='job0', options=options)
if 'return' in result:
assert result['return'] == {}
@@ -52,7 +54,9 @@ with iotests.FilePath('t.qcow2') as disk_path, \
'filename': disk_path,
'size': 0 })
- vm.qmp_log('blockdev-add', driver='file', filename=disk_path,
+ vm.qmp_log('blockdev-add',
+ filters=[iotests.filter_testfiles],
+ driver='file', filename=disk_path,
node_name='imgfile')
blockdev_create(vm, { 'driver': imgfmt,
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 565eebb1ab..57fe20db45 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -460,7 +460,7 @@ class VM(qtest.QEMUQtestMachine):
result.append(filter_qmp_event(ev))
return result
- def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
+ def qmp_log(self, cmd, filters=[], **kwargs):
full_cmd = OrderedDict((
("execute", cmd),
("arguments", ordered_kwargs(kwargs))
--
2.17.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 08/11] iotests: remove default filters from qmp_log
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 08/11] iotests: remove default filters from qmp_log John Snow
@ 2018-12-20 2:50 ` Eric Blake
2018-12-20 9:48 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Eric Blake @ 2018-12-20 2:50 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block
Cc: vsementsov, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
On 12/19/18 8:29 PM, John Snow wrote:
> Several places in iotests deal with serializing objects into JSON
> strings, but to add pretty-printing it seems desireable to localize
s/desireable/desirable/
> all of those cases.
>
> log() seems like a good candidate for that centralized behavior.
> log() can already serialize json objects, but when it does so,
> it assumes filters=[] operates on QMP objects, not strings.
>
> qmp_log currently operates by dumping outgoing and incoming QMP
> objects into strings and filtering them assuming that filters=[]
> are string filters.
>
> To have qmp_log use log's serialization, qmp_log will need to
> accept only qmp filters, not text filters.
>
> However, only a single caller of qmp_log actually requires any
> filters at all. I remove the default filter and add it explicitly
> to the caller in preparation for refactoring qmp_log to use rich
> filters instead.
>
> test 206 is amended to name the filter explicitly and the default
> is removed.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/206 | 8 ++++++--
> tests/qemu-iotests/iotests.py | 2 +-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v5 08/11] iotests: remove default filters from qmp_log
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 08/11] iotests: remove default filters from qmp_log John Snow
2018-12-20 2:50 ` Eric Blake
@ 2018-12-20 9:48 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-20 9:48 UTC (permalink / raw)
To: John Snow, qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Eric Blake, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
20.12.2018 5:29, John Snow wrote:
> Several places in iotests deal with serializing objects into JSON
> strings, but to add pretty-printing it seems desireable to localize
> all of those cases.
>
> log() seems like a good candidate for that centralized behavior.
> log() can already serialize json objects, but when it does so,
> it assumes filters=[] operates on QMP objects, not strings.
>
> qmp_log currently operates by dumping outgoing and incoming QMP
> objects into strings and filtering them assuming that filters=[]
> are string filters.
>
> To have qmp_log use log's serialization, qmp_log will need to
> accept only qmp filters, not text filters.
>
> However, only a single caller of qmp_log actually requires any
> filters at all. I remove the default filter and add it explicitly
> to the caller in preparation for refactoring qmp_log to use rich
> filters instead.
>
> test 206 is amended to name the filter explicitly and the default
> is removed.
>
> Signed-off-by: John Snow<jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v5 09/11] iotests: change qmp_log filters to expect QMP objects only
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
` (7 preceding siblings ...)
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 08/11] iotests: remove default filters from qmp_log John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:53 ` Eric Blake
2018-12-20 11:21 ` Vladimir Sementsov-Ogievskiy
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 10/11] iotests: implement pretty-print for log and qmp_log John Snow
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge John Snow
10 siblings, 2 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
As laid out in the previous commit's message:
```
Several places in iotests deal with serializing objects into JSON
strings, but to add pretty-printing it seems desireable to localize
all of those cases.
log() seems like a good candidate for that centralized behavior.
log() can already serialize json objects, but when it does so,
it assumes filters=[] operates on QMP objects, not strings.
qmp_log currently operates by dumping outgoing and incoming QMP
objects into strings and filtering them assuming that filters=[]
are string filters.
```
Therefore:
Change qmp_log to treat filters as if they're always qmp object filters,
then change the logging call to rely on log()'s ability to serialize QMP
objects, so we're not duplicating that effort.
Add a qmp version of filter_testfiles and adjust the only caller using
it for qmp_log to use the qmp version.
Signed-off-by: John Snow <jsnow@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/206 | 4 ++--
tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
index e92550fa59..5bb738bf23 100755
--- a/tests/qemu-iotests/206
+++ b/tests/qemu-iotests/206
@@ -27,7 +27,7 @@ iotests.verify_image_format(supported_fmts=['qcow2'])
def blockdev_create(vm, options):
result = vm.qmp_log('blockdev-create',
- filters=[iotests.filter_testfiles],
+ filters=[iotests.filter_qmp_testfiles],
job_id='job0', options=options)
if 'return' in result:
@@ -55,7 +55,7 @@ with iotests.FilePath('t.qcow2') as disk_path, \
'size': 0 })
vm.qmp_log('blockdev-add',
- filters=[iotests.filter_testfiles],
+ filters=[iotests.filter_qmp_testfiles],
driver='file', filename=disk_path,
node_name='imgfile')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 57fe20db45..dcd0c6f71d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -246,10 +246,29 @@ def filter_qmp_event(event):
event['timestamp']['microseconds'] = 'USECS'
return event
+def filter_qmp(qmsg, filter_fn):
+ '''Given a string filter, filter a QMP object's values.
+ filter_fn takes a (key, value) pair.'''
+ for key in qmsg:
+ if isinstance(qmsg[key], list):
+ qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
+ elif isinstance(qmsg[key], dict):
+ qmsg[key] = filter_qmp(qmsg[key], filter_fn)
+ else:
+ qmsg[key] = filter_fn(key, qmsg[key])
+ return qmsg
+
def filter_testfiles(msg):
prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
return msg.replace(prefix, 'TEST_DIR/PID-')
+def filter_qmp_testfiles(qmsg):
+ def _filter(key, value):
+ if key == 'filename' or key == 'backing-file':
+ return filter_testfiles(value)
+ return value
+ return filter_qmp(qmsg, _filter)
+
def filter_generated_node_ids(msg):
return re.sub("#block[0-9]+", "NODE_NAME", msg)
@@ -465,10 +484,9 @@ class VM(qtest.QEMUQtestMachine):
("execute", cmd),
("arguments", ordered_kwargs(kwargs))
))
- logmsg = json.dumps(full_cmd)
- log(logmsg, filters)
+ log(full_cmd, filters)
result = self.qmp(cmd, **kwargs)
- log(json.dumps(result, sort_keys=True), filters)
+ log(result, filters)
return result
def run_job(self, job, auto_finalize=True, auto_dismiss=False):
--
2.17.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 09/11] iotests: change qmp_log filters to expect QMP objects only
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 09/11] iotests: change qmp_log filters to expect QMP objects only John Snow
@ 2018-12-20 2:53 ` Eric Blake
2018-12-20 22:12 ` John Snow
2018-12-20 11:21 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 31+ messages in thread
From: Eric Blake @ 2018-12-20 2:53 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block
Cc: vsementsov, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
On 12/19/18 8:29 PM, John Snow wrote:
> As laid out in the previous commit's message:
>
> ```
> Several places in iotests deal with serializing objects into JSON
> strings, but to add pretty-printing it seems desireable to localize
s/desireable/desirable/
> all of those cases.
>
> log() seems like a good candidate for that centralized behavior.
> log() can already serialize json objects, but when it does so,
> it assumes filters=[] operates on QMP objects, not strings.
>
> qmp_log currently operates by dumping outgoing and incoming QMP
> objects into strings and filtering them assuming that filters=[]
> are string filters.
> ```
>
> Therefore:
>
> Change qmp_log to treat filters as if they're always qmp object filters,
> then change the logging call to rely on log()'s ability to serialize QMP
> objects, so we're not duplicating that effort.
>
> Add a qmp version of filter_testfiles and adjust the only caller using
> it for qmp_log to use the qmp version.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
Odd double S-o-B differing only by space.
> ---
> tests/qemu-iotests/206 | 4 ++--
> tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v5 09/11] iotests: change qmp_log filters to expect QMP objects only
2018-12-20 2:53 ` Eric Blake
@ 2018-12-20 22:12 ` John Snow
0 siblings, 0 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 22:12 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block
Cc: Kevin Wolf, Fam Zheng, vsementsov, Markus Armbruster, Max Reitz
On 12/19/18 9:53 PM, Eric Blake wrote:
> On 12/19/18 8:29 PM, John Snow wrote:
>> As laid out in the previous commit's message:
>>
>> ```
>> Several places in iotests deal with serializing objects into JSON
>> strings, but to add pretty-printing it seems desireable to localize
>
> s/desireable/desirable/
>
>> all of those cases.
>>
>> log() seems like a good candidate for that centralized behavior.
>> log() can already serialize json objects, but when it does so,
>> it assumes filters=[] operates on QMP objects, not strings.
>>
>> qmp_log currently operates by dumping outgoing and incoming QMP
>> objects into strings and filtering them assuming that filters=[]
>> are string filters.
>> ```
>>
>> Therefore:
>>
>> Change qmp_log to treat filters as if they're always qmp object filters,
>> then change the logging call to rely on log()'s ability to serialize QMP
>> objects, so we're not duplicating that effort.
>>
>> Add a qmp version of filter_testfiles and adjust the only caller using
>> it for qmp_log to use the qmp version.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Odd double S-o-B differing only by space.
I fixed my auto-signer! It has rudely detected my typo and decided that
it needed a fresh SOB.
>
>> ---
>> tests/qemu-iotests/206 | 4 ++--
>> tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
>> 2 files changed, 23 insertions(+), 5 deletions(-)
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
Thanks!
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v5 09/11] iotests: change qmp_log filters to expect QMP objects only
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 09/11] iotests: change qmp_log filters to expect QMP objects only John Snow
2018-12-20 2:53 ` Eric Blake
@ 2018-12-20 11:21 ` Vladimir Sementsov-Ogievskiy
2018-12-20 22:26 ` John Snow
1 sibling, 1 reply; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-20 11:21 UTC (permalink / raw)
To: John Snow, qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Eric Blake, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
20.12.2018 5:29, John Snow wrote:
> As laid out in the previous commit's message:
>
> ```
> Several places in iotests deal with serializing objects into JSON
> strings, but to add pretty-printing it seems desireable to localize
> all of those cases.
>
> log() seems like a good candidate for that centralized behavior.
> log() can already serialize json objects, but when it does so,
> it assumes filters=[] operates on QMP objects, not strings.
>
> qmp_log currently operates by dumping outgoing and incoming QMP
> objects into strings and filtering them assuming that filters=[]
> are string filters.
> ```
>
> Therefore:
>
> Change qmp_log to treat filters as if they're always qmp object filters,
> then change the logging call to rely on log()'s ability to serialize QMP
> objects, so we're not duplicating that effort.
>
> Add a qmp version of filter_testfiles and adjust the only caller using
> it for qmp_log to use the qmp version.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/206 | 4 ++--
> tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
> 2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
> index e92550fa59..5bb738bf23 100755
> --- a/tests/qemu-iotests/206
> +++ b/tests/qemu-iotests/206
> @@ -27,7 +27,7 @@ iotests.verify_image_format(supported_fmts=['qcow2'])
>
> def blockdev_create(vm, options):
> result = vm.qmp_log('blockdev-create',
> - filters=[iotests.filter_testfiles],
> + filters=[iotests.filter_qmp_testfiles],
> job_id='job0', options=options)
>
> if 'return' in result:
> @@ -55,7 +55,7 @@ with iotests.FilePath('t.qcow2') as disk_path, \
> 'size': 0 })
>
> vm.qmp_log('blockdev-add',
> - filters=[iotests.filter_testfiles],
> + filters=[iotests.filter_qmp_testfiles],
> driver='file', filename=disk_path,
> node_name='imgfile')
>
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 57fe20db45..dcd0c6f71d 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -246,10 +246,29 @@ def filter_qmp_event(event):
> event['timestamp']['microseconds'] = 'USECS'
> return event
>
> +def filter_qmp(qmsg, filter_fn):
> + '''Given a string filter, filter a QMP object's values.
> + filter_fn takes a (key, value) pair.'''
hm, I decided to look into PEP8, which in turn refers to PEP257,
which asks:
- For consistency, always use """triple double quotes""" around docstring
- Unless the entire docstring fits on a line, place the closing quotes on a line by themselves
Unfortunately, iotests.py prefers to be in opposition.. And consistency within the file is more
important. May be we'll fix it one day..
> + for key in qmsg:
and here again we can benefit (or all right-value qmsg[key]) of using qmsg.items()
> + if isinstance(qmsg[key], list):
> + qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
hmm, stop. filter_qmp() assumes that its argument is dict. but atom may not be dict.
so, to fit into the concept of fn(key, value) filtering function, we should do something like
this:
for i in len(qmsg[key]):
if isinstance(qmsg[key], dict):
qmsg[key][i] = filter_qmp(qmsg[key][i], filter_fn)
qmsg[key] = filter_fn(key, qmsg[key])
---
or, we may want to apply filter_fn only to lists of non-dicts, and filter only list of dicts,
assuming that we don't have mixed lists.
> + elif isinstance(qmsg[key], dict):
> + qmsg[key] = filter_qmp(qmsg[key], filter_fn)
> + else:
> + qmsg[key] = filter_fn(key, qmsg[key]) > + return qmsg
> +
> def filter_testfiles(msg):
> prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
> return msg.replace(prefix, 'TEST_DIR/PID-')
>
> +def filter_qmp_testfiles(qmsg):
> + def _filter(key, value):
> + if key == 'filename' or key == 'backing-file':
> + return filter_testfiles(value)
> + return value
> + return filter_qmp(qmsg, _filter)
> +
> def filter_generated_node_ids(msg):
> return re.sub("#block[0-9]+", "NODE_NAME", msg)
>
> @@ -465,10 +484,9 @@ class VM(qtest.QEMUQtestMachine):
> ("execute", cmd),
> ("arguments", ordered_kwargs(kwargs))
> ))
> - logmsg = json.dumps(full_cmd)
> - log(logmsg, filters)
> + log(full_cmd, filters)
> result = self.qmp(cmd, **kwargs)
> - log(json.dumps(result, sort_keys=True), filters)
> + log(result, filters)
> return result
>
> def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 09/11] iotests: change qmp_log filters to expect QMP objects only
2018-12-20 11:21 ` Vladimir Sementsov-Ogievskiy
@ 2018-12-20 22:26 ` John Snow
0 siblings, 0 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 22:26 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel@nongnu.org,
qemu-block@nongnu.org
Cc: Kevin Wolf, Fam Zheng, Markus Armbruster, Max Reitz
On 12/20/18 6:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.12.2018 5:29, John Snow wrote:
>> As laid out in the previous commit's message:
>>
>> ```
>> Several places in iotests deal with serializing objects into JSON
>> strings, but to add pretty-printing it seems desireable to localize
>> all of those cases.
>>
>> log() seems like a good candidate for that centralized behavior.
>> log() can already serialize json objects, but when it does so,
>> it assumes filters=[] operates on QMP objects, not strings.
>>
>> qmp_log currently operates by dumping outgoing and incoming QMP
>> objects into strings and filtering them assuming that filters=[]
>> are string filters.
>> ```
>>
>> Therefore:
>>
>> Change qmp_log to treat filters as if they're always qmp object filters,
>> then change the logging call to rely on log()'s ability to serialize QMP
>> objects, so we're not duplicating that effort.
>>
>> Add a qmp version of filter_testfiles and adjust the only caller using
>> it for qmp_log to use the qmp version.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> tests/qemu-iotests/206 | 4 ++--
>> tests/qemu-iotests/iotests.py | 24 +++++++++++++++++++++---
>> 2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206
>> index e92550fa59..5bb738bf23 100755
>> --- a/tests/qemu-iotests/206
>> +++ b/tests/qemu-iotests/206
>> @@ -27,7 +27,7 @@ iotests.verify_image_format(supported_fmts=['qcow2'])
>>
>> def blockdev_create(vm, options):
>> result = vm.qmp_log('blockdev-create',
>> - filters=[iotests.filter_testfiles],
>> + filters=[iotests.filter_qmp_testfiles],
>> job_id='job0', options=options)
>>
>> if 'return' in result:
>> @@ -55,7 +55,7 @@ with iotests.FilePath('t.qcow2') as disk_path, \
>> 'size': 0 })
>>
>> vm.qmp_log('blockdev-add',
>> - filters=[iotests.filter_testfiles],
>> + filters=[iotests.filter_qmp_testfiles],
>> driver='file', filename=disk_path,
>> node_name='imgfile')
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 57fe20db45..dcd0c6f71d 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -246,10 +246,29 @@ def filter_qmp_event(event):
>> event['timestamp']['microseconds'] = 'USECS'
>> return event
>>
>> +def filter_qmp(qmsg, filter_fn):
>> + '''Given a string filter, filter a QMP object's values.
>> + filter_fn takes a (key, value) pair.'''
>
> hm, I decided to look into PEP8, which in turn refers to PEP257,
> which asks:
> - For consistency, always use """triple double quotes""" around docstring
> - Unless the entire docstring fits on a line, place the closing quotes on a line by themselves
>
> Unfortunately, iotests.py prefers to be in opposition.. And consistency within the file is more
> important. May be we'll fix it one day..
>
>
>> + for key in qmsg:
>
> and here again we can benefit (or all right-value qmsg[key]) of using qmsg.items()
>
>> + if isinstance(qmsg[key], list):
>> + qmsg[key] = [filter_qmp(atom, filter_fn) for atom in qmsg[key]]
>
> hmm, stop. filter_qmp() assumes that its argument is dict. but atom may not be dict.
>
Oh, good catch. Will fix.
> so, to fit into the concept of fn(key, value) filtering function, we should do something like
> this:
>
> for i in len(qmsg[key]):
> if isinstance(qmsg[key], dict):
> qmsg[key][i] = filter_qmp(qmsg[key][i], filter_fn)
>
> qmsg[key] = filter_fn(key, qmsg[key])
>
> ---
> or, we may want to apply filter_fn only to lists of non-dicts, and filter only list of dicts,
> assuming that we don't have mixed lists.
>
>
>> + elif isinstance(qmsg[key], dict):
>> + qmsg[key] = filter_qmp(qmsg[key], filter_fn)
>> + else:
>> + qmsg[key] = filter_fn(key, qmsg[key]) > + return qmsg
>> +
>> def filter_testfiles(msg):
>> prefix = os.path.join(test_dir, "%s-" % (os.getpid()))
>> return msg.replace(prefix, 'TEST_DIR/PID-')
>>
>> +def filter_qmp_testfiles(qmsg):
>> + def _filter(key, value):
>> + if key == 'filename' or key == 'backing-file':
>> + return filter_testfiles(value)
>> + return value
>> + return filter_qmp(qmsg, _filter)
>> +
>> def filter_generated_node_ids(msg):
>> return re.sub("#block[0-9]+", "NODE_NAME", msg)
>>
>> @@ -465,10 +484,9 @@ class VM(qtest.QEMUQtestMachine):
>> ("execute", cmd),
>> ("arguments", ordered_kwargs(kwargs))
>> ))
>> - logmsg = json.dumps(full_cmd)
>> - log(logmsg, filters)
>> + log(full_cmd, filters)
>> result = self.qmp(cmd, **kwargs)
>> - log(json.dumps(result, sort_keys=True), filters)
>> + log(result, filters)
>> return result
>>
>> def run_job(self, job, auto_finalize=True, auto_dismiss=False):
>>
>
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v5 10/11] iotests: implement pretty-print for log and qmp_log
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
` (8 preceding siblings ...)
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 09/11] iotests: change qmp_log filters to expect QMP objects only John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 2:55 ` Eric Blake
2018-12-20 11:29 ` Vladimir Sementsov-Ogievskiy
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge John Snow
10 siblings, 2 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
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 | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index dcd0c6f71d..d65bcaf953 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -284,13 +284,18 @@ def filter_img_info(output, filename):
lines.append(line)
return '\n'.join(lines)
-def log(msg, filters=[]):
+def log(msg, filters=[], indent=None):
+ '''Logs either a string message or a JSON serializable message (like QMP).
+ If indent is provided, JSON serializable messages are pretty-printed.'''
for flt in filters:
msg = flt(msg)
if isinstance(msg, dict) or isinstance(msg, list):
+ # Python < 3.4 needs to know not to add whitespace when pretty-printing:
+ separators = (', ', ': ') if indent is None else (',', ': ')
# Don't sort if it's already sorted
do_sort = not isinstance(msg, OrderedDict)
- print(json.dumps(msg, sort_keys=do_sort))
+ print(json.dumps(msg, sort_keys=do_sort,
+ indent=indent, separators=separators))
else:
print(msg)
@@ -479,14 +484,14 @@ class VM(qtest.QEMUQtestMachine):
result.append(filter_qmp_event(ev))
return result
- def qmp_log(self, cmd, filters=[], **kwargs):
+ def qmp_log(self, cmd, filters=[], indent=None, **kwargs):
full_cmd = OrderedDict((
("execute", cmd),
("arguments", ordered_kwargs(kwargs))
))
- log(full_cmd, filters)
+ log(full_cmd, filters, indent=indent)
result = self.qmp(cmd, **kwargs)
- log(result, filters)
+ log(result, filters, indent=indent)
return result
def run_job(self, job, auto_finalize=True, auto_dismiss=False):
--
2.17.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 10/11] iotests: implement pretty-print for log and qmp_log
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 10/11] iotests: implement pretty-print for log and qmp_log John Snow
@ 2018-12-20 2:55 ` Eric Blake
2018-12-20 11:29 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Eric Blake @ 2018-12-20 2:55 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block
Cc: vsementsov, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
On 12/19/18 8:29 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 | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v5 10/11] iotests: implement pretty-print for log and qmp_log
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 10/11] iotests: implement pretty-print for log and qmp_log John Snow
2018-12-20 2:55 ` Eric Blake
@ 2018-12-20 11:29 ` Vladimir Sementsov-Ogievskiy
1 sibling, 0 replies; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-20 11:29 UTC (permalink / raw)
To: John Snow, qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Eric Blake, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
20.12.2018 5:29, 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge
2018-12-20 2:29 [Qemu-devel] [PATCH v5 00/11] bitmaps: remove x- prefix from QMP api John Snow
` (9 preceding siblings ...)
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 10/11] iotests: implement pretty-print for log and qmp_log John Snow
@ 2018-12-20 2:29 ` John Snow
2018-12-20 3:02 ` Eric Blake
2018-12-20 12:12 ` Vladimir Sementsov-Ogievskiy
10 siblings, 2 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 2:29 UTC (permalink / raw)
To: qemu-devel, qemu-block
Cc: Eric Blake, vsementsov, Kevin Wolf, John Snow, Max Reitz,
Fam Zheng, Markus Armbruster
New interface, new smoke test.
Signed-off-by: John Snow <jsnow@redhat.com>
---
tests/qemu-iotests/236 | 161 +++++++++++++++++
tests/qemu-iotests/236.out | 351 +++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 513 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..42b93da3ad
--- /dev/null
+++ b/tests/qemu-iotests/236
@@ -0,0 +1,161 @@
+#!/usr/bin/env python
+#
+# Test bitmap merges.
+#
+# Copyright (c) 2018 John Snow for Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+# owner=jsnow@redhat.com
+
+import iotests
+from iotests import log
+
+iotests.verify_image_format(supported_fmts=['generic'])
+size = 64 * 1024 * 1024
+granularity = 64 * 1024
+
+patterns = [("0x5d", "0", "64k"),
+ ("0xd5", "1M", "64k"),
+ ("0xdc", "32M", "64k"),
+ ("0xcd", "0x3ff0000", "64k")] # 64M - 64K
+
+overwrite = [("0xab", "0", "64k"), # Full overwrite
+ ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K)
+ ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K)
+ ("0xea", "0x3fe0000", "64k")] # Adjacent-left (64M - 128K)
+
+def query_bitmaps(vm):
+ res = vm.qmp("query-block")
+ return { "bitmaps": { device['device']: device.get('dirty-bitmaps', []) for
+ device in res['return'] } }
+
+with iotests.FilePath('img') as img_path, \
+ iotests.VM() as vm:
+
+ log('--- Preparing image & VM ---\n')
+ iotests.qemu_img_create('-f', iotests.imgfmt, img_path, str(size))
+ vm.add_drive(img_path)
+ vm.launch()
+
+ log('\n--- Adding preliminary bitmaps A & B ---\n')
+ vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+ name="bitmapA", granularity=granularity)
+ vm.qmp_log("block-dirty-bitmap-add", node="drive0",
+ name="bitmapB", granularity=granularity)
+
+ # Dirties 4 clusters. count=262144
+ log('\n--- Emulating writes ---\n')
+ for p in patterns:
+ cmd = "write -P%s %s %s" % p
+ log(cmd)
+ log(vm.hmp_qemu_io("drive0", cmd))
+
+ log(query_bitmaps(vm), indent=2)
+
+ log('\n--- Submitting Bad Transaction ---\n')
+ vm.qmp_log("transaction", indent=2, actions=[
+ { "type": "block-dirty-bitmap-disable",
+ "data": { "node": "drive0", "name": "bitmapB" }},
+ { "type": "block-dirty-bitmap-add",
+ "data": { "node": "drive0", "name": "bitmapC",
+ "granularity": granularity }},
+ { "type": "block-dirty-bitmap-clear",
+ "data": { "node": "drive0", "name": "bitmapA" }},
+ { "type": "abort", "data": {}}
+ ])
+ log(query_bitmaps(vm), indent=2)
+
+ log('\n--- Disabling B & Adding C ---\n')
+ vm.qmp_log("transaction", indent=2, actions=[
+ { "type": "block-dirty-bitmap-disable",
+ "data": { "node": "drive0", "name": "bitmapB" }},
+ { "type": "block-dirty-bitmap-add",
+ "data": { "node": "drive0", "name": "bitmapC",
+ "granularity": granularity }},
+ # Purely extraneous, but test that it works:
+ { "type": "block-dirty-bitmap-disable",
+ "data": { "node": "drive0", "name": "bitmapC" }},
+ { "type": "block-dirty-bitmap-enable",
+ "data": { "node": "drive0", "name": "bitmapC" }},
+ ])
+
+ log('\n--- 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('\n--- Disabling A & C ---\n')
+ vm.qmp_log("transaction", indent=2, actions=[
+ { "type": "block-dirty-bitmap-disable",
+ "data": { "node": "drive0", "name": "bitmapA" }},
+ { "type": "block-dirty-bitmap-disable",
+ "data": { "node": "drive0", "name": "bitmapC" }}
+ ])
+
+ # A: 7 clusters
+ # B: 4 clusters
+ # C: 6 clusters
+ log(query_bitmaps(vm), indent=2)
+
+ log('\n--- Submitting Bad Merge ---\n')
+ vm.qmp_log("transaction", indent=2, actions=[
+ { "type": "block-dirty-bitmap-add",
+ "data": { "node": "drive0", "name": "bitmapD",
+ "disabled": True, "granularity": granularity }},
+ { "type": "block-dirty-bitmap-merge",
+ "data": { "node": "drive0", "target": "bitmapD",
+ "bitmaps": ["bitmapB", "bitmapC"] }},
+ { "type": "abort", "data": {}}
+ ])
+ log(query_bitmaps(vm), indent=2)
+
+ log('\n--- Creating D as a merge of B & C ---\n')
+ # Good hygiene: create a disabled bitmap as a merge target.
+ vm.qmp_log("transaction", indent=2, actions=[
+ { "type": "block-dirty-bitmap-add",
+ "data": { "node": "drive0", "name": "bitmapD",
+ "disabled": True, "granularity": granularity }},
+ { "type": "block-dirty-bitmap-merge",
+ "data": { "node": "drive0", "target": "bitmapD",
+ "bitmaps": ["bitmapB", "bitmapC"] }}
+ ])
+
+ # A and D should now both have 7 clusters apiece.
+ # B and C remain unchanged with 4 and 6 respectively.
+ log(query_bitmaps(vm), indent=2)
+
+ # A and D should be equivalent.
+ # Some formats round the size of the disk, so don't print the checksums.
+ check_a = vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node="drive0", name="bitmapA")['return']['sha256']
+ check_b = vm.qmp('x-debug-block-dirty-bitmap-sha256',
+ node="drive0", name="bitmapD")['return']['sha256']
+ assert(check_a == check_b)
+
+ log('\n--- Removing bitmaps A, B, C, and D ---\n')
+ vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapA")
+ vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapB")
+ vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapC")
+ vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapD")
+
+ log('\n--- Final Query ---\n')
+ log(query_bitmaps(vm), indent=2)
+
+ log('\n--- Done ---\n')
+ vm.shutdown()
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
new file mode 100644
index 0000000000..1934035795
--- /dev/null
+++ b/tests/qemu-iotests/236.out
@@ -0,0 +1,351 @@
+--- Preparing image & VM ---
+
+
+--- Adding preliminary bitmaps A & B ---
+
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmapA", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-add", "arguments": {"granularity": 65536, "name": "bitmapB", "node": "drive0"}}
+{"return": {}}
+
+--- Emulating writes ---
+
+write -P0x5d 0 64k
+{"return": ""}
+write -P0xd5 1M 64k
+{"return": ""}
+write -P0xdc 32M 64k
+{"return": ""}
+write -P0xcd 0x3ff0000 64k
+{"return": ""}
+{
+ "bitmaps": {
+ "drive0": [
+ {
+ "count": 262144,
+ "granularity": 65536,
+ "name": "bitmapB",
+ "status": "active"
+ },
+ {
+ "count": 262144,
+ "granularity": 65536,
+ "name": "bitmapA",
+ "status": "active"
+ }
+ ]
+ }
+}
+
+--- Submitting Bad Transaction ---
+
+{
+ "execute": "transaction",
+ "arguments": {
+ "actions": [
+ {
+ "data": {
+ "node": "drive0",
+ "name": "bitmapB"
+ },
+ "type": "block-dirty-bitmap-disable"
+ },
+ {
+ "data": {
+ "node": "drive0",
+ "name": "bitmapC",
+ "granularity": 65536
+ },
+ "type": "block-dirty-bitmap-add"
+ },
+ {
+ "data": {
+ "node": "drive0",
+ "name": "bitmapA"
+ },
+ "type": "block-dirty-bitmap-clear"
+ },
+ {
+ "data": {},
+ "type": "abort"
+ }
+ ]
+ }
+}
+{
+ "error": {
+ "class": "GenericError",
+ "desc": "Transaction aborted using Abort action"
+ }
+}
+{
+ "bitmaps": {
+ "drive0": [
+ {
+ "count": 262144,
+ "granularity": 65536,
+ "name": "bitmapB",
+ "status": "active"
+ },
+ {
+ "count": 262144,
+ "granularity": 65536,
+ "name": "bitmapA",
+ "status": "active"
+ }
+ ]
+ }
+}
+
+--- Disabling B & Adding C ---
+
+{
+ "execute": "transaction",
+ "arguments": {
+ "actions": [
+ {
+ "data": {
+ "node": "drive0",
+ "name": "bitmapB"
+ },
+ "type": "block-dirty-bitmap-disable"
+ },
+ {
+ "data": {
+ "node": "drive0",
+ "name": "bitmapC",
+ "granularity": 65536
+ },
+ "type": "block-dirty-bitmap-add"
+ },
+ {
+ "data": {
+ "node": "drive0",
+ "name": "bitmapC"
+ },
+ "type": "block-dirty-bitmap-disable"
+ },
+ {
+ "data": {
+ "node": "drive0",
+ "name": "bitmapC"
+ },
+ "type": "block-dirty-bitmap-enable"
+ }
+ ]
+ }
+}
+{
+ "return": {}
+}
+
+--- Emulating further writes ---
+
+write -P0xab 0 64k
+{"return": ""}
+write -P0xad 0x00f8000 64k
+{"return": ""}
+write -P0x1d 0x2008000 64k
+{"return": ""}
+write -P0xea 0x3fe0000 64k
+{"return": ""}
+
+--- Disabling A & C ---
+
+{
+ "execute": "transaction",
+ "arguments": {
+ "actions": [
+ {
+ "data": {
+ "node": "drive0",
+ "name": "bitmapA"
+ },
+ "type": "block-dirty-bitmap-disable"
+ },
+ {
+ "data": {
+ "node": "drive0",
+ "name": "bitmapC"
+ },
+ "type": "block-dirty-bitmap-disable"
+ }
+ ]
+ }
+}
+{
+ "return": {}
+}
+{
+ "bitmaps": {
+ "drive0": [
+ {
+ "count": 393216,
+ "granularity": 65536,
+ "name": "bitmapC",
+ "status": "disabled"
+ },
+ {
+ "count": 262144,
+ "granularity": 65536,
+ "name": "bitmapB",
+ "status": "disabled"
+ },
+ {
+ "count": 458752,
+ "granularity": 65536,
+ "name": "bitmapA",
+ "status": "disabled"
+ }
+ ]
+ }
+}
+
+--- Submitting Bad Merge ---
+
+{
+ "execute": "transaction",
+ "arguments": {
+ "actions": [
+ {
+ "data": {
+ "node": "drive0",
+ "disabled": true,
+ "name": "bitmapD",
+ "granularity": 65536
+ },
+ "type": "block-dirty-bitmap-add"
+ },
+ {
+ "data": {
+ "node": "drive0",
+ "target": "bitmapD",
+ "bitmaps": [
+ "bitmapB",
+ "bitmapC"
+ ]
+ },
+ "type": "block-dirty-bitmap-merge"
+ },
+ {
+ "data": {},
+ "type": "abort"
+ }
+ ]
+ }
+}
+{
+ "error": {
+ "class": "GenericError",
+ "desc": "Transaction aborted using Abort action"
+ }
+}
+{
+ "bitmaps": {
+ "drive0": [
+ {
+ "count": 393216,
+ "granularity": 65536,
+ "name": "bitmapC",
+ "status": "disabled"
+ },
+ {
+ "count": 262144,
+ "granularity": 65536,
+ "name": "bitmapB",
+ "status": "disabled"
+ },
+ {
+ "count": 458752,
+ "granularity": 65536,
+ "name": "bitmapA",
+ "status": "disabled"
+ }
+ ]
+ }
+}
+
+--- Creating D as a merge of B & C ---
+
+{
+ "execute": "transaction",
+ "arguments": {
+ "actions": [
+ {
+ "data": {
+ "node": "drive0",
+ "disabled": true,
+ "name": "bitmapD",
+ "granularity": 65536
+ },
+ "type": "block-dirty-bitmap-add"
+ },
+ {
+ "data": {
+ "node": "drive0",
+ "target": "bitmapD",
+ "bitmaps": [
+ "bitmapB",
+ "bitmapC"
+ ]
+ },
+ "type": "block-dirty-bitmap-merge"
+ }
+ ]
+ }
+}
+{
+ "return": {}
+}
+{
+ "bitmaps": {
+ "drive0": [
+ {
+ "count": 458752,
+ "granularity": 65536,
+ "name": "bitmapD",
+ "status": "disabled"
+ },
+ {
+ "count": 393216,
+ "granularity": 65536,
+ "name": "bitmapC",
+ "status": "disabled"
+ },
+ {
+ "count": 262144,
+ "granularity": 65536,
+ "name": "bitmapB",
+ "status": "disabled"
+ },
+ {
+ "count": 458752,
+ "granularity": 65536,
+ "name": "bitmapA",
+ "status": "disabled"
+ }
+ ]
+ }
+}
+
+--- Removing bitmaps A, B, C, and D ---
+
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmapA", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmapB", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmapC", "node": "drive0"}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-remove", "arguments": {"name": "bitmapD", "node": "drive0"}}
+{"return": {}}
+
+--- Final Query ---
+
+{
+ "bitmaps": {
+ "drive0": []
+ }
+}
+
+--- Done ---
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 61a6d98ebd..f6b245917a 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
--
2.17.2
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge John Snow
@ 2018-12-20 3:02 ` Eric Blake
2018-12-20 20:58 ` John Snow
2018-12-20 12:12 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 31+ messages in thread
From: Eric Blake @ 2018-12-20 3:02 UTC (permalink / raw)
To: John Snow, qemu-devel, qemu-block
Cc: vsementsov, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
On 12/19/18 8:29 PM, John Snow wrote:
> New interface, new smoke test.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> tests/qemu-iotests/236 | 161 +++++++++++++++++
> tests/qemu-iotests/236.out | 351 +++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 513 insertions(+)
> create mode 100755 tests/qemu-iotests/236
> create mode 100644 tests/qemu-iotests/236.out
>
Reviewed-by: Eric Blake <eblake@redhat.com>
(and glad that my insistence on beefing up the test has caught bugs)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge
2018-12-20 3:02 ` Eric Blake
@ 2018-12-20 20:58 ` John Snow
0 siblings, 0 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 20:58 UTC (permalink / raw)
To: Eric Blake, qemu-devel, qemu-block
Cc: Kevin Wolf, Fam Zheng, vsementsov, Markus Armbruster, Max Reitz
On 12/19/18 10:02 PM, Eric Blake wrote:
> On 12/19/18 8:29 PM, John Snow wrote:
>> New interface, new smoke test.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> tests/qemu-iotests/236 | 161 +++++++++++++++++
>> tests/qemu-iotests/236.out | 351 +++++++++++++++++++++++++++++++++++++
>> tests/qemu-iotests/group | 1 +
>> 3 files changed, 513 insertions(+)
>> create mode 100755 tests/qemu-iotests/236
>> create mode 100644 tests/qemu-iotests/236.out
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> (and glad that my insistence on beefing up the test has caught bugs)
>
Me too. Sorry to have been lazy about it. Your suggestions are always
worth following.
--js
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge
2018-12-20 2:29 ` [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge John Snow
2018-12-20 3:02 ` Eric Blake
@ 2018-12-20 12:12 ` Vladimir Sementsov-Ogievskiy
2018-12-20 20:53 ` John Snow
1 sibling, 1 reply; 31+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2018-12-20 12:12 UTC (permalink / raw)
To: John Snow, qemu-devel@nongnu.org, qemu-block@nongnu.org
Cc: Eric Blake, Kevin Wolf, Max Reitz, Fam Zheng, Markus Armbruster
20.12.2018 5:29, John Snow wrote:
> New interface, new smoke test.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
[...]
> + # A: 7 clusters
> + # B: 4 clusters
> + # C: 6 clusters
> + log(query_bitmaps(vm), indent=2)
> +
> + log('\n--- Submitting Bad Merge ---\n')
aha, spent some time, trying to understand, what is bad with merge, until understand that
that is abort. I didn't sleep enough last night, but anyway, 'Aborting Merge Transaction'
is a bit clearer, I think.
> + vm.qmp_log("transaction", indent=2, actions=[
> + { "type": "block-dirty-bitmap-add",
> + "data": { "node": "drive0", "name": "bitmapD",
> + "disabled": True, "granularity": granularity }},
> + { "type": "block-dirty-bitmap-merge",
> + "data": { "node": "drive0", "target": "bitmapD",
> + "bitmaps": ["bitmapB", "bitmapC"] }},
> + { "type": "abort", "data": {}}
> + ])
> + log(query_bitmaps(vm), indent=2)
> +
> + log('\n--- Creating D as a merge of B & C ---\n')
> + # Good hygiene: create a disabled bitmap as a merge target.
> + vm.qmp_log("transaction", indent=2, actions=[
> + { "type": "block-dirty-bitmap-add",
> + "data": { "node": "drive0", "name": "bitmapD",
> + "disabled": True, "granularity": granularity }},
> + { "type": "block-dirty-bitmap-merge",
> + "data": { "node": "drive0", "target": "bitmapD",
> + "bitmaps": ["bitmapB", "bitmapC"] }}
> + ])
> +
> + # A and D should now both have 7 clusters apiece.
> + # B and C remain unchanged with 4 and 6 respectively.
> + log(query_bitmaps(vm), indent=2)
> +
> + # A and D should be equivalent.
> + # Some formats round the size of the disk, so don't print the checksums.
Just interested: round 64M? to what?
> + check_a = vm.qmp('x-debug-block-dirty-bitmap-sha256',
> + node="drive0", name="bitmapA")['return']['sha256']
> + check_b = vm.qmp('x-debug-block-dirty-bitmap-sha256',
> + node="drive0", name="bitmapD")['return']['sha256']
> + assert(check_a == check_b)
hmm, a funny suggestion: s/check_b/check_d/
> +
> + log('\n--- Removing bitmaps A, B, C, and D ---\n')
what about failed transaction with remove command, for a full kit?
> + vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapA")
> + vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapB")
> + vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapC")
> + vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapD")
> +
> + log('\n--- Final Query ---\n')
> + log(query_bitmaps(vm), indent=2)
> +
> + log('\n--- Done ---\n')
> + vm.shutdown()
with or without any of my suggestions:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge
2018-12-20 12:12 ` Vladimir Sementsov-Ogievskiy
@ 2018-12-20 20:53 ` John Snow
0 siblings, 0 replies; 31+ messages in thread
From: John Snow @ 2018-12-20 20:53 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-devel@nongnu.org,
qemu-block@nongnu.org
Cc: Kevin Wolf, Fam Zheng, Markus Armbruster, Max Reitz
On 12/20/18 7:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.12.2018 5:29, John Snow wrote:
>> New interface, new smoke test.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>
> [...]
>
>> + # A: 7 clusters
>> + # B: 4 clusters
>> + # C: 6 clusters
>> + log(query_bitmaps(vm), indent=2)
>> +
>> + log('\n--- Submitting Bad Merge ---\n')
>
> aha, spent some time, trying to understand, what is bad with merge, until understand that
> that is abort. I didn't sleep enough last night, but anyway, 'Aborting Merge Transaction'
> is a bit clearer, I think.
>
Sure, I'll rephrase it: "Submitting & Aborting Merge Transaction"
>> + vm.qmp_log("transaction", indent=2, actions=[
>> + { "type": "block-dirty-bitmap-add",
>> + "data": { "node": "drive0", "name": "bitmapD",
>> + "disabled": True, "granularity": granularity }},
>> + { "type": "block-dirty-bitmap-merge",
>> + "data": { "node": "drive0", "target": "bitmapD",
>> + "bitmaps": ["bitmapB", "bitmapC"] }},
>> + { "type": "abort", "data": {}}
>> + ])
>> + log(query_bitmaps(vm), indent=2)
>> +
>> + log('\n--- Creating D as a merge of B & C ---\n')
>> + # Good hygiene: create a disabled bitmap as a merge target.
>> + vm.qmp_log("transaction", indent=2, actions=[
>> + { "type": "block-dirty-bitmap-add",
>> + "data": { "node": "drive0", "name": "bitmapD",
>> + "disabled": True, "granularity": granularity }},
>> + { "type": "block-dirty-bitmap-merge",
>> + "data": { "node": "drive0", "target": "bitmapD",
>> + "bitmaps": ["bitmapB", "bitmapC"] }}
>> + ])
>> +
>> + # A and D should now both have 7 clusters apiece.
>> + # B and C remain unchanged with 4 and 6 respectively.
>> + log(query_bitmaps(vm), indent=2)
>> +
>> + # A and D should be equivalent.
>> + # Some formats round the size of the disk, so don't print the checksums.
>
> Just interested: round 64M? to what?
>
VPC does weird stuff. If you ask for 64M you get 64M+16K. "round" is
maybe a bad adjective here, but VPC really won't give you what you ask
for. Loosening the restriction to "generic" was a good idea.
>> + check_a = vm.qmp('x-debug-block-dirty-bitmap-sha256',
>> + node="drive0", name="bitmapA")['return']['sha256']
>> + check_b = vm.qmp('x-debug-block-dirty-bitmap-sha256',
>> + node="drive0", name="bitmapD")['return']['sha256']
>> + assert(check_a == check_b)
>
> hmm, a funny suggestion: s/check_b/check_d/
Oh, yes, that would be better.
>
>> +
>> + log('\n--- Removing bitmaps A, B, C, and D ---\n')
>
> what about failed transaction with remove command, for a full kit?
>
Remove isn't transactionable!
>> + vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapA")
>> + vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapB")
>> + vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapC")
>> + vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapD")
>> +
>> + log('\n--- Final Query ---\n')
>> + log(query_bitmaps(vm), indent=2)
>> +
>> + log('\n--- Done ---\n')
>> + vm.shutdown()
>
>
> with or without any of my suggestions:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
Thanks!
--js
^ permalink raw reply [flat|nested] 31+ messages in thread