From f4de0f8c40b70c4c9308b4670e0a6ad9faed0262 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 11 Jan 2019 11:59:16 -0600 Subject: 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. 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 Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181221093529.23855-2-jsnow@redhat.com> [eblake: rebase] Signed-off-by: Eric Blake --- blockdev.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/blockdev.c b/blockdev.c index 1cc893f..3d46f81 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(, BlkActionState) snap_bdrv_states; - QSIMPLEQ_INIT(&snap_bdrv_states); + QTAILQ_HEAD(, 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, 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); } -- cgit v1.1 From 07d5a8df6a0018d831baef6d50f53f31a06b5a60 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:20 -0500 Subject: block/dirty-bitmap: remove assertion from restore When making a backup of a dirty bitmap (for transactions), we want to restore that backup whether or not the bitmap is enabled. 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. Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: John Snow Message-Id: <20181221093529.23855-3-jsnow@redhat.com> Signed-off-by: Eric Blake --- block/dirty-bitmap.c | 1 - 1 file changed, 1 deletion(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 89fd1d7..6b68839 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); -- cgit v1.1 From 360d4e4e9a501d92fb8866ac307d33a25f70c6d1 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:21 -0500 Subject: blockdev: n-ary bitmap merge 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 Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181221093529.23855-4-jsnow@redhat.com> Signed-off-by: Eric Blake --- blockdev.c | 75 +++++++++++++++++++++++++++++++++++----------------- qapi/block-core.json | 22 +++++++-------- 2 files changed, 62 insertions(+), 35 deletions(-) diff --git a/blockdev.c b/blockdev.c index 3d46f81..f7bf48d 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; + } + + 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; + } } - bdrv_merge_dirty_bitmap(dst, src, NULL, errp); + /* 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 762000f..a153ea4 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": {} } # ## -- cgit v1.1 From 0e2b7f09837f1e2828d428af1e4ebb61e3b3ea5f Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:22 -0500 Subject: block: remove 'x' prefix from experimental bitmap APIs 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 Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181221093529.23855-5-jsnow@redhat.com> Signed-off-by: Eric Blake --- 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 f7bf48d..a8fa874 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 a153ea4..91685be 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 5875cdb..95edb78 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 397b865..5513dc6 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", -- cgit v1.1 From 58ebcb65d8e38fd693bf1e58dd941a4d7a8dfdef Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:23 -0500 Subject: iotests.py: don't abort if IMGKEYSECRET is undefined 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 Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181221093529.23855-6-jsnow@redhat.com> Signed-off-by: Eric Blake --- 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 d537538..a34e668 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' -- cgit v1.1 From fa1151f811ffa1973b6980b09f095d0e2ebea08b Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:24 -0500 Subject: iotests: add filter_generated_node_ids To mimic the common filter of the same name, but for the python tests. Signed-off-by: John Snow Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181221093529.23855-7-jsnow@redhat.com> Signed-off-by: Eric Blake --- 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 a34e668..9595429 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'): -- cgit v1.1 From 0706e87d72b02f28bfa04400388f9c9df1b9c943 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:25 -0500 Subject: iotests: add qmp recursive sorting function 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. This means that iotests output will sometimes show arguments in an order not specified by the test author. Presently, we accomplish this by using json.dumps' sort_keys argument, where we only serialize the arguments dictionary, but not the command. 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 after "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. The qmp_log function uses this to build a QMP object that keeps "execute" above "arguments", but sorts all keys and keys in any subdicts in "arguments" lexicographically to maintain consistent iotests output, with no incompatible changes to any current test. Signed-off-by: John Snow Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181221093529.23855-8-jsnow@redhat.com> Signed-off-by: Eric Blake --- 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 9595429..565eebb 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) -- cgit v1.1 From f8ca8609d8549def45b28e82ecac64adaeee9f12 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:26 -0500 Subject: iotests: remove default filters from qmp_log Several places in iotests deal with serializing objects into JSON strings, but to add pretty-printing it seems desirable 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 Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181221093529.23855-9-jsnow@redhat.com> Signed-off-by: Eric Blake --- 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 128c334..e92550f 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 565eebb..57fe20d 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)) -- cgit v1.1 From 08fcd6111e1949f456e1b232ebeeb0cc17019a92 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:27 -0500 Subject: iotests: change qmp_log filters to expect QMP objects only 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 desirable 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 Message-Id: <20181221093529.23855-10-jsnow@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy Signed-off-by: Eric Blake --- tests/qemu-iotests/206 | 4 ++-- tests/qemu-iotests/iotests.py | 28 +++++++++++++++++++++++++--- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/tests/qemu-iotests/206 b/tests/qemu-iotests/206 index e92550f..5bb738b 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 57fe20d..a96a701 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -246,10 +246,33 @@ 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.''' + # Iterate through either lists or dicts; + if isinstance(qmsg, list): + items = enumerate(qmsg) + else: + items = qmsg.items() + + for k, v in items: + if isinstance(v, list) or isinstance(v, dict): + qmsg[k] = filter_qmp(v, filter_fn) + else: + qmsg[k] = filter_fn(k, v) + 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 +488,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): -- cgit v1.1 From 55cd64eab5cb7958c629edbf5f2233b87dfbd1b0 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:28 -0500 Subject: iotests: implement pretty-print for log and qmp_log 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 Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20181221093529.23855-11-jsnow@redhat.com> Signed-off-by: Eric Blake --- 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 a96a701..cbedfaf 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -288,13 +288,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) @@ -483,14 +488,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): -- cgit v1.1 From 14da540f2a68b3f730b1a7c31de783f3d68f6fc7 Mon Sep 17 00:00:00 2001 From: John Snow Date: Fri, 21 Dec 2018 04:35:29 -0500 Subject: iotests: add iotest 236 for testing bitmap merge New interface, new smoke test. Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Message-Id: <20181221093529.23855-12-jsnow@redhat.com> [eblake: fix last-minute change to echo text] Signed-off-by: Eric Blake --- 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 0000000..79a6381 --- /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 . +# +# 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 & Aborting 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 & Aborting Merge Transaction ---\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_d = vm.qmp('x-debug-block-dirty-bitmap-sha256', + node="drive0", name="bitmapD")['return']['sha256'] + assert(check_a == check_d) + + 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 0000000..1dad24d --- /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 & Aborting 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 & Aborting Merge Transaction --- + +{ + "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 61a6d98..f6b2459 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 -- cgit v1.1 From 9d97658020db922b68da05faadcdd61f49fbbdc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Fri, 11 Jan 2019 17:35:19 +0100 Subject: qemu-nbd: Rename 'exp' variable clashing with math::exp() symbol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The use of a variable named 'exp' prevents includes to import . Rename it to avoid: qemu-nbd.c:64:19: error: ‘exp’ redeclared as different kind of symbol static NBDExport *exp; ^~~ In file included from /usr/include/features.h:428, from /usr/include/bits/libc-header-start.h:33, from /usr/include/stdint.h:26, from /usr/lib/gcc/x86_64-redhat-linux/8/include/stdint.h:9, from /source/qemu/include/qemu/osdep.h:80, from /source/qemu/qemu-nbd.c:19: /usr/include/bits/mathcalls.h:95:1: note: previous declaration of ‘exp’ was here __MATHCALL_VEC (exp,, (_Mdouble_ __x)); ^~~~~~~~~~~~~~ Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Eric Blake Message-Id: <20190111163519.11457-1-philmd@redhat.com> Signed-off-by: Eric Blake --- qemu-nbd.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index 2807e13..6ca02b6 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -61,7 +61,7 @@ #define MBR_SIZE 512 -static NBDExport *exp; +static NBDExport *export; static int verbose; static char *srcpath; static SocketAddress *saddr; @@ -335,7 +335,7 @@ static int nbd_can_accept(void) return state == RUNNING && nb_fds < shared; } -static void nbd_export_closed(NBDExport *exp) +static void nbd_export_closed(NBDExport *export) { assert(state == TERMINATING); state = TERMINATED; @@ -1015,10 +1015,11 @@ int main(int argc, char **argv) } } - exp = nbd_export_new(bs, dev_offset, fd_size, nbdflags, nbd_export_closed, - writethrough, NULL, &error_fatal); - nbd_export_set_name(exp, export_name); - nbd_export_set_description(exp, export_description); + export = nbd_export_new(bs, dev_offset, fd_size, nbdflags, + nbd_export_closed, writethrough, + NULL, &error_fatal); + nbd_export_set_name(export, export_name); + nbd_export_set_description(export, export_description); if (device) { #if HAVE_NBD_DEVICE @@ -1055,9 +1056,9 @@ int main(int argc, char **argv) main_loop_wait(false); if (state == TERMINATE) { state = TERMINATING; - nbd_export_close(exp); - nbd_export_put(exp); - exp = NULL; + nbd_export_close(export); + nbd_export_put(export); + export = NULL; } } while (state != TERMINATED); -- cgit v1.1 From 2d2fd67428f0cfbffea16969d2635af3e2d78d3d Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 11 Jan 2019 13:47:13 -0600 Subject: nbd: Add some error case testing to iotests 223 Testing success paths is important, but it's also nice to highlight expected failure handling, to show that we don't crash, and so that upcoming tests that change behavior can demonstrate the resulting effects on error paths. Add the following errors: Attempting to export without a running server Attempting to start a second server Attempting to export a bad node name Attempting to export a name that is already exported Attempting to export an enabled bitmap Attempting to remove an already removed export Attempting to quit server a second time All of these properly complain except for a second server-stop, which will be fixed next. Signed-off-by: Eric Blake Message-Id: <20190111194720.15671-2-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/223 | 19 +++++++++++++++++-- tests/qemu-iotests/223.out | 7 +++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223 index 5513dc6..61b46a2 100755 --- a/tests/qemu-iotests/223 +++ b/tests/qemu-iotests/223 @@ -107,6 +107,7 @@ echo _launch_qemu 2> >(_filter_nbd) +# Intentionally provoke some errors as well, to check error handling silent= _send_qemu_cmd $QEMU_HANDLE '{"execute":"qmp_capabilities"}' "return" _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add", @@ -114,18 +115,29 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"blockdev-add", "file":{"driver":"file", "filename":"'"$TEST_IMG"'"}}}' "return" _send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable", "arguments":{"node":"n", "name":"b"}}' "return" -_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable", - "arguments":{"node":"n", "name":"b2"}}' "return" +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", + "arguments":{"device":"n"}}' "error" # Attempt add without server _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start", "arguments":{"addr":{"type":"unix", "data":{"path":"'"$TEST_DIR/nbd"'"}}}}' "return" +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start", + "arguments":{"addr":{"type":"unix", + "data":{"path":"'"$TEST_DIR/nbd"1'"}}}}' "error" # Attempt second server _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", "arguments":{"device":"n"}}' "return" +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", + "arguments":{"device":"nosuch"}}' "error" # Attempt to export missing node +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", + "arguments":{"device":"n"}}' "error" # Attempt to export same name twice _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap", "arguments":{"name":"n", "bitmap":"b"}}' "return" _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", "arguments":{"device":"n", "name":"n2"}}' "return" _send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap", + "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Attempt enabled bitmap +_send_qemu_cmd $QEMU_HANDLE '{"execute":"block-dirty-bitmap-disable", + "arguments":{"node":"n", "name":"b2"}}' "return" +_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap", "arguments":{"name":"n2", "bitmap":"b2"}}' "return" echo @@ -157,7 +169,10 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove", "arguments":{"name":"n"}}' "return" _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove", "arguments":{"name":"n2"}}' "return" +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove", + "arguments":{"name":"n2"}}' "error" # Attempt duplicate clean _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return" +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return" # Oops _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return" # success, all done diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index 99ca172..e6ede05 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -27,10 +27,15 @@ wrote 2097152/2097152 bytes at offset 2097152 {"return": {}} {"return": {}} {"return": {}} +{"error": {"class": "GenericError", "desc": "NBD server not running"}} {"return": {}} +{"error": {"class": "GenericError", "desc": "NBD server already running"}} {"return": {}} +{"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}} +{"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}} {"return": {}} {"return": {}} +{"error": {"class": "GenericError", "desc": "Bitmap 'b2' is enabled"}} {"return": {}} {"return": {}} @@ -62,6 +67,8 @@ read 2097152/2097152 bytes at offset 2097152 {"return": {}} {"return": {}} +{"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}} +{"return": {}} {"return": {}} {"return": {}} *** done -- cgit v1.1 From 7801c3a7fd7042fd2f9435af4b9a6d2094073174 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 11 Jan 2019 13:47:14 -0600 Subject: nbd: Forbid nbd-server-stop when server is not running Since we already forbid other nbd-server commands when not in the right state, it is unlikely that any caller was relying on a second stop to behave as a silent no-op. Update iotest 223 to show the improved behavior. Signed-off-by: Eric Blake Message-Id: <20190111194720.15671-3-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy --- blockdev-nbd.c | 5 +++++ tests/qemu-iotests/223 | 2 +- tests/qemu-iotests/223.out | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 1d170c8..ca58491 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -214,6 +214,11 @@ void qmp_nbd_server_remove(const char *name, void qmp_nbd_server_stop(Error **errp) { + if (!nbd_server) { + error_setg(errp, "NBD server not running"); + return; + } + nbd_export_close_all(); nbd_server_free(nbd_server); diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223 index 61b46a2..a401609 100755 --- a/tests/qemu-iotests/223 +++ b/tests/qemu-iotests/223 @@ -172,7 +172,7 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove", _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove", "arguments":{"name":"n2"}}' "error" # Attempt duplicate clean _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return" -_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return" # Oops +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "error" # Again _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return" # success, all done diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index e6ede05..8a4d63a 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -69,6 +69,6 @@ read 2097152/2097152 bytes at offset 2097152 {"return": {}} {"error": {"class": "GenericError", "desc": "Export 'n2' is not found"}} {"return": {}} -{"return": {}} +{"error": {"class": "GenericError", "desc": "NBD server not running"}} {"return": {}} *** done -- cgit v1.1 From 702aa50d61497d29ef3b5e9c75d1404b3e6fd831 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 11 Jan 2019 13:47:15 -0600 Subject: nbd: Only require disabled bitmap for read-only exports Our initial implementation of x-nbd-server-add-bitmap put in a restriction because of incremental backups: in that usage, we are exporting one qcow2 file (the temporary overlay target of a blockdev-backup sync:none job) and a dirty bitmap owned by a second qcow2 file (the source of the blockdev-backup, which is the backing file of the temporary). While both qcow2 files are still writable (the target in order to capture copy-on-write of old contents, and the source in order to track live guest writes in the meantime), the NBD client expects to see constant data, including the dirty bitmap. An enabled bitmap in the source would be modified by guest writes, which is at odds with the NBD export being a read-only constant view, hence the initial code choice of enforcing a disabled bitmap (the intent is that the exposed bitmap was disabled in the same transaction that started the blockdev-backup job, although we don't want to track enough state to actually enforce that). However, consider the case of a bitmap contained in a read-only node (including when the bitmap is found in a backing layer of the active image). Because the node can't be modified, the bitmap won't change due to writes, regardless of whether it is still enabled. Forbidding the export unless the bitmap is disabled is awkward, paritcularly since we can't change the bitmap to be disabled (because the node is read-only). Alternatively, consider the case of live storage migration, where management directs the destination to create a writable NBD server, then performs a drive-mirror from the source to the target, prior to doing the rest of the live migration. Since storage migration can be time-consuming, it may be wise to let the destination include a dirty bitmap to track which portions it has already received, where even if the migration is interrupted and restarted, the source can query the destination block status in order to potentially minimize re-sending data that has not changed in the meantime on a second attempt. Such code has not been written, and might not be trivial (after all, a cluster being marked dirty in the bitmap does not necessarily guarantee it has the desired contents), but it makes sense that letting an active dirty bitmap be exposed and changing alongside writes may prove useful in the future. Solve both issues by gating the restriction against a disabled bitmap to only happen when the caller has requested a read-only export, and where the BDS that owns the bitmap (whether or not it is the BDS handed to nbd_export_new() or from its backing chain) is still writable. We could drop the check altogether (if management apps are prepared to deal with a changing bitmap even on a read-only image), but for now keeping a check for the read-only case still stands a chance of preventing management errors. Update iotest 223 to show the looser behavior by leaving a bitmap enabled the whole run; note that we have to tear down and re-export a node when handling an error. Signed-off-by: Eric Blake Message-Id: <20190111194720.15671-4-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy --- nbd/server.c | 7 +++++-- tests/qemu-iotests/223 | 10 +++++++--- tests/qemu-iotests/223.out | 3 ++- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 7af0ddf..9832708 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2456,8 +2456,11 @@ void nbd_export_bitmap(NBDExport *exp, const char *bitmap, return; } - if (bdrv_dirty_bitmap_enabled(bm)) { - error_setg(errp, "Bitmap '%s' is enabled", bitmap); + if ((exp->nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) && + bdrv_dirty_bitmap_enabled(bm)) { + error_setg(errp, + "Enabled bitmap '%s' incompatible with readonly export", + bitmap); return; } diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223 index a401609..f200e31 100755 --- a/tests/qemu-iotests/223 +++ b/tests/qemu-iotests/223 @@ -61,6 +61,8 @@ echo "=== Create partially sparse image, then add dirty bitmaps ===" echo # Two bitmaps, to contrast granularity issues +# Also note that b will be disabled, while b2 is left enabled, to +# check for read-only interactions _make_test_img -o cluster_size=4k 4M $QEMU_IO -c 'w -P 0x11 1M 2M' "$TEST_IMG" | _filter_qemu_io run_qemu < Date: Fri, 11 Jan 2019 13:47:16 -0600 Subject: nbd: Merge nbd_export_set_name into nbd_export_new The existing NBD code had a weird split where nbd_export_new() created an export but did not add it to the list of exported names until a later nbd_export_set_name() came along and grabbed a second reference on the object; later, the first call to nbd_export_close() drops the second reference while removing the export from the list. This is in part because the QAPI NbdServerRemoveNode enum documents the possibility of adding a mode where we could do a soft disconnect: preventing new clients, but waiting for existing clients to gracefully quit, based on the mode used when calling nbd_export_close(). But in spite of all that, note that we never change the name of an NBD export while it is exposed, which means it is easier to just inline the process of setting the name as part of creating the export. Inline the contents of nbd_export_set_name() and nbd_export_set_description() into the two points in an export lifecycle where they matter, then adjust both callers to pass the name up front. Note that for creation, all callers pass a non-NULL name, (passing NULL at creation was for old style servers, but we removed support for that in commit 7f7dfe2a), so we can add an assert and do things unconditionally; but for cleanup, because of the dual nature of nbd_export_close(), we still have to be careful to avoid use-after-free. Along the way, add a comment reminding ourselves of the potential of adding a middle mode disconnect. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20190111194720.15671-5-eblake@redhat.com> --- blockdev-nbd.c | 5 ++--- include/block/nbd.h | 3 +-- nbd/server.c | 52 +++++++++++++++++++++++----------------------------- qemu-nbd.c | 8 +++----- 4 files changed, 29 insertions(+), 39 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index ca58491..582ffde 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -174,14 +174,13 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, writable = false; } - exp = nbd_export_new(bs, 0, -1, writable ? 0 : NBD_FLAG_READ_ONLY, + exp = nbd_export_new(bs, 0, -1, name, NULL, + writable ? 0 : NBD_FLAG_READ_ONLY, NULL, false, on_eject_blk, errp); if (!exp) { return; } - nbd_export_set_name(exp, name); - /* The list of named exports has a strong reference to this export now and * our only way of accessing it is through nbd_export_find(), so we can drop * the strong reference that is @exp. */ diff --git a/include/block/nbd.h b/include/block/nbd.h index 65402d3..2f9a2ae 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -295,6 +295,7 @@ typedef struct NBDExport NBDExport; typedef struct NBDClient NBDClient; NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, + const char *name, const char *description, uint16_t nbdflags, void (*close)(NBDExport *), bool writethrough, BlockBackend *on_eject_blk, Error **errp); @@ -306,8 +307,6 @@ void nbd_export_put(NBDExport *exp); BlockBackend *nbd_export_get_blockdev(NBDExport *exp); NBDExport *nbd_export_find(const char *name); -void nbd_export_set_name(NBDExport *exp, const char *name); -void nbd_export_set_description(NBDExport *exp, const char *description); void nbd_export_close_all(void); void nbd_client_new(QIOChannelSocket *sioc, diff --git a/nbd/server.c b/nbd/server.c index 9832708..bb5438c 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1456,6 +1456,7 @@ static void nbd_eject_notifier(Notifier *n, void *data) } NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, + const char *name, const char *description, uint16_t nbdflags, void (*close)(NBDExport *), bool writethrough, BlockBackend *on_eject_blk, Error **errp) @@ -1471,6 +1472,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, * that BDRV_O_INACTIVE is cleared and the image is ready for write * access since the export could be available before migration handover. */ + assert(name); ctx = bdrv_get_aio_context(bs); aio_context_acquire(ctx); bdrv_invalidate_cache(bs, NULL); @@ -1494,6 +1496,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, QTAILQ_INIT(&exp->clients); exp->blk = blk; exp->dev_offset = dev_offset; + exp->name = g_strdup(name); + exp->description = g_strdup(description); exp->nbdflags = nbdflags; exp->size = size < 0 ? blk_getlength(blk) : size; if (exp->size < 0) { @@ -1513,10 +1517,14 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, exp->eject_notifier.notify = nbd_eject_notifier; blk_add_remove_bs_notifier(on_eject_blk, &exp->eject_notifier); } + QTAILQ_INSERT_TAIL(&exports, exp, next); + nbd_export_get(exp); return exp; fail: blk_unref(blk); + g_free(exp->name); + g_free(exp->description); g_free(exp); return NULL; } @@ -1533,43 +1541,29 @@ NBDExport *nbd_export_find(const char *name) return NULL; } -void nbd_export_set_name(NBDExport *exp, const char *name) -{ - if (exp->name == name) { - return; - } - - nbd_export_get(exp); - if (exp->name != NULL) { - g_free(exp->name); - exp->name = NULL; - QTAILQ_REMOVE(&exports, exp, next); - nbd_export_put(exp); - } - if (name != NULL) { - nbd_export_get(exp); - exp->name = g_strdup(name); - QTAILQ_INSERT_TAIL(&exports, exp, next); - } - nbd_export_put(exp); -} - -void nbd_export_set_description(NBDExport *exp, const char *description) -{ - g_free(exp->description); - exp->description = g_strdup(description); -} - void nbd_export_close(NBDExport *exp) { NBDClient *client, *next; nbd_export_get(exp); + /* + * TODO: Should we expand QMP NbdServerRemoveNode enum to allow a + * close mode that stops advertising the export to new clients but + * still permits existing clients to run to completion? Because of + * that possibility, nbd_export_close() can be called more than + * once on an export. + */ QTAILQ_FOREACH_SAFE(client, &exp->clients, next, next) { client_close(client, true); } - nbd_export_set_name(exp, NULL); - nbd_export_set_description(exp, NULL); + if (exp->name) { + nbd_export_put(exp); + g_free(exp->name); + exp->name = NULL; + QTAILQ_REMOVE(&exports, exp, next); + } + g_free(exp->description); + exp->description = NULL; nbd_export_put(exp); } diff --git a/qemu-nbd.c b/qemu-nbd.c index 6ca02b6..b93fa19 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -1015,11 +1015,9 @@ int main(int argc, char **argv) } } - export = nbd_export_new(bs, dev_offset, fd_size, nbdflags, - nbd_export_closed, writethrough, - NULL, &error_fatal); - nbd_export_set_name(export, export_name); - nbd_export_set_description(export, export_description); + export = nbd_export_new(bs, dev_offset, fd_size, export_name, + export_description, nbdflags, nbd_export_closed, + writethrough, NULL, &error_fatal); if (device) { #if HAVE_NBD_DEVICE -- cgit v1.1 From 5fcbeb06812685a2c6d7e0e6f28f018987d08b79 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 11 Jan 2019 13:47:17 -0600 Subject: nbd: Allow bitmap export during QMP nbd-server-add With the experimental x-nbd-server-add-bitmap command, there was a window of time where an NBD client could see the export but not the associated dirty bitmap, which can cause a client that planned on using the dirty bitmap to be forced to treat the entire image as dirty as a safety fallback. Furthermore, if the QMP client successfully exports a disk but then fails to add the bitmap, it has to take on the burden of removing the export. Since we don't allow changing the exposed dirty bitmap (whether to a different bitmap, or removing advertisement of the bitmap), it is nicer to make the bitmap tied to the export at the time the export is created, with automatic failure to export if the bitmap is not available. The experimental command included an optional 'bitmap-export-name' field for remapping the name exposed over NBD to be different from the bitmap name stored on disk. However, my libvirt demo code for implementing differential backups on top of persistent bitmaps did not need to take advantage of that feature (it is instead possible to create a new temporary bitmap with the desired name, use block-dirty-bitmap-merge to merge one or more persistent bitmaps into the temporary, then associate the temporary with the NBD export, if control is needed over the exported bitmap name). Hence, I'm not copying that part of the experiment over to the stable addition. For more details on the libvirt demo, see https://www.redhat.com/archives/libvir-list/2018-October/msg01254.html, https://kvmforum2018.sched.com/event/FzuB/facilitating-incremental-backup-eric-blake-red-hat This patch focuses on the user interface, and reduces (but does not completely eliminate) the window where an NBD client can see the export but not the dirty bitmap, with less work to clean up after errors. Later patches will add further cleanups now that this interface is declared stable via a single QMP command, including removing the race window. Update test 223 to use the new interface. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20190111194720.15671-6-eblake@redhat.com> --- blockdev-nbd.c | 12 +++++++++++- hmp.c | 5 +++-- qapi/block.json | 7 ++++++- tests/qemu-iotests/223 | 19 ++++++++----------- tests/qemu-iotests/223.out | 5 +---- 5 files changed, 29 insertions(+), 19 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 582ffde..ec8cf0a 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -140,7 +140,8 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr, } void qmp_nbd_server_add(const char *device, bool has_name, const char *name, - bool has_writable, bool writable, Error **errp) + bool has_writable, bool writable, + bool has_bitmap, const char *bitmap, Error **errp) { BlockDriverState *bs = NULL; BlockBackend *on_eject_blk; @@ -185,6 +186,15 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, * our only way of accessing it is through nbd_export_find(), so we can drop * the strong reference that is @exp. */ nbd_export_put(exp); + + if (has_bitmap) { + Error *err = NULL; + nbd_export_bitmap(exp, bitmap, bitmap, &err); + if (err) { + error_propagate(errp, err); + nbd_export_remove(exp, NBD_SERVER_REMOVE_MODE_HARD, NULL); + } + } } void qmp_nbd_server_remove(const char *name, diff --git a/hmp.c b/hmp.c index 80aa5ab..8da5fd8 100644 --- a/hmp.c +++ b/hmp.c @@ -2326,7 +2326,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict) } qmp_nbd_server_add(info->value->device, false, NULL, - true, writable, &local_err); + true, writable, false, NULL, &local_err); if (local_err != NULL) { qmp_nbd_server_stop(NULL); @@ -2347,7 +2347,8 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict) bool writable = qdict_get_try_bool(qdict, "writable", false); Error *local_err = NULL; - qmp_nbd_server_add(device, !!name, name, true, writable, &local_err); + qmp_nbd_server_add(device, !!name, name, true, writable, + false, NULL, &local_err); hmp_handle_error(mon, &local_err); } diff --git a/qapi/block.json b/qapi/block.json index 11f01f2..3d70420 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -246,6 +246,10 @@ # # @writable: Whether clients should be able to write to the device via the # NBD connection (default false). + +# @bitmap: Also export the dirty bitmap reachable from @device, so the +# NBD client can use NBD_OPT_SET_META_CONTEXT with +# "qemu:dirty-bitmap:NAME" to inspect the bitmap. (since 4.0) # # Returns: error if the server is not running, or export with the same name # already exists. @@ -253,7 +257,8 @@ # Since: 1.3.0 ## { 'command': 'nbd-server-add', - 'data': {'device': 'str', '*name': 'str', '*writable': 'bool'} } + 'data': {'device': 'str', '*name': 'str', '*writable': 'bool', + '*bitmap': 'str' } } ## # @NbdServerRemoveMode: diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223 index f200e31..0bcc98a 100755 --- a/tests/qemu-iotests/223 +++ b/tests/qemu-iotests/223 @@ -126,23 +126,20 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-start", "arguments":{"addr":{"type":"unix", "data":{"path":"'"$TEST_DIR/nbd"1'"}}}}' "error" # Attempt second server _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", - "arguments":{"device":"n"}}' "return" + "arguments":{"device":"n", "bitmap":"b"}}' "return" _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", "arguments":{"device":"nosuch"}}' "error" # Attempt to export missing node _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", "arguments":{"device":"n"}}' "error" # Attempt to export same name twice -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap", - "arguments":{"name":"n", "bitmap":"b"}}' "return" _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", - "arguments":{"device":"n", "name":"n2"}}' "return" -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap", - "arguments":{"name":"n2", "bitmap":"b2"}}' "error" # Enabled vs. read-only -_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove", - "arguments":{"name":"n2"}}' "return" + "arguments":{"device":"n", "name":"n2", + "bitmap":"b2"}}' "error" # enabled vs. read-only +_send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", + "arguments":{"device":"n", "name":"n2", + "bitmap":"b3"}}' "error" # Missing bitmap _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-add", - "arguments":{"device":"n", "name":"n2", "writable":true}}' "return" -_send_qemu_cmd $QEMU_HANDLE '{"execute":"x-nbd-server-add-bitmap", - "arguments":{"name":"n2", "bitmap":"b2"}}' "return" + "arguments":{"device":"n", "name":"n2", "writable":true, + "bitmap":"b2"}}' "return" echo echo "=== Contrast normal status to large granularity dirty-bitmap ===" diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index 7d14729..ebd3935 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -33,11 +33,8 @@ wrote 2097152/2097152 bytes at offset 2097152 {"return": {}} {"error": {"class": "GenericError", "desc": "Cannot find device=nosuch nor node_name=nosuch"}} {"error": {"class": "GenericError", "desc": "NBD server already has export named 'n'"}} -{"return": {}} -{"return": {}} {"error": {"class": "GenericError", "desc": "Enabled bitmap 'b2' incompatible with readonly export"}} -{"return": {}} -{"return": {}} +{"error": {"class": "GenericError", "desc": "Bitmap 'b3' is not found"}} {"return": {}} === Contrast normal status to large granularity dirty-bitmap === -- cgit v1.1 From 7dc570b3806e5b0a4c9219061556ed5a4a0de80c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 11 Jan 2019 13:47:18 -0600 Subject: nbd: Remove x-nbd-server-add-bitmap Now that nbd-server-add can do the same functionality (well, other than making the exported bitmap name different than the underlying bitamp - but we argued that was not essential, since it is just as easy to create a new non-persistent bitmap with the desired name), we no longer need the experimental separate command. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20190111194720.15671-7-eblake@redhat.com> --- blockdev-nbd.c | 23 ----------------------- qapi/block.json | 23 ----------------------- 2 files changed, 46 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index ec8cf0a..cd86b38 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -233,26 +233,3 @@ void qmp_nbd_server_stop(Error **errp) nbd_server_free(nbd_server); nbd_server = NULL; } - -void qmp_x_nbd_server_add_bitmap(const char *name, const char *bitmap, - bool has_bitmap_export_name, - const char *bitmap_export_name, - Error **errp) -{ - NBDExport *exp; - - if (!nbd_server) { - error_setg(errp, "NBD server not running"); - return; - } - - exp = nbd_export_find(name); - if (exp == NULL) { - error_setg(errp, "Export '%s' is not found", name); - return; - } - - nbd_export_bitmap(exp, bitmap, - has_bitmap_export_name ? bitmap_export_name : bitmap, - errp); -} diff --git a/qapi/block.json b/qapi/block.json index 3d70420..5a79d63 100644 --- a/qapi/block.json +++ b/qapi/block.json @@ -302,29 +302,6 @@ 'data': {'name': 'str', '*mode': 'NbdServerRemoveMode'} } ## -# @x-nbd-server-add-bitmap: -# -# Expose a dirty bitmap associated with the selected export. The bitmap search -# starts at the device attached to the export, and includes all backing files. -# The exported bitmap is then locked until the NBD export is removed. -# -# @name: Export name. -# -# @bitmap: Bitmap name to search for. -# -# @bitmap-export-name: How the bitmap will be seen by nbd clients -# (default @bitmap) -# -# Note: the client must use NBD_OPT_SET_META_CONTEXT with a query of -# "qemu:dirty-bitmap:NAME" (where NAME matches @bitmap-export-name) to access -# the exposed bitmap. -# -# Since: 3.0 -## - { 'command': 'x-nbd-server-add-bitmap', - 'data': {'name': 'str', 'bitmap': 'str', '*bitmap-export-name': 'str'} } - -## # @nbd-server-stop: # # Stop QEMU's embedded NBD server, and unregister all devices previously -- cgit v1.1 From 678ba275c77b5b12f3bc9bb369a1b824fc9f679f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 11 Jan 2019 13:47:19 -0600 Subject: nbd: Merge nbd_export_bitmap into nbd_export_new We only have one caller that wants to export a bitmap name, which it does right after creation of the export. But there is still a brief window of time where an NBD client could see the export but not the dirty bitmap, which a robust client would have to interpret as meaning the entire image should be treated as dirty. Better is to eliminate the window entirely, by inlining nbd_export_bitmap() into nbd_export_new(), and refusing to create the bitmap in the first place if the requested bitmap can't be located. We also no longer need logic for setting a different bitmap name compared to the bitmap being exported. Signed-off-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <20190111194720.15671-8-eblake@redhat.com> --- blockdev-nbd.c | 11 +------ include/block/nbd.h | 9 ++---- nbd/server.c | 87 ++++++++++++++++++++++++----------------------------- qemu-nbd.c | 5 +-- 4 files changed, 47 insertions(+), 65 deletions(-) diff --git a/blockdev-nbd.c b/blockdev-nbd.c index cd86b38..c76d541 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -175,7 +175,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, writable = false; } - exp = nbd_export_new(bs, 0, -1, name, NULL, + exp = nbd_export_new(bs, 0, -1, name, NULL, bitmap, writable ? 0 : NBD_FLAG_READ_ONLY, NULL, false, on_eject_blk, errp); if (!exp) { @@ -186,15 +186,6 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, * our only way of accessing it is through nbd_export_find(), so we can drop * the strong reference that is @exp. */ nbd_export_put(exp); - - if (has_bitmap) { - Error *err = NULL; - nbd_export_bitmap(exp, bitmap, bitmap, &err); - if (err) { - error_propagate(errp, err); - nbd_export_remove(exp, NBD_SERVER_REMOVE_MODE_HARD, NULL); - } - } } void qmp_nbd_server_remove(const char *name, diff --git a/include/block/nbd.h b/include/block/nbd.h index 2f9a2ae..1971b55 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -296,9 +296,9 @@ typedef struct NBDClient NBDClient; NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, const char *name, const char *description, - uint16_t nbdflags, void (*close)(NBDExport *), - bool writethrough, BlockBackend *on_eject_blk, - Error **errp); + const char *bitmap, uint16_t nbdflags, + void (*close)(NBDExport *), bool writethrough, + BlockBackend *on_eject_blk, Error **errp); void nbd_export_close(NBDExport *exp); void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp); void nbd_export_get(NBDExport *exp); @@ -319,9 +319,6 @@ void nbd_client_put(NBDClient *client); void nbd_server_start(SocketAddress *addr, const char *tls_creds, Error **errp); -void nbd_export_bitmap(NBDExport *exp, const char *bitmap, - const char *bitmap_export_name, Error **errp); - /* nbd_read * Reads @size bytes from @ioc. Returns 0 on success. */ diff --git a/nbd/server.c b/nbd/server.c index bb5438c..e8c5660 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1457,9 +1457,9 @@ static void nbd_eject_notifier(Notifier *n, void *data) NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, const char *name, const char *description, - uint16_t nbdflags, void (*close)(NBDExport *), - bool writethrough, BlockBackend *on_eject_blk, - Error **errp) + const char *bitmap, uint16_t nbdflags, + void (*close)(NBDExport *), bool writethrough, + BlockBackend *on_eject_blk, Error **errp) { AioContext *ctx; BlockBackend *blk; @@ -1507,6 +1507,43 @@ NBDExport *nbd_export_new(BlockDriverState *bs, off_t dev_offset, off_t size, } exp->size -= exp->size % BDRV_SECTOR_SIZE; + if (bitmap) { + BdrvDirtyBitmap *bm = NULL; + BlockDriverState *bs = blk_bs(blk); + + while (true) { + bm = bdrv_find_dirty_bitmap(bs, bitmap); + if (bm != NULL || bs->backing == NULL) { + break; + } + + bs = bs->backing->bs; + } + + if (bm == NULL) { + error_setg(errp, "Bitmap '%s' is not found", bitmap); + goto fail; + } + + if ((nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) && + bdrv_dirty_bitmap_enabled(bm)) { + error_setg(errp, + "Enabled bitmap '%s' incompatible with readonly export", + bitmap); + goto fail; + } + + if (bdrv_dirty_bitmap_user_locked(bm)) { + error_setg(errp, "Bitmap '%s' is in use", bitmap); + goto fail; + } + + bdrv_dirty_bitmap_set_qmp_locked(bm, true); + exp->export_bitmap = bm; + exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", + bitmap); + } + exp->close = close; exp->ctx = blk_get_aio_context(blk); blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp); @@ -2424,47 +2461,3 @@ void nbd_client_new(QIOChannelSocket *sioc, co = qemu_coroutine_create(nbd_co_client_start, client); qemu_coroutine_enter(co); } - -void nbd_export_bitmap(NBDExport *exp, const char *bitmap, - const char *bitmap_export_name, Error **errp) -{ - BdrvDirtyBitmap *bm = NULL; - BlockDriverState *bs = blk_bs(exp->blk); - - if (exp->export_bitmap) { - error_setg(errp, "Export bitmap is already set"); - return; - } - - while (true) { - bm = bdrv_find_dirty_bitmap(bs, bitmap); - if (bm != NULL || bs->backing == NULL) { - break; - } - - bs = bs->backing->bs; - } - - if (bm == NULL) { - error_setg(errp, "Bitmap '%s' is not found", bitmap); - return; - } - - if ((exp->nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) && - bdrv_dirty_bitmap_enabled(bm)) { - error_setg(errp, - "Enabled bitmap '%s' incompatible with readonly export", - bitmap); - return; - } - - if (bdrv_dirty_bitmap_user_locked(bm)) { - error_setg(errp, "Bitmap '%s' is in use", bitmap); - return; - } - - bdrv_dirty_bitmap_set_qmp_locked(bm, true); - exp->export_bitmap = bm; - exp->export_bitmap_context = - g_strdup_printf("qemu:dirty-bitmap:%s", bitmap_export_name); -} diff --git a/qemu-nbd.c b/qemu-nbd.c index b93fa19..1552274 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -1016,8 +1016,9 @@ int main(int argc, char **argv) } export = nbd_export_new(bs, dev_offset, fd_size, export_name, - export_description, nbdflags, nbd_export_closed, - writethrough, NULL, &error_fatal); + export_description, NULL, nbdflags, + nbd_export_closed, writethrough, NULL, + &error_fatal); if (device) { #if HAVE_NBD_DEVICE -- cgit v1.1 From 636192c4b6052820ea126a5287c58a8f53f3c84f Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 11 Jan 2019 13:47:20 -0600 Subject: qemu-nbd: Add --bitmap=NAME option Having to fire up qemu, then use QMP commands for nbd-server-start and nbd-server-add, just to expose a persistent dirty bitmap, is rather tedious. Make it possible to expose a dirty bitmap using just qemu-nbd (of course, for now this only works when qemu-nbd is visiting a BDS formatted as qcow2). Of course, any good feature also needs unit testing, so expand iotest 223 to cover it. Signed-off-by: Eric Blake Message-Id: <20190111194720.15671-9-eblake@redhat.com> --- qemu-nbd.c | 10 ++++++++-- qemu-nbd.texi | 4 ++++ tests/qemu-iotests/223 | 18 +++++++++++++++++- tests/qemu-iotests/223.out | 12 +++++++++++- 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/qemu-nbd.c b/qemu-nbd.c index 1552274..51b55f2 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -95,6 +95,7 @@ static void usage(const char *name) "Exposing part of the image:\n" " -o, --offset=OFFSET offset into the image\n" " -P, --partition=NUM only expose partition NUM\n" +" -B, --bitmap=NAME expose a persistent dirty bitmap\n" "\n" "General purpose options:\n" " --object type,id=ID,... define an object such as 'secret' for providing\n" @@ -509,7 +510,7 @@ int main(int argc, char **argv) off_t fd_size; QemuOpts *sn_opts = NULL; const char *sn_id_or_name = NULL; - const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:"; + const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:"; struct option lopt[] = { { "help", no_argument, NULL, 'h' }, { "version", no_argument, NULL, 'V' }, @@ -519,6 +520,7 @@ int main(int argc, char **argv) { "offset", required_argument, NULL, 'o' }, { "read-only", no_argument, NULL, 'r' }, { "partition", required_argument, NULL, 'P' }, + { "bitmap", required_argument, NULL, 'B' }, { "connect", required_argument, NULL, 'c' }, { "disconnect", no_argument, NULL, 'd' }, { "snapshot", no_argument, NULL, 's' }, @@ -558,6 +560,7 @@ int main(int argc, char **argv) QDict *options = NULL; const char *export_name = ""; /* Default export name */ const char *export_description = NULL; + const char *bitmap = NULL; const char *tlscredsid = NULL; bool imageOpts = false; bool writethrough = true; @@ -695,6 +698,9 @@ int main(int argc, char **argv) exit(EXIT_FAILURE); } break; + case 'B': + bitmap = optarg; + break; case 'k': sockpath = optarg; if (sockpath[0] != '/') { @@ -1016,7 +1022,7 @@ int main(int argc, char **argv) } export = nbd_export_new(bs, dev_offset, fd_size, export_name, - export_description, NULL, nbdflags, + export_description, bitmap, nbdflags, nbd_export_closed, writethrough, NULL, &error_fatal); diff --git a/qemu-nbd.texi b/qemu-nbd.texi index 9a84e81..96b1546 100644 --- a/qemu-nbd.texi +++ b/qemu-nbd.texi @@ -45,6 +45,10 @@ auto-detecting Export the disk as read-only @item -P, --partition=@var{num} Only expose partition @var{num} +@item -B, --bitmap=@var{name} +If @var{filename} has a qcow2 persistent bitmap @var{name}, expose +that bitmap via the ``qemu:dirty-bitmap:@var{name}'' context +accessible through NBD_OPT_SET_META_CONTEXT. @item -s, --snapshot Use @var{filename} as an external snapshot, create a temporary file with backing_file=@var{filename}, redirect the write to diff --git a/tests/qemu-iotests/223 b/tests/qemu-iotests/223 index 0bcc98a..773892d 100755 --- a/tests/qemu-iotests/223 +++ b/tests/qemu-iotests/223 @@ -25,6 +25,7 @@ status=1 # failure is the default! _cleanup() { + nbd_server_stop _cleanup_test_img _cleanup_qemu rm -f "$TEST_DIR/nbd" @@ -35,6 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15 . ./common.rc . ./common.filter . ./common.qemu +. ./common.nbd _supported_fmt qcow2 _supported_proto file # uses NBD as well @@ -163,7 +165,7 @@ $QEMU_IMG map --output=json --image-opts \ "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map echo -echo "=== End NBD server ===" +echo "=== End qemu NBD server ===" echo _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-remove", @@ -176,6 +178,20 @@ _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "return" _send_qemu_cmd $QEMU_HANDLE '{"execute":"nbd-server-stop"}' "error" # Again _send_qemu_cmd $QEMU_HANDLE '{"execute":"quit"}' "return" +echo +echo "=== Use qemu-nbd as server ===" +echo + +nbd_server_start_unix_socket -r -f $IMGFMT -B b "$TEST_IMG" +IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket" +$QEMU_IMG map --output=json --image-opts \ + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b" | _filter_qemu_img_map + +nbd_server_start_unix_socket -f $IMGFMT -B b2 "$TEST_IMG" +IMG="driver=nbd,server.type=unix,server.path=$nbd_unix_socket" +$QEMU_IMG map --output=json --image-opts \ + "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map + # success, all done echo '*** done' rm -f $seq.full diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out index ebd3935..0de5240 100644 --- a/tests/qemu-iotests/223.out +++ b/tests/qemu-iotests/223.out @@ -61,7 +61,7 @@ read 2097152/2097152 bytes at offset 2097152 { "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true}, { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}] -=== End NBD server === +=== End qemu NBD server === {"return": {}} {"return": {}} @@ -69,4 +69,14 @@ read 2097152/2097152 bytes at offset 2097152 {"return": {}} {"error": {"class": "GenericError", "desc": "NBD server not running"}} {"return": {}} + +=== Use qemu-nbd as server === + +[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": false}, +{ "start": 65536, "length": 2031616, "depth": 0, "zero": false, "data": true}, +{ "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}] +[{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true}, +{ "start": 512, "length": 512, "depth": 0, "zero": false, "data": false}, +{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true}, +{ "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}] *** done -- cgit v1.1