bitmaps: remove x- prefix from QMP api
v2 cover bitmaps: remove x- prefix from QMP api
Touch up a few last things and remove the x- prefix.
V2:
- Adding R-Bs from Eric and Vladimir.
- A few trivial patches for iotests...
- Added quick smoke test.
John Snow (7):
blockdev: abort transactions in reverse order
blockdev: n-ary bitmap merge
block: remove 'x' prefix from experimental bitmap APIs iotests.py: don't abort if IMGKEYSECRET is undefined
iotests: add filter_generated_node_ids
iotests: allow pretty-print for qmp_log
iotests: add iotest 236 for testing bitmap merge
blockdev.c | 96 +++++++----- qapi/block-core.json | 56 +++---- qapi/transaction.json | 12 +- tests/qemu-iotests/223 | 4 +- tests/qemu-iotests/236 | 123 ++++++++++++++++ tests/qemu-iotests/236.out | 265 ++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + tests/qemu-iotests/iotests.py | 9 +- 8 files changed, 489 insertions(+), 77 deletions(-) create mode 100755 tests/qemu-iotests/236 create mode 100644 tests/qemu-iotests/236.out
Signature · 1 line
2.17.2
PATCH v2 1/7 blockdev: abort transactions in reverse order
Presently, we abort transactions in the same order they were processed in. Bitmap commands, though, attempt to restore backup data structures on abort. To that end, though, they need to be aborted in reverse chronological order.
Replace the QSIMPLEQ data structure with a QTAILQ one, so we can iterate in reverse for the abort phase of the transaction.
--- blockdev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
blockdev.c +7 -7
@@ -1341,7 +1341,7 @@ struct BlkActionState {const BlkActionOps *ops;JobTxn *block_job_txn;TransactionProperties *txn_props; -QSIMPLEQ_ENTRY (BlkActionState) entry; +QTAILQ_ENTRY (BlkActionState) entry; };/* internal snapshot private data */
@@ -2269,8 +2269,8 @@ void qmp_transaction(TransactionActionList *dev_list,BlkActionState *state, *next;Error *local_err =NULL ; - QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states; -QSIMPLEQ_INIT (&snap_bdrv_states); + QTAILQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states; +QTAILQ_INIT (&snap_bdrv_states);/* Does this transaction get canceled as a group on failure? * If not, we don't really need to make a JobTxn.
@@ -2301,7 +2301,7 @@ void qmp_transaction(TransactionActionList *dev_list, state->action = dev_info; state->block_job_txn = block_job_txn; state->txn_props = props; -QSIMPLEQ_INSERT_TAIL (&snap_bdrv_states, state, entry); +QTAILQ_INSERT_TAIL (&snap_bdrv_states, state, entry); state->ops->prepare (state, &local_err);if (local_err) {
@@ -2310,7 +2310,7 @@ void qmp_transaction(TransactionActionList *dev_list, } } - QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) { + QTAILQ_FOREACH(state, &snap_bdrv_states, entry) {if (state->ops->commit) { state->ops->commit (state); }
@@ -2321,13 +2321,13 @@ void qmp_transaction(TransactionActionList *dev_list, delete_and_fail:/* failure, and it is all-or-none; roll back all operations */ - QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) { + QTAILQ_FOREACH_REVERSE(state, &snap_bdrv_states, snap_bdrv_states, entry) {if (state->ops->abort) { state->ops->abort (state); } } exit: - QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) { + QTAILQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {if (state->ops->clean) { state->ops->clean (state); }
Signature · 1 line
2.17.2
PATCH v2 2/7 blockdev: n-ary bitmap merge
In reply to John SnowEspecially 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.
--- blockdev.c | 64 +++++++++++++++++++++++++++++--------------- qapi/block-core.json | 22 +++++++-------- 2 files changed, 53 insertions(+), 33 deletions(-)
blockdev.c +42 -22
@@ -2122,33 +2122,26 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common) } } +void do_block_dirty_bitmap_merge (const char *node,const char *target, +strList *bitmaps,HBitmap **backup, +Error **errp); +static void block_dirty_bitmap_merge_prepare (BlkActionState *common,Error **errp) {BlockDirtyBitmapMerge *action;BlockDirtyBitmapState *state =DO_UPCAST (BlockDirtyBitmapState, common, common); -BdrvDirtyBitmap *merge_source;if (action_check_completion_mode (common, errp) <0 ) {return ; } action = common->action->u.x_block_dirty_bitmap_merge.data; - state->bitmap =block_dirty_bitmap_lookup (action->node, - action->dst_name, - &state->bs, - errp); -if (!state->bitmap) { -return ; - } - merge_source =bdrv_find_dirty_bitmap (state->bs, action->src_name); -if (!merge_source) { -return ; - } - -bdrv_merge_dirty_bitmap (state->bitmap, merge_source, &state->backup, errp); +do_block_dirty_bitmap_merge (action->node, action->target, + action->bitmaps, &state->backup, + errp); }static void abort_prepare (BlkActionState *common,Error **errp)
@@ -2980,24 +2973,51 @@ void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name,bdrv_disable_dirty_bitmap (bitmap); } -void qmp_x_block_dirty_bitmap_merge (const char *node,const char *dst_name, -const char *src_name,Error **errp) +void do_block_dirty_bitmap_merge (const char *node,const char *target, +strList *bitmaps,HBitmap **backup, +Error **errp) {BlockDriverState *bs; -BdrvDirtyBitmap *dst, *src; +BdrvDirtyBitmap *dst, *src, *anon; +strList *lst; +Error *local_err =NULL ; - dst =block_dirty_bitmap_lookup (node, dst_name, &bs, errp); + dst =block_dirty_bitmap_lookup (node, target, &bs, errp);if (!dst) {return ; } - src =bdrv_find_dirty_bitmap (bs, src_name); -if (!src) { -error_setg (errp,"Dirty bitmap '%s' not found" , src_name); + anon =bdrv_create_dirty_bitmap (bs,bdrv_dirty_bitmap_granularity (dst), +NULL , errp); +if (!anon) {return ; } -bdrv_merge_dirty_bitmap (dst, src,NULL , errp); +for (lst = bitmaps; lst; lst = lst->next) { + src =bdrv_find_dirty_bitmap (bs, lst->value); +if (!src) { +error_setg (errp,"Dirty bitmap '%s' not found" , lst->value); + goto out; + } + +bdrv_merge_dirty_bitmap (anon, src,NULL , &local_err); +if (local_err) { +error_propagate (errp, local_err); + goto out; + } + } + +/* Merge into dst; dst is unchanged on failure. */ +bdrv_merge_dirty_bitmap (dst, anon, backup, errp); + + out: +bdrv_release_dirty_bitmap (bs, anon); +} + +void qmp_x_block_dirty_bitmap_merge (const char *node,const char *target, +strList *bitmaps,Error **errp) +{ +do_block_dirty_bitmap_merge (node, target, bitmaps,NULL , errp); } BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
qapi/block-core.json +11 -11
@@ -1818,14 +1818,14 @@ # # @node: name of device/node which the bitmap is tracking # -# @dst_name: name of the destination dirty bitmap +# @target: name of the destination dirty bitmap # -# @src_name: name of the source dirty bitmap +# @bitmaps: name(s) of the source dirty bitmap(s) # # Since: 3.0 ## { 'struct': 'BlockDirtyBitmapMerge', - 'data': { 'node': 'str', 'dst_name': 'str', 'src_name': 'str' } } + 'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } } ## # @block-dirty-bitmap-add:
@@ -1940,23 +1940,23 @@ ## # @x-block-dirty-bitmap-merge: # -# FIXME: Rename @src_name and @dst_name to src-name and dst-name. -# -# Merge @src_name dirty bitmap to @dst_name dirty bitmap. @src_name dirty -# bitmap is unchanged. On error, @dst_name is unchanged. +# Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap. +# The @bitmaps dirty bitmaps are unchanged. +# On error, @target is unchanged. # # Returns: nothing on success # If @node is not a valid block device, DeviceNotFound -# If @dst_name or @src_name is not found, GenericError -# If bitmaps has different sizes or granularities, GenericError +# If any bitmap in @bitmaps or @target is not found, GenericError +# If any of the bitmaps have different sizes or granularities, +# GenericError # # Since: 3.0 # # Example: # # -> { "execute": "x-block-dirty-bitmap-merge", -# "arguments": { "node": "drive0", "dst_name": "bitmap0", -# "src_name": "bitmap1" } } +# "arguments": { "node": "drive0", "target": "bitmap0", +# "bitmaps": ["bitmap1"] } } # <- { "return": {} } # ##
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 2/7] blockdev: n-ary bitmap merge
Quoted 11 lines · John Snow
Especially outside of transactions, it is helpful to provide all-or-nothing semantics for bitmap merges. This facilitates the coalescing of multiple bitmaps into a single target for the "checkpoint" interpretation when assembling bitmaps that represent arbitrary points in time from component bitmaps.
This is an incompatible change from the preliminary version of the API.
Signed-off-by John Snow <[email protected]>Reviewed-by Eric Blake <[email protected]>
Signature · 2 lines
Best regards, Vladimir
PATCH v2 3/7 block: remove 'x' prefix from experimental bitmap APIs
In reply to John SnowThe '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.
--- blockdev.c | 22 +++++++++++----------- qapi/block-core.json | 34 +++++++++++++++++----------------- qapi/transaction.json | 12 ++++++------ tests/qemu-iotests/223 | 4 ++-- 4 files changed, 36 insertions(+), 36 deletions(-)
blockdev.c +11 -11
@@ -1966,7 +1966,7 @@ static void block_dirty_bitmap_add_prepare(BlkActionState *common, action->has_granularity, action->granularity, action->has_persistent, action->persistent, action->has_autoload, action->autoload, - action->has_x_disabled, action->x_disabled, + action->has_disabled, action->disabled, &local_err);if (!local_err) {
@@ -2051,7 +2051,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common,return ; } - action = common->action->u.x_block_dirty_bitmap_enable.data; + action = common->action->u.block_dirty_bitmap_enable.data; state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, NULL,
@@ -2092,7 +2092,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common,return ; } - action = common->action->u.x_block_dirty_bitmap_disable.data; + action = common->action->u.block_dirty_bitmap_disable.data; state->bitmap = block_dirty_bitmap_lookup(action->node, action->name, NULL,
@@ -2137,7 +2137,7 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,return ; } - action = common->action->u.x_block_dirty_bitmap_merge.data; + action = common->action->u.block_dirty_bitmap_merge.data; do_block_dirty_bitmap_merge(action->node, action->target, action->bitmaps, &state->backup,
@@ -2205,17 +2205,17 @@ static const BlkActionOps actions[] = { .commit = block_dirty_bitmap_free_backup, .abort = block_dirty_bitmap_restore, }, - [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_ENABLE] = { + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = { .instance_size = sizeof(BlockDirtyBitmapState), .prepare = block_dirty_bitmap_enable_prepare, .abort = block_dirty_bitmap_enable_abort, }, - [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_DISABLE] = { + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = { .instance_size = sizeof(BlockDirtyBitmapState), .prepare = block_dirty_bitmap_disable_prepare, .abort = block_dirty_bitmap_disable_abort, }, - [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = { + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_MERGE] = { .instance_size = sizeof(BlockDirtyBitmapState), .prepare = block_dirty_bitmap_merge_prepare, .commit = block_dirty_bitmap_free_backup,
@@ -2931,7 +2931,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name,bdrv_clear_dirty_bitmap (bitmap,NULL ); } -void qmp_x_block_dirty_bitmap_enable (const char *node,const char *name, +void qmp_block_dirty_bitmap_enable (const char *node,const char *name,Error **errp) {BlockDriverState *bs;
@@ -2952,7 +2952,7 @@ void qmp_x_block_dirty_bitmap_enable(const char *node, const char *name,bdrv_enable_dirty_bitmap (bitmap); } -void qmp_x_block_dirty_bitmap_disable (const char *node,const char *name, +void qmp_block_dirty_bitmap_disable (const char *node,const char *name,Error **errp) {BlockDriverState *bs;
@@ -3014,8 +3014,8 @@ void do_block_dirty_bitmap_merge(const char *node, const char *target,bdrv_release_dirty_bitmap (bs, anon); } -void qmp_x_block_dirty_bitmap_merge (const char *node,const char *target, -strList *bitmaps,Error **errp) +void qmp_block_dirty_bitmap_merge (const char *node,const char *target, +strList *bitmaps,Error **errp) {do_block_dirty_bitmap_merge (node, target, bitmaps,NULL , errp); }
qapi/block-core.json +17 -17
@@ -1803,15 +1803,15 @@ # Currently, all dirty tracking bitmaps are loaded from Qcow2 on # open. # -# @x-disabled: the bitmap is created in the disabled state, which means that -# it will not track drive changes. The bitmap may be enabled with -# x-block-dirty-bitmap-enable. Default is false. (Since: 3.0) +# @disabled: the bitmap is created in the disabled state, which means that +# it will not track drive changes. The bitmap may be enabled with +# block-dirty-bitmap-enable. Default is false. (Since: 4.0) # # Since: 2.4 ## { 'struct': 'BlockDirtyBitmapAdd', 'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32', - '*persistent': 'bool', '*autoload': 'bool', '*x-disabled': 'bool' } } + '*persistent': 'bool', '*autoload': 'bool', '*disabled': 'bool' } } ## # @BlockDirtyBitmapMerge:
@@ -1822,7 +1822,7 @@ # # @bitmaps: name(s) of the source dirty bitmap(s) # -# Since: 3.0 +# Since: 4.0 ## { 'struct': 'BlockDirtyBitmapMerge', 'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
@@ -1896,7 +1896,7 @@ 'data': 'BlockDirtyBitmap' } ## -# @x-block-dirty-bitmap-enable: +# @block-dirty-bitmap-enable: # # Enables a dirty bitmap so that it will begin tracking disk changes. #
@@ -1904,20 +1904,20 @@ # If @node is not a valid block device, DeviceNotFound # If @name is not found, GenericError with an explanation # -# Since: 3.0 +# Since: 4.0 # # Example: # -# -> { "execute": "x-block-dirty-bitmap-enable", +# -> { "execute": "block-dirty-bitmap-enable", # "arguments": { "node": "drive0", "name": "bitmap0" } } # <- { "return": {} } # ## - { 'command': 'x-block-dirty-bitmap-enable', + { 'command': 'block-dirty-bitmap-enable', 'data': 'BlockDirtyBitmap' } ## -# @x-block-dirty-bitmap-disable: +# @block-dirty-bitmap-disable: # # Disables a dirty bitmap so that it will stop tracking disk changes. #
@@ -1925,20 +1925,20 @@ # If @node is not a valid block device, DeviceNotFound # If @name is not found, GenericError with an explanation # -# Since: 3.0 +# Since: 4.0 # # Example: # -# -> { "execute": "x-block-dirty-bitmap-disable", +# -> { "execute": "block-dirty-bitmap-disable", # "arguments": { "node": "drive0", "name": "bitmap0" } } # <- { "return": {} } # ## - { 'command': 'x-block-dirty-bitmap-disable', + { 'command': 'block-dirty-bitmap-disable', 'data': 'BlockDirtyBitmap' } ## -# @x-block-dirty-bitmap-merge: +# @block-dirty-bitmap-merge: # # Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap. # The @bitmaps dirty bitmaps are unchanged.
@@ -1950,17 +1950,17 @@ # If any of the bitmaps have different sizes or granularities, # GenericError # -# Since: 3.0 +# Since: 4.0 # # Example: # -# -> { "execute": "x-block-dirty-bitmap-merge", +# -> { "execute": "block-dirty-bitmap-merge", # "arguments": { "node": "drive0", "target": "bitmap0", # "bitmaps": ["bitmap1"] } } # <- { "return": {} } # ## - { 'command': 'x-block-dirty-bitmap-merge', + { 'command': 'block-dirty-bitmap-merge', 'data': 'BlockDirtyBitmapMerge' } ##
qapi/transaction.json +6 -6
@@ -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',
tests/qemu-iotests/223 +2 -2
@@ -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",
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 3/7] block: remove 'x' prefix from experimental bitmap APIs
Quoted 6 lines · John Snow
The 'x' prefix was added because I was uncertain of the direction we'd take for the libvirt API. With the general approach solidified, I feel comfortable committing to this API for 4.0.
Signed-off-by John Snow<[email protected]>Reviewed-by Eric Blake<[email protected]>
Signature · 2 lines
Best regards, Vladimir
PATCH v2 4/7 iotests.py: don't abort if IMGKEYSECRET is undefined
In reply to John SnowInstead 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.
--- tests/qemu-iotests/iotests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
tests/qemu-iotests/iotests.py +1 -1
@@ -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'
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 4/7] iotests.py: don't abort if IMGKEYSECRET is undefined
Quoted 10 lines · John Snow
Instead of using os.environ[], use .get with a default of empty string to match the setup in check to allow us to import the iotests module (for debugging, say) without needing a crafted environment just to import the module.
Signed-off-by John Snow <[email protected]>--- tests/qemu-iotests/iotests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 14 lines
tests/qemu-iotests/iotests.py +1
-1@@ -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'
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 4/7] iotests.py: don't abort if IMGKEYSECRET is undefined
In reply to John SnowQuoted 6 lines · John Snow
Instead of using os.environ[], use .get with a default of empty string to match the setup in check to allow us to import the iotests module (for debugging, say) without needing a crafted environment just to import the module.
Signed-off-by John Snow <[email protected]>
Quoted 18 lines
--- tests/qemu-iotests/iotests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)tests/qemu-iotests/iotests.py +1
-1@@ -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'
Signature · 2 lines
Best regards, Vladimir
PATCH v2 5/7 iotests: add filter_generated_node_ids
In reply to John SnowTo mimic the common filter of the same name, but for the python tests.
--- tests/qemu-iotests/iotests.py | 3 +++ 1 file changed, 3 insertions(+)
tests/qemu-iotests/iotests.py +3 -0
@@ -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'):
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 5/7] iotests: add filter_generated_node_ids
Quoted 6 lines · John Snow
To mimic the common filter of the same name, but for the python tests.
Signed-off-by John Snow <[email protected]>--- tests/qemu-iotests/iotests.py | 3 +++ 1 file changed, 3 insertions(+)
Quoted 16 lines
tests/qemu-iotests/iotests.py +3
-0@@ -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'):
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 5/7] iotests: add filter_generated_node_ids
In reply to John SnowQuoted 3 lines · John Snow
To mimic the common filter of the same name, but for the python tests.
Signed-off-by John Snow <[email protected]>
Quoted 19 lines
--- tests/qemu-iotests/iotests.py | 3 +++ 1 file changed, 3 insertions(+)tests/qemu-iotests/iotests.py +3
-0@@ -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'):
Signature · 2 lines
Best regards, Vladimir
PATCH v2 6/7 iotests: allow pretty-print for qmp_log
In reply to John SnowIf iotests have lines exceeding >998 characters long, git doesn't want to send it plaintext to the list. We can solve this by allowing the iotests to use pretty printed QMP output that we can match against instead.
As a bonus, it's much nicer for human eyes, too.
--- tests/qemu-iotests/iotests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
tests/qemu-iotests/iotests.py +2 -2
@@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine): result.append(filter_qmp_event(ev)) return result - def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs): + def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs): logmsg = '{"execute": "%s", "arguments": %s}' % \ (cmd, json.dumps(kwargs, sort_keys=True)) log(logmsg, filters) result = self.qmp(cmd, **kwargs) - log(json.dumps(result, sort_keys=True), filters) + log(json.dumps(result, indent=indent, sort_keys=True), filters) return result def run_job(self, job, auto_finalize=True, auto_dismiss=False):
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log
Quoted 17 lines · John Snow
If iotests have lines exceeding >998 characters long, git doesn't want to send it plaintext to the list. We can solve this by allowing the iotests to use pretty printed QMP output that we can match against instead.
As a bonus, it's much nicer for human eyes, too.
Signed-off-by John Snow <[email protected]>--- tests/qemu-iotests/iotests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)tests/qemu-iotests/iotests.py +0
-0
tests/qemu-iotests/iotests.py class VM(qtest.QEMUQtestMachine): · line 450
Quoted 5 lines
tests/qemu-iotests/iotests.py +1
-1@@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine): result.append(filter_qmp_event(ev)) return result - def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs): + def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):
Why None instead of False?
tests/qemu-iotests/iotests.py class VM(qtest.QEMUQtestMachine): · line 455
Quoted 6 lines
tests/qemu-iotests/iotests.py +1
-1logmsg = '{"execute": "%s", "arguments": %s}' % \ (cmd, json.dumps(kwargs, sort_keys=True)) log(logmsg, filters) result = self.qmp(cmd, **kwargs) - log(json.dumps(result, sort_keys=True), filters) + log(json.dumps(result, indent=indent, sort_keys=True), filters)
But I'd actually have to read the contract to json.dumps() to learn what is expected.
/me goes and does that...
https://docs.python.org/2/library/json.html
If indent is a non-negative integer, then JSON array elements and object members will be pretty-printed with that indent level. An indent level of 0, or negative, will only insert newlines. None (the default) selects the most compact representation.
Okay, makes sense.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log
Quoted 40 lines · Eric Blake
Quoted 19 lines · John Snow
If iotests have lines exceeding >998 characters long, git doesn't want to send it plaintext to the list. We can solve this by allowing the iotests to use pretty printed QMP output that we can match against instead.
As a bonus, it's much nicer for human eyes, too.
Signed-off-by John Snow <[email protected]>---
tests/qemu-iotests/iotests.py| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)tests/qemu-iotests/iotests.py +0
-0@@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine): result.append(filter_qmp_event(ev))Quoted 4 lines
return result
- defqmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
+ defqmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs):Why None instead of False?
Quoted 6 lines
logmsg = '{"execute": "%s", "arguments": %s}' % \
(cmd,json.dumps(kwargs, sort_keys=True))log(logmsg, filters)
result =self.qmp(cmd, **kwargs)
-log(json.dumps(result, sort_keys=True), filters)
+log(json.dumps(result, indent=indent, sort_keys=True), filters)But I'd actually have to read the contract to
json.dumps()to learn what is expected./me goes and does that...
https://docs.python.org/2/library/json.html
Quoted 8 lines
If indent is a non-negative integer, then JSON array elements and object members will be pretty-printed with that indent level. An indent level of 0, or negative, will only insert newlines. None (the default) selects the most compact representation.
Okay, makes sense.
Reviewed-by Eric Blake <[email protected]>
Yeah, it's a bubbling up of API in this case and a little messy, but it was the quickest route to making the output look pretty.
Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log
In reply to John SnowQuoted 17 lines · John Snow
If iotests have lines exceeding >998 characters long, git doesn't want to send it plaintext to the list. We can solve this by allowing the iotests to use pretty printed QMP output that we can match against instead.
As a bonus, it's much nicer for human eyes, too.
Signed-off-by John Snow <[email protected]>--- tests/qemu-iotests/iotests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)tests/qemu-iotests/iotests.py +0
-0
tests/qemu-iotests/iotests.py class VM(qtest.QEMUQtestMachine): · line 453
Quoted 8 lines
tests/qemu-iotests/iotests.py +1
-1@@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine): result.append(filter_qmp_event(ev)) return result - def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs): + def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs): logmsg = '{"execute": "%s", "arguments": %s}' % \ (cmd, json.dumps(kwargs, sort_keys=True)) log(logmsg, filters)
why not to prettify cmd json too? Just make fullobj = {"execute": cmd, "arguments": kwargs}, and prettify it. (hmm, unfortunately, "execute" < "arguments", and they will be rearranged)
with or without (as the patch don't aim to prettify everything):
tests/qemu-iotests/iotests.py class VM(qtest.QEMUQtestMachine): · line 456
Quoted 4 lines
tests/qemu-iotests/iotests.py +1
-1result = self.qmp(cmd, **kwargs) - log(json.dumps(result, sort_keys=True), filters) + log(json.dumps(result, indent=indent, sort_keys=True), filters) return result
hmm, doing the same thing as Eric (check specs), I see the following note:
Quoted 1 line
Note: Since the default item separator is ', ', the output might include trailing whitespace when indent is specified.
And I remember a pain of trailing whitespaces in iotests on, may be, backporting, or something like this some time ago. It's of course not about this patch, but I think it is a good idea to avoid trailing whitespaces in test output, at least in common helpers. May be best place to fix is iotests.log() function
Quoted 3 lines
def
run_job(self, job, auto_finalize=True, auto_dismiss=False):
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log
Quoted 21 lines · Vladimir Sementsov-Ogievskiy
Quoted 20 lines · John Snow
If iotests have lines exceeding >998 characters long, git doesn't want to send it plaintext to the list. We can solve this by allowing the iotests to use pretty printed QMP output that we can match against instead.
As a bonus, it's much nicer for human eyes, too.
Signed-off-by John Snow <[email protected]>--- tests/qemu-iotests/iotests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)tests/qemu-iotests/iotests.py +0
-0@@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine): result.append(filter_qmp_event(ev)) return result
Quoted 9 lines
tests/qemu-iotests/iotests.py class VM(qtest.QEMUQtestMachine): · line 453
Quoted 5 lines
tests/qemu-iotests/iotests.py +1
-1- def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs): + def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs): logmsg = '{"execute": "%s", "arguments": %s}' % \ (cmd, json.dumps(kwargs, sort_keys=True)) log(logmsg, filters)why not to prettify cmd json too? Just make fullobj = {"execute": cmd, "arguments": kwargs}, and prettify it. (hmm, unfortunately, "execute" < "arguments", and they will be rearranged)
It wasn't long enough to irritate me :)
but we're here, so I'll do that too.
Quoted 8 lines
with or without (as the patch don't aim to prettify everything):
Reviewed-by Vladimir Sementsov-Ogievskiy <[email protected]>tests/qemu-iotests/iotests.py class VM(qtest.QEMUQtestMachine): · line 456
Quoted 4 lines
tests/qemu-iotests/iotests.py +1
-1result = self.qmp(cmd, **kwargs) - log(json.dumps(result, sort_keys=True), filters) + log(json.dumps(result, indent=indent, sort_keys=True), filters) return result
Quoted 8 lines
hmm, doing the same thing as Eric (check specs), I see the following note:
Quoted 1 line
Note: Since the default item separator is ', ', the output might include trailing whitespace when indent is specified.
And I remember a pain of trailing whitespaces in iotests on, may be, backporting, or something like this some time ago. It's of course not about this patch, but I think it is a good idea to avoid trailing whitespaces in test output, at least in common helpers. May be best place to fix is
iotests.log()function
Oh, good spot. I actually did run into this and wasn't aware of what caused it! I will change the default separator.
Quoted 5 lines
Quoted 3 lines
def
run_job(self, job, auto_finalize=True, auto_dismiss=False):
Re: [Qemu-devel] [PATCH v2 6/7] iotests: allow pretty-print for qmp_log
Quoted 49 lines · John Snow
Quoted 21 lines · Vladimir Sementsov-Ogievskiy
Quoted 20 lines · John Snow
If iotests have lines exceeding >998 characters long, git doesn't want to send it plaintext to the list. We can solve this by allowing the iotests to use pretty printed QMP output that we can match against instead.
As a bonus, it's much nicer for human eyes, too.
Signed-off-by John Snow <[email protected]>--- tests/qemu-iotests/iotests.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)tests/qemu-iotests/iotests.py +0
-0@@ -447,12 +447,12 @@ class VM(qtest.QEMUQtestMachine): result.append(filter_qmp_event(ev)) return resultQuoted 9 lines
tests/qemu-iotests/iotests.py class VM(qtest.QEMUQtestMachine): · line 453
Quoted 5 lines
tests/qemu-iotests/iotests.py +1
-1- def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs): + def qmp_log(self, cmd, indent=None, filters=[filter_testfiles], **kwargs): logmsg = '{"execute": "%s", "arguments": %s}' % \ (cmd, json.dumps(kwargs, sort_keys=True)) log(logmsg, filters)why not to prettify cmd json too? Just make fullobj = {"execute": cmd, "arguments": kwargs}, and prettify it. (hmm, unfortunately, "execute" < "arguments", and they will be rearranged)
It wasn't long enough to irritate me :)
but we're here, so I'll do that too.Quoted 12 lines
with or without (as the patch don't aim to prettify everything):
Reviewed-by Vladimir Sementsov-Ogievskiy <[email protected]>tests/qemu-iotests/iotests.py class VM(qtest.QEMUQtestMachine): · line 456
Quoted 4 lines
tests/qemu-iotests/iotests.py +1
-1result = self.qmp(cmd, **kwargs) - log(json.dumps(result, sort_keys=True), filters) + log(json.dumps(result, indent=indent, sort_keys=True), filters) return resulthmm, doing the same thing as Eric (check specs), I see the following note:
Quoted 1 line
Note: Since the default item separator is ', ', the output might include trailing whitespace when indent is specified.
Quoted 7 lines
Quoted 4 lines
And I remember a pain of trailing whitespaces in iotests on, may be, backporting, or something like this some time ago. It's of course not about this patch, but I think it is a good idea to avoid trailing whitespaces in test output, at least in common helpers. May be best place to fix is
iotests.log()functionOh, good spot. I actually did run into this and wasn't aware of what caused it! I will change the default separator.
In this case output would be less pretty. I'd prefer just remove trailing whitespace as part of filtering process in log().
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
def
run_job(self, job, auto_finalize=True, auto_dismiss=False):
Signature · 2 lines
Best regards, Vladimir
PATCH v2 7/7 iotests: add iotest 236 for testing bitmap merge
In reply to John SnowNew interface, new smoke test.
--- tests/qemu-iotests/236 | 123 +++++++++++++++++ tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 389 insertions(+) create mode 100755 tests/qemu-iotests/236 create mode 100644 tests/qemu-iotests/236.out
tests/qemu-iotests/236 new file +123 -0
@@ -0,0 +1,123 @@ +#!/usr/bin/env python +# +# Test bitmap merges. +# +# Copyright (c) 2018 John Snow for Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# +# [email protected] + +import sys +import os +import iotests +from iotests import log +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts')) +from qemu import QEMUMachine + +iotests.verify_image_format(supported_fmts=['qcow2']) + +patterns = [("0x5d", "0", "64k"), + ("0xd5", "1M", "64k"), + ("0xdc", "32M", "64k"), + ("0xcd", "0x3ff0000", "64k")] # 64M - 64K + +overwrite = [("0xab", "0", "64k"), # Full overwrite + ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K) + ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K) + ("0xea", "0x3fe0000", "64k")] # Adjacent-left (64M - 128K) + +with iotests.FilePath('img') as img_path, \ + iotests.VM() as vm: + + log('--- Preparing image & VM ---\n') + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M') + vm.add_drive(img_path) + vm.launch() + + log('--- Adding preliminary bitmaps A & B ---\n') + vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapA") + vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB") + + # Dirties 4 clusters. count=262144 + log('') + log('--- Emulating writes ---\n') + for p in patterns: + cmd = "write -P%s %s %s" % p + log(cmd) + log(vm.hmp_qemu_io("drive0", cmd)) + + vm.qmp_log("query-block", indent=2, + filters=[iotests.filter_generated_node_ids, + iotests.filter_testfiles]) + + log('') + log('--- Disabling B & Adding C ---\n') + vm.qmp_log("transaction", actions=[ + { "type": "block-dirty-bitmap-disable", + "data": { "node": "drive0", "name": "bitmapB" }}, + { "type": "block-dirty-bitmap-add", + "data": { "node": "drive0", "name": "bitmapC" }} + ]) + + log('') + log('--- Emulating further writes ---\n') + # Dirties 6 clusters, 3 of which are new in contrast to "A". + # A = 64 * 1024 * (4 + 3) = 458752 + # C = 64 * 1024 * 6 = 393216 + for p in overwrite: + cmd = "write -P%s %s %s" % p + log(cmd) + log(vm.hmp_qemu_io("drive0", cmd)) + + log('') + log('--- Disabling A & C ---\n') + vm.qmp_log("transaction", actions=[ + { "type": "block-dirty-bitmap-disable", + "data": { "node": "drive0", "name": "bitmapA" }}, + { "type": "block-dirty-bitmap-disable", + "data": { "node": "drive0", "name": "bitmapC" }} + ]) + + # A: 7 clusters + # B: 4 clusters + # C: 6 clusters + vm.qmp_log("query-block", indent=2, + filters=[iotests.filter_generated_node_ids, + iotests.filter_testfiles]) + + log('') + log('--- Creating D as a merge of B & C ---\n') + # Good hygiene: create a disabled bitmap as a merge target. + vm.qmp_log("transaction", actions=[ + { "type": "block-dirty-bitmap-add", + "data": { "node": "drive0", "name": "bitmapD", "disabled": True }}, + { "type": "block-dirty-bitmap-merge", + "data": { "node": "drive0", "target": "bitmapD", + "bitmaps": ["bitmapB", "bitmapC"] }} + ]) + + # A and D should now both have 7 clusters apiece. + # B and C remain unchanged with 4 and 6 respectively. + vm.qmp_log("query-block", indent=2, + filters=[iotests.filter_generated_node_ids, + iotests.filter_testfiles]) + + # A and D should be equivalent. + vm.qmp_log('x-debug-block-dirty-bitmap-sha256', + node="drive0", name="bitmapA") + vm.qmp_log('x-debug-block-dirty-bitmap-sha256', + node="drive0", name="bitmapD") + + vm.shutdown()
tests/qemu-iotests/236.out new file +265 -0
265 lines not on this page · raw message
tests/qemu-iotests/group +1 -0
@@ -233,3 +233,4 @@ 233 auto quick 234 auto quick migration 235 auto quick +236 auto quick
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 7/7] iotests: add iotest 236 for testing bitmap merge
Quoted 12 lines · John Snow
New interface, new smoke test.
Signed-off-by John Snow <[email protected]>--- tests/qemu-iotests/236 | 123 +++++++++++++++++ tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 389 insertions(+)create mode 100755 tests/qemu-iotests/236
create mode 100644 tests/qemu-iotests/236.out
tests/qemu-iotests/236 line 72
Quoted 9 lines
tests/qemu-iotests/236 +8
-0+ log('') + log('--- Disabling B & Adding C ---\n') + vm.qmp_log("transaction", actions=[ + { "type": "block-dirty-bitmap-disable", + "data": { "node": "drive0", "name": "bitmapB" }}, + { "type": "block-dirty-bitmap-add", + "data": { "node": "drive0", "name": "bitmapC" }} + ])
Just for grins, what happens if we extend that transaction to additionally call:
{ "type": "block-dirty-bitmap-disable",
"data": { "node": "drive0", "name": "bitmapC" }},
{ "type": "block-dirty-bitmap-enable",
"data": { "node": "drive0", "name": "bitmapC" }}Yes, we have a redundancy where plain '-add' coupled with '-disable' in a transaction does the same as '-add' with 'enabled':false. And I'd rather keep 'enabled':false as part of '-add', as it's handy to do that without having to code up a transaction. But the specific act of toggling the enabled bit twice on a brand new bitmap as part of the same transaction, while unlikely to be done by libvirt, may still prove to be a useful validation of our transaction semantics.
tests/qemu-iotests/236 line 236
Quoted 7 lines
+++ b/tests/qemu-iotests/group
tests/qemu-iotests/236 +1
-0@@ -233,3 +233,4 @@ 233 auto quick 234 auto quick migration 235 auto quick +236 auto quick
You'll want to fix that.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 7/7] iotests: add iotest 236 for testing bitmap merge
Quoted 39 lines · Eric Blake
Quoted 12 lines · John Snow
New interface, new smoke test.
Signed-off-by John Snow <[email protected]>---
tests/qemu-iotests/236 | 123 +++++++++++++++++ tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 +3 files changed, 389 insertions(+)create mode 100755 tests/qemu-iotests/236
create mode 100644 tests/qemu-iotests/236.outtests/qemu-iotests/236 line 72
Quoted 9 lines
tests/qemu-iotests/236 +8
-0+ log('') + log('--- Disabling B & Adding C ---\n') + vm.qmp_log("transaction", actions=[ + { "type": "block-dirty-bitmap-disable", + "data": { "node": "drive0", "name": "bitmapB" }}, + { "type": "block-dirty-bitmap-add", + "data": { "node": "drive0", "name": "bitmapC" }} + ])Just for grins, what happens if we extend that transaction to additionally call:
{ "type": "block-dirty-bitmap-disable", "data": { "node": "drive0", "name": "bitmapC" }}, { "type": "block-dirty-bitmap-enable", "data": { "node": "drive0", "name": "bitmapC" }}
Yes, we have a redundancy where plain '-add' coupled with '-disable' in a transaction does the same as '-add' with 'enabled':false. And I'd rather keep 'enabled':false as part of '-add', as it's handy to do that without having to code up a transaction. But the specific act of toggling the enabled bit twice on a brand new bitmap as part of the same transaction, while unlikely to be done by libvirt, may still prove to be a useful validation of our transaction semantics.
Quoted 10 lines
Quoted 7 lines
+++ b/tests/qemu-iotests/group
tests/qemu-iotests/236 +0
-0@@ -233,3 +233,4 @@233 auto quick
234 auto quick migration
235 auto quick
+236 auto quick
\ No newline at end of fileYou'll want to fix that.
Now that you've pointed it out, I have to. :)
Quoted 2 lines
Reviewed-by Eric Blake <[email protected]>
OK. I want more tests, too; mostly around malformed requests (bad target, single bad source, multiple bad sources, mixed bad sources). your suggestion to merge self into self,
and maybe anything else I've forgotten by now.
--js
Re: [Qemu-devel] [PATCH v2 7/7] iotests: add iotest 236 for testing bitmap merge
In reply to John SnowQuoted 34 lines · John Snow
New interface, new smoke test.
Signed-off-by John Snow <[email protected]>--- tests/qemu-iotests/236 | 123 +++++++++++++++++ tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 389 insertions(+)create mode 100755 tests/qemu-iotests/236
create mode 100644 tests/qemu-iotests/236.outtests/qemu-iotests/236 new file +17
-0@@ -0,0 +1,123 @@ +#!/usr/bin/env python +# +# Test bitmap merges. +# +# Copyright (c) 2018 John Snow for Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License
tests/qemu-iotests/236 line 27
Quoted 10 lines
tests/qemu-iotests/236 new file +10
-0+# along with this program. If not, see <http://www.gnu.org/licenses/>. +# +# [email protected] + +import sys +import os +import iotests +from iotests import log +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts')) +from qemu import QEMUMachine
unused, with previous line and, therefore, os and sys modules)
Quoted 8 lines
tests/qemu-iotests/236 new file +8
-0+ +iotests.verify_image_format(supported_fmts=['qcow2']) + +patterns = [("0x5d", "0", "64k"), + ("0xd5", "1M", "64k"), + ("0xdc", "32M", "64k"), + ("0xcd", "0x3ff0000", "64k")] # 64M - 64K +
tests/qemu-iotests/236 line 45
Quoted 10 lines
tests/qemu-iotests/236 new file +10
-0+overwrite = [("0xab", "0", "64k"), # Full overwrite + ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K) + ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K) + ("0xea", "0x3fe0000", "64k")] # Adjacent-left (64M - 128K) + +with iotests.FilePath('img') as img_path, \ + iotests.VM() as vm: + + log('--- Preparing image & VM ---\n') + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
hm, actually null device is enough here.
Quoted 7 lines
tests/qemu-iotests/236 new file +7
-0+ vm.add_drive(img_path) + vm.launch() + + log('--- Adding preliminary bitmaps A & B ---\n') + vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapA") + vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB") +
tests/qemu-iotests/236 line 63
Quoted 10 lines
tests/qemu-iotests/236 new file +10
-0+ # Dirties 4 clusters. count=262144 > + log('') + log('--- Emulating writes ---\n') + for p in patterns: + cmd = "write -P%s %s %s" % p + log(cmd) + log(vm.hmp_qemu_io("drive0", cmd)) + + vm.qmp_log("query-block", indent=2, + filters=[iotests.filter_generated_node_ids, + iotests.filter_testfiles])
I'm against. query-block prints a lot of unrelated things, which may change from version to version (for example, Andrey is now adding bitmap information to qcow2 format-specific info), then, backported test may fail for previous versions (or just different config) because of something absolutely unrelated to bitmaps.
I think, it should be shortened to result[0]['device'], result[0]['dirty-bitmaps']
And, I think, any way it would be good to create a separate helper for printing current state of bitmaps (which may output their sha256 too)
tests/qemu-iotests/236 line 66
Quoted 3 lines
tests/qemu-iotests/236 new file +3
-0+ + log('') + log('--- Disabling B & Adding C ---\n')
why not justlog('\n--- Disabling B & Adding C ---\n')
[...]
Otherwise, the test looks fine for me
Signature · 2 lines
Best regards, Vladimir
Re: [Qemu-devel] [PATCH v2 7/7] iotests: add iotest 236 for testing bitmap merge
Quoted 39 lines · Vladimir Sementsov-Ogievskiy
Quoted 38 lines · John Snow
New interface, new smoke test.
Signed-off-by John Snow <[email protected]>--- tests/qemu-iotests/236 | 123 +++++++++++++++++ tests/qemu-iotests/236.out | 265 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 389 insertions(+)create mode 100755 tests/qemu-iotests/236
create mode 100644 tests/qemu-iotests/236.outtests/qemu-iotests/236 new file +21
-0@@ -0,0 +1,123 @@ +#!/usr/bin/env python +# +# Test bitmap merges. +# +# Copyright (c) 2018 John Snow for Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# +# [email protected] +
Quoted 9 lines
tests/qemu-iotests/236 line 27
Quoted 6 lines
tests/qemu-iotests/236 new file +6
-0+import sys +import os +import iotests +from iotests import log +sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'scripts')) +from qemu import QEMUMachineunused, with previous line and, therefore, os and sys modules)
Forgive the copy and paste. I'll trim it down.
Quoted 12 lines
Quoted 12 lines
tests/qemu-iotests/236 new file +12
-0+ +iotests.verify_image_format(supported_fmts=['qcow2']) + +patterns = [("0x5d", "0", "64k"), + ("0xd5", "1M", "64k"), + ("0xdc", "32M", "64k"), + ("0xcd", "0x3ff0000", "64k")] # 64M - 64K + +overwrite = [("0xab", "0", "64k"), # Full overwrite + ("0xad", "0x00f8000", "64k"), # Partial-left (1M-32K) + ("0x1d", "0x2008000", "64k"), # Partial-right (32M+32K) + ("0xea", "0x3fe0000", "64k")] # Adjacent-left (64M - 128K)
Quoted 9 lines
tests/qemu-iotests/236 line 45
Quoted 6 lines
tests/qemu-iotests/236 new file +6
-0+ +with iotests.FilePath('img') as img_path, \ + iotests.VM() as vm: + + log('--- Preparing image & VM ---\n') + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')hm, actually null device is enough here.
Sure.
Quoted 14 lines
Quoted 14 lines
tests/qemu-iotests/236 new file +14
-0+ vm.add_drive(img_path) + vm.launch() + + log('--- Adding preliminary bitmaps A & B ---\n') + vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapA") + vm.qmp_log("block-dirty-bitmap-add", node="drive0", name="bitmapB") + + # Dirties 4 clusters. count=262144 > + log('') + log('--- Emulating writes ---\n') + for p in patterns: + cmd = "write -P%s %s %s" % p + log(cmd) + log(vm.hmp_qemu_io("drive0", cmd)) +
Quoted 9 lines
tests/qemu-iotests/236 line 63
Quoted 3 lines
tests/qemu-iotests/236 new file +3
-0+ vm.qmp_log("query-block", indent=2, + filters=[iotests.filter_generated_node_ids, + iotests.filter_testfiles])I'm against. query-block prints a lot of unrelated things, which may change from version to version (for example, Andrey is now adding bitmap information to qcow2 format-specific info), then, backported test may fail for previous versions (or just different config) because of something absolutely unrelated to bitmaps.
You have a point. I'm only interested in the bitmap info structures, here.
Quoted 2 lines
I think, it should be shortened to result[0]['device'], result[0]['dirty-bitmaps']
OK, let me look at printing something like
{
"device0": bitmapInfo0,
"device1": bitmapInfo1,
...
}
to the log.
Quoted 3 lines
And, I think, any way it would be good to create a separate helper for printing current state of bitmaps (which may output their sha256 too)
Yeah, I was working on a command to do just this but I ran into troubles with caching the bitmap info for computing it and wound up shelving the series and I got sidetracked on other issues...
Quoted 7 lines
tests/qemu-iotests/236 line 66
Quoted 3 lines
tests/qemu-iotests/236 new file +3
-0+ + log('') + log('--- Disabling B & Adding C ---\n')why not just
log('\n--- Disabling B & Adding C ---\n')
Just preference, there's no reason.
I have a habit of avoiding pre-pending control characters to string lines, but don't have an aversion to appending them.
I can change it if it sticks out as too unusual.
Quoted 5 lines
[...]
Otherwise, the test looks fine for me
Thanks for looking!
Patchew URL: https://patchew.org/QEMU/[email protected]/
Patchew URL: https://patchew.org/QEMU/[email protected]/
Hi,
This series seems to have some coding style problems. See output below for more information:
Type: series
Message-id: [email protected]
Subject: [Qemu-devel] [PATCH v2 0/7] bitmaps: remove x- prefix from QMP api
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test'
7edca32 iotests: add iotest 236 for testing bitmap merge 4760f12 iotests: allow pretty-print for qmp_log 18479b6 iotests: add filter_generated_node_ids e74d8c5 iotests.py: don't abort if IMGKEYSECRET is undefined 32d68bd4 block: remove 'x' prefix from experimental bitmap APIs 82059d7 blockdev: n-ary bitmap merge
51b6f48 blockdev: abort transactions in reverse order
=== OUTPUT BEGIN ===
Checking PATCH 1/7: blockdev: abort transactions in reverse order... Checking PATCH 2/7: blockdev: n-ary bitmap merge...
ERROR: externs should be avoided in .c files
#27: FILE: blockdev.c:2125:
+void do_block_dirty_bitmap_merge(const char *node, const char *target,
total: 1 errors, 0 warnings, 147 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
Checking PATCH 3/7: block: remove 'x' prefix from experimental bitmap APIs... Checking PATCH 4/7: iotests.py: don't abort if IMGKEYSECRET is undefined... Checking PATCH 5/7: iotests: add filter_generated_node_ids... Checking PATCH 6/7: iotests: allow pretty-print for qmp_log... Checking PATCH 7/7: iotests: add iotest 236 for testing bitmap merge...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #12:
new file mode 100755
total: 0 errors, 1 warnings, 392 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/[email protected]/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]