Modify block jobs to use node-names
v5 cover Modify block jobs to use node-names
Changes from v4->v5:
* Rebased on master
* Fixed commit log typos / stale paragraphs (Eric)
* Fixed comment typo (Eric)
* Added Eric's remaining R-b's
Changes from v3->v4:
* Rebased on master
* Dropped overlay pointers, Eric's concerns are correct
* Require "device" for all arguments, in light of the above, so we can find the active layer in all cases.
* Simplify more operations!
* Dropped Eric's Reviewed-by: on patches 3,5,6 Added Eric's Reviewed-by: on patches 8,9
Changes from v2->v3:
* Add Eric's reviewed-by
* Addressed Eric's review comments
* Dropped HMP changes
* Added helper function for setting the overlay, and
set the overlay in bdrv_append()
* Use bs->backing_file instead of bs->backing_hd->filename in block_stream
Using node-names instead of filenames for block job operations over QMP is a superior method of identifying the block driver images to operate on, as it removes all pathname ambiguity.
This series modifies block-commit and block-stream to use node-names, and creates a new QAPI command to allow stand-alone backing file changes on an image file.
So that node-names can be used as desired for all block job operations, this series also auto-generates node-names for every BDS. User-specified node-names will override any autogenerated
Jeff Cody (10):
block: Auto-generate node_names for each BDS entry
block: add helper function to determine if a BDS is in a chain
block: simplify bdrv_find_base() and bdrv_find_overlay()
block: make 'top' argument to block-commit optional
block: Accept node-name arguments for block-commit
block: extend block-commit to accept a string for the backing file
block: add ability for block-stream to use node-name
block: add backing-file option to block-stream
block: Add QMP documentation for block-stream
block: add QAPI command to allow live backing file change
block.c | 80 ++++++++-------- block/commit.c | 9 +- block/stream.c | 11 +-- blockdev.c | 228 ++++++++++++++++++++++++++++++++++++++++++---- hmp.c | 1 + include/block/block.h | 4 +- include/block/block_int.h | 3 +- qapi/block-core.json | 145 ++++++++++++++++++++++++++--- qmp-commands.hx | 181 ++++++++++++++++++++++++++++++++++-- tests/qemu-iotests/040 | 28 ++++-- 10 files changed, 592 insertions(+), 98 deletions(-)
Signature · 1 line
1.8.3.1
PATCH v5 1/10 block: Auto-generate node_names for each BDS entry
Currently, node_name is only filled in when done so explicitly by the user. If no node_name is specified, then the node name field is not populated.
If node_names are automatically generated when not specified, that means that all block job operations can be done by reference to the unique node_name field. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.
If a node name is specified, then it will not be automatically generated for that BDS entry.
If it is automatically generated, it will be prefaced with "__qemu##", followed by 8 characters of a unique number, followed by 8 random ASCII characters in the range of 'A-Z'. Some sample generated node-name strings:
__qemu##00000000IAIYNXXR
__qemu##00000002METXTRBQ
__qemu##00000001FMBORDWGThe prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.
--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
block.c +15 -1
@@ -844,12 +844,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)return open_flags; } +#define GEN_NODE_NAME_PREFIX "__qemu##" +#define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)static void bdrv_assign_node_name (BlockDriverState *bs,const char *node_name,Error **errp) { +char gen_node_name[GEN_NODE_NAME_MAX_LEN ]; +static uint32_t counter;/* simple counter to guarantee uniqueness */ + +/* if node_name is NULL, auto-generate a node name */ if (!node_name) { -return ; +int len; +snprintf (gen_node_name,GEN_NODE_NAME_MAX_LEN , +"%s%08x" ,GEN_NODE_NAME_PREFIX , counter++); + len =strlen (gen_node_name); +while (len <GEN_NODE_NAME_MAX_LEN -1 ) { + gen_node_name[len++] =g_random_int_range ('A' ,'Z' ); + } + gen_node_name[GEN_NODE_NAME_MAX_LEN -1 ] ='\0' ; + node_name = gen_node_name; }/* empty string node name is invalid */
Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH v5 01/10] block: Auto-generate node_names for each BDS entry
Quoted 17 lines · Jeff Cody
Currently,
node_nameis only filled in when done so explicitly by the user. If nonode_nameis specified, then the node name field is not populated.If
node_namesare automatically generated when not specified, that means that all block job operations can be done by reference to the uniquenode_namefield. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.If a node name is specified, then it will not be automatically generated for that BDS entry.
If it is automatically generated, it will be prefaced with "__qemu##", followed by 8 characters of a unique number, followed by 8 random ASCII characters in the range of 'A-Z'. Some sample generated node-name strings:
Quoted 8 lines
__qemu##00000000IAIYNXXR __qemu##00000002METXTRBQ __qemu##00000001FMBORDWGThe prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.
if the __qemu## determined a different name space no collision resolution would be needed.
Also Eric you must take care that these node name does not ends up in libvirt XML since a new qemu process can generate a new different set of node name.
Quoted 43 lines
Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)block.c +15
-1@@ -844,12 +844,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)return open_flags; } +#define GEN_NODE_NAME_PREFIX "__qemu##" +#define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)static void bdrv_assign_node_name (BlockDriverState *bs,const char *node_name,Error **errp) { +char gen_node_name[GEN_NODE_NAME_MAX_LEN ]; +static uint32_t counter;/* simple counter to guarantee uniqueness */ + +/* if node_name is NULL, auto-generate a node name */ if (!node_name) { -return ; +int len; +snprintf (gen_node_name,GEN_NODE_NAME_MAX_LEN , +"%s%08x" ,GEN_NODE_NAME_PREFIX , counter++); + len =strlen (gen_node_name); +while (len <GEN_NODE_NAME_MAX_LEN -1 ) { + gen_node_name[len++] =g_random_int_range ('A' ,'Z' ); + } + gen_node_name[GEN_NODE_NAME_MAX_LEN -1 ] ='\0' ; + node_name = gen_node_name; }/* empty string node name is invalid */ Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH v5 01/10] block: Auto-generate node_names for each BDS entry
Quoted 10 lines · Benoît Canet
Quoted 4 lines
The prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.
if the __qemu## determined a different name space no collision resolution would be needed.
Also Eric you must take care that these node name does not ends up in libvirt XML since a new qemu process can generate a new different set of node name.
Correct. Either libvirt will always generate its own node names (and the presence of a qemu-generated name is a bug in libvirt), or libvirt will query node-names as it runs, and clear them out when the qemu process goes away. It will NOT expose qemu names to user-visible XML (although it should be safe to save the qemu names to internal-only XML used to cache results across libvirtd restarts while still communicating to the same qemu process).
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v5 01/10] block: Auto-generate node_names for each BDS entry
In reply to Benoît CanetQuoted 22 lines · Benoît Canet
Quoted 21 lines · Jeff Cody
Currently,
node_nameis only filled in when done so explicitly by the user. If nonode_nameis specified, then the node name field is not populated.If
node_namesare automatically generated when not specified, that means that all block job operations can be done by reference to the uniquenode_namefield. This eliminates ambiguity in resolving filenames (relative filenames, or file descriptors, symlinks, mounts, etc..) that qemu currently needs to deal with.If a node name is specified, then it will not be automatically generated for that BDS entry.
If it is automatically generated, it will be prefaced with "__qemu##", followed by 8 characters of a unique number, followed by 8 random ASCII characters in the range of 'A-Z'. Some sample generated node-name strings:
__qemu##00000000IAIYNXXR __qemu##00000002METXTRBQ __qemu##00000001FMBORDWG
Quoted 8 lines
Quoted 4 lines
The prefix is to aid in identifying it as a qemu-generated name, the numeric portion is to guarantee uniqueness in a given qemu session, and the random characters are to further avoid any accidental collisions with user-specified node-names.
if the __qemu## determined a different name space no collision resolution would be needed.
True. The random string generation was pretty trivial, however, and has an added benefit: it helps prevent the user / management software from assuming or attempting to predict node names for images.
Quoted 3 lines
Also Eric you must take care that these node name does not ends up in libvirt XML since a new qemu process can generate a new different set of node name.
Yes.. the node names are guaranteed to be unique, but are ephemeral in nature.
Quoted 43 lines
Quoted 43 lines
Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)block.c +15
-1@@ -844,12 +844,26 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)return open_flags; } +#define GEN_NODE_NAME_PREFIX "__qemu##" +#define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)static void bdrv_assign_node_name (BlockDriverState *bs,const char *node_name,Error **errp) { +char gen_node_name[GEN_NODE_NAME_MAX_LEN ]; +static uint32_t counter;/* simple counter to guarantee uniqueness */ + +/* if node_name is NULL, auto-generate a node name */ if (!node_name) { -return ; +int len; +snprintf (gen_node_name,GEN_NODE_NAME_MAX_LEN , +"%s%08x" ,GEN_NODE_NAME_PREFIX , counter++); + len =strlen (gen_node_name); +while (len <GEN_NODE_NAME_MAX_LEN -1 ) { + gen_node_name[len++] =g_random_int_range ('A' ,'Z' ); + } + gen_node_name[GEN_NODE_NAME_MAX_LEN -1 ] ='\0' ; + node_name = gen_node_name; }/* empty string node name is invalid */ Signature · 1 line
1.8.3.1
PATCH v5 2/10 block: add helper function to determine if a BDS is in a chain
In reply to Jeff CodyThis is a small helper function, to determine if 'base' is in the chain of BlockDriverState 'top'. It returns true if it is in the chain, and false otherwise.
If either argument is NULL, it will also return false.
--- block.c | 11 +++++++++++ include/block/block.h | 1 + 2 files changed, 12 insertions(+)
block.c +11 -0
@@ -3822,6 +3822,17 @@ BlockDriverState *bdrv_lookup_bs(const char *device,return NULL ; } +/* If 'base' is in the same chain as 'top', return true. Otherwise, +* return false. If either argument is NULL, return false. */ +bool bdrv_chain_contains (BlockDriverState *top,BlockDriverState *base) +{ +while (top && top != base) { + top = top->backing_hd; + } + +return top !=NULL ; +} +BlockDriverState *bdrv_next (BlockDriverState *bs) {if (!bs) {
include/block/block.h +1 -0
@@ -404,6 +404,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void);BlockDriverState *bdrv_lookup_bs (const char *device,const char *node_name,Error **errp); +bool bdrv_chain_contains (BlockDriverState *top,BlockDriverState *base);BlockDriverState *bdrv_next (BlockDriverState *bs);void bdrv_iterate (void (*it)(void *opaque,BlockDriverState *bs),void *opaque);
Signature · 1 line
1.8.3.1
PATCH v5 3/10 block: simplify bdrv_find_base() and bdrv_find_overlay()
In reply to Jeff CodyThis simplifies the function bdrv_find_overlay(). With this change, bdrv_find_base() is just a subset of usage of bdrv_find_overlay(), so this also takes advantage of that.
--- block.c | 45 ++++++++++----------------------------------- 1 file changed, 10 insertions(+), 35 deletions(-)
block.c +10 -35
@@ -2569,32 +2569,23 @@ int bdrv_change_backing_file(BlockDriverState *bs,* * Returns NULL if bs is not found in active's image chain, * or if active == bs. +* +* Returns the bottommost base image if bs == NULL. */ BlockDriverState *bdrv_find_overlay (BlockDriverState *active,BlockDriverState *bs) { -BlockDriverState *overlay =NULL ; -BlockDriverState *intermediate; - -assert (active !=NULL ); -assert (bs !=NULL ); - -/* if bs is the same as active, then by definition it has no overlay -*/ -if (active == bs) { -return NULL ; +while (active && bs != active->backing_hd) { + active = active->backing_hd; } - intermediate = active; -while (intermediate->backing_hd) { -if (intermediate->backing_hd == bs) { - overlay = intermediate; -break ; - } - intermediate = intermediate->backing_hd; - } +return active; +} -return overlay; +/* Given a BDS, searches for the base layer. */ +BlockDriverState *bdrv_find_base (BlockDriverState *bs) +{ +return bdrv_find_overlay (bs,NULL ); } typedef struct BlkIntermediateStates {
@@ -4376,22 +4367,6 @@ int bdrv_get_backing_file_depth(BlockDriverState *bs)return 1 +bdrv_get_backing_file_depth (bs->backing_hd); } -BlockDriverState *bdrv_find_base (BlockDriverState *bs) -{ -BlockDriverState *curr_bs =NULL ; - -if (!bs) { -return NULL ; - } - - curr_bs = bs; - -while (curr_bs->backing_hd) { - curr_bs = curr_bs->backing_hd; - } -return curr_bs; -} -/**************************************************************/ /* async I/Os */
Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH v5 03/10] block: simplify bdrv_find_base() and bdrv_find_overlay()
Quoted 84 lines · Jeff Cody
This simplifies the function
bdrv_find_overlay(). With this change,bdrv_find_base()is just a subset of usage ofbdrv_find_overlay(), so this also takes advantage of that.Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>--- block.c | 45 ++++++++++----------------------------------- 1 file changed, 10 insertions(+), 35 deletions(-)block.c +10
-35@@ -2569,32 +2569,23 @@ int bdrv_change_backing_file(BlockDriverState *bs,* * Returns NULL if bs is not found in active's image chain, * or if active == bs. +* +* Returns the bottommost base image if bs == NULL. */ BlockDriverState *bdrv_find_overlay (BlockDriverState *active,BlockDriverState *bs) { -BlockDriverState *overlay =NULL ; -BlockDriverState *intermediate; - -assert (active !=NULL ); -assert (bs !=NULL ); - -/* if bs is the same as active, then by definition it has no overlay -*/ -if (active == bs) { -return NULL ; +while (active && bs != active->backing_hd) { + active = active->backing_hd; } - intermediate = active; -while (intermediate->backing_hd) { -if (intermediate->backing_hd == bs) { - overlay = intermediate; -break ; - } - intermediate = intermediate->backing_hd; - } +return active; +} -return overlay; +/* Given a BDS, searches for the base layer. */ +BlockDriverState *bdrv_find_base (BlockDriverState *bs) +{ +return bdrv_find_overlay (bs,NULL ); } typedef struct BlkIntermediateStates {@@ -4376,22 +4367,6 @@ int bdrv_get_backing_file_depth(BlockDriverState *bs)return 1 +bdrv_get_backing_file_depth (bs->backing_hd); } -BlockDriverState *bdrv_find_base (BlockDriverState *bs) -{ -BlockDriverState *curr_bs =NULL ; - -if (!bs) { -return NULL ; - } - - curr_bs = bs; - -while (curr_bs->backing_hd) { - curr_bs = curr_bs->backing_hd; - } -return curr_bs; -} -/**************************************************************/ /* async I/Os */ Signature · 1 line
1.8.3.1
That is a neat simplification.
PATCH v5 4/10 block: make 'top' argument to block-commit optional
In reply to Jeff CodyNow that active layer block-commit is supported, the 'top' argument no longer needs to be mandatory.
Change it to optional, with the default being the active layer in the device chain.
--- blockdev.c | 5 +++-- qapi/block-core.json | 7 ++++--- qmp-commands.hx | 5 +++-- tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- 4 files changed, 28 insertions(+), 17 deletions(-)
blockdev.c +3 -2
@@ -1920,7 +1920,8 @@ void qmp_block_stream(const char *device, bool has_base, }void qmp_block_commit (const char *device, -bool has_base,const char *base,const char *top, +bool has_base,const char *base, +bool has_top,const char *top,bool has_speed,int64_t speed,Error **errp) {
@@ -1952,7 +1953,7 @@ void qmp_block_commit(const char *device,/* default top_bs is the active layer */ top_bs = bs; -if (top) { +if (has_top && top) {if (strcmp (bs->filename, top) !=0 ) { top_bs =bdrv_find_backing_image (bs, top); }
qapi/block-core.json +4 -3
@@ -690,8 +690,9 @@ # @base: #optional The file name of the backing image to write data into. # If not specified, this is the deepest backing image # -# @top: The file name of the backing image within the image chain, -# which contains the topmost data to be committed down. +# @top: #optional The file name of the backing image within the image chain, +# which contains the topmost data to be committed down. If +# not specified, this is the active layer. # # If top == base, that is an error. # If top == active, the job will not be completed by itself,
@@ -719,7 +720,7 @@ # ## { 'command': 'block-commit', - 'data': { 'device': 'str', '*base': 'str', 'top': 'str', + 'data': { 'device': 'str', '*base': 'str', '*top': 'str', '*speed': 'int' } } ##
qmp-commands.hx +3 -2
@@ -985,7 +985,7 @@ EQMP { .name = "block-commit", - .args_type = "device:B,base:s?,top:s,speed:o?", + .args_type = "device:B,base:s?,top:s?,speed:o?", .mhandler.cmd_new = qmp_marshal_input_block_commit, },
@@ -1003,7 +1003,8 @@ Arguments: If not specified, this is the deepest backing image (json-string, optional) - "top": The file name of the backing image within the image chain, - which contains the topmost data to be committed down. + which contains the topmost data to be committed down. If + not specified, this is the active layer. (json-string, optional) If top == base, that is an error. If top == active, the job will not be completed by itself,
tests/qemu-iotests/040 +18 -10
@@ -35,11 +35,7 @@ test_img = os.path.join(iotests.test_dir, 'test.img') class ImageCommitTestCase(iotests.QMPTestCase): '''Abstract base class for image commit test cases''' - def run_commit_test(self, top, base): - self.assert_no_active_block_jobs() - result = self.vm.qmp('block-commit', device='drive0', top=top, base=base) - self.assert_qmp(result, 'return', {}) - + def wait_for_complete(self): completed = False while not completed: for event in self.vm.get_qmp_events(wait=True):
@@ -58,6 +54,18 @@ class ImageCommitTestCase(iotests.QMPTestCase): self.assert_no_active_block_jobs() self.vm.shutdown() + def run_commit_test(self, top, base): + self.assert_no_active_block_jobs() + result = self.vm.qmp('block-commit', device='drive0', top=top, base=base) + self.assert_qmp(result, 'return', {}) + self.wait_for_complete() + + def run_default_commit_test(self): + self.assert_no_active_block_jobs() + result = self.vm.qmp('block-commit', device='drive0') + self.assert_qmp(result, 'return', {}) + self.wait_for_complete() + class TestSingleDrive(ImageCommitTestCase): image_len = 1 * 1024 * 1024 test_len = 1 * 1024 * 256
@@ -109,17 +117,17 @@ class TestSingleDrive(ImageCommitTestCase): self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed")) self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed")) + def test_top_is_default_active(self): + self.run_default_commit_test() + self.assertEqual(-1, qemu_io('-c', 'read -P 0xab 0 524288', backing_img).find("verification failed")) + self.assertEqual(-1, qemu_io('-c', 'read -P 0xef 524288 524288', backing_img).find("verification failed")) + def test_top_and_base_reversed(self): self.assert_no_active_block_jobs() result = self.vm.qmp('block-commit', device='drive0', top='%s' % backing_img, base='%s' % mid_img) self.assert_qmp(result, 'error/class', 'GenericError') self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img) - def test_top_omitted(self): - self.assert_no_active_block_jobs() - result = self.vm.qmp('block-commit', device='drive0') - self.assert_qmp(result, 'error/class', 'GenericError') - self.assert_qmp(result, 'error/desc', "Parameter 'top' is missing") class TestRelativePaths(ImageCommitTestCase): image_len = 1 * 1024 * 1024
Signature · 1 line
1.8.3.1
PATCH v5 5/10 block: Accept node-name arguments for block-commit
In reply to Jeff CodyThis modifies the block operation block-commit so that it will accept node-name arguments for either 'top' or 'base' BDS.
The filename and node-name are mutually exclusive to each other; i.e.:
"top" and "top-node-name" are mutually exclusive (enforced) "base" and "base-node-name" are mutually exclusive (enforced)
The preferred and recommended method now of using block-commit is to specify the BDS to operate on via their node-name arguments.
This also adds an explicit check that base_bs is in the chain of top_bs, because if they are referenced by their unique node-name arguments, the discovery process does not intrinsically verify they are in the same chain.
--- blockdev.c | 55 ++++++++++++++++++++++++++++++++++++++++++++-------- qapi/block-core.json | 37 +++++++++++++++++++++++++---------- qmp-commands.hx | 31 +++++++++++++++++++++-------- 3 files changed, 97 insertions(+), 26 deletions(-)
blockdev.c +47 -8
@@ -1921,12 +1921,15 @@ void qmp_block_stream(const char *device, bool has_base,void qmp_block_commit (const char *device,bool has_base,const char *base, +bool has_base_node_name,const char *base_node_name,bool has_top,const char *top, +bool has_top_node_name,const char *top_node_name,bool has_speed,int64_t speed,Error **errp) { -BlockDriverState *bs; -BlockDriverState *base_bs, *top_bs; +BlockDriverState *bs =NULL ; +BlockDriverState *base_bs =NULL ; +BlockDriverState *top_bs =NULL ;Error *local_err =NULL ;/* This will be part of the QMP command, if/when the * BlockdevOnError change for blkmirror makes it in
@@ -1940,20 +1943,33 @@ void qmp_block_commit(const char *device,/* drain all i/o before commits */ bdrv_drain_all (); - bs =bdrv_find (device); -if (!bs) { -error_set (errp,QERR_DEVICE_NOT_FOUND , device); +/* argument combination validation */ +if (has_base && has_base_node_name) { +error_setg (errp,"'base' and 'base-node-name' are mutually exclusive" ); +return ; + } +if (has_top && has_top_node_name) { +error_setg (errp,"'top' and 'top-node-name' are mutually exclusive" );return ; } -if (bdrv_op_is_blocked (bs,BLOCK_OP_TYPE_COMMIT , errp)) { +/* device lookups */ + bs =bdrv_find (device); +if (!bs) { +error_set (errp,QERR_DEVICE_NOT_FOUND , device);return ; }/* default top_bs is the active layer */ top_bs = bs; -if (has_top && top) { +if (has_top_node_name) { + top_bs =bdrv_lookup_bs (NULL , top_node_name, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } + }else if (has_top && top) {if (strcmp (bs->filename, top) !=0 ) { top_bs =bdrv_find_backing_image (bs, top); }
@@ -1964,7 +1980,13 @@ void qmp_block_commit(const char *device,return ; } -if (has_base && base) { +if (has_base_node_name) { + base_bs =bdrv_lookup_bs (NULL , base_node_name, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } + }else if (has_base && base) { base_bs =bdrv_find_backing_image (top_bs, base); }else { base_bs =bdrv_find_base (top_bs);
@@ -1975,6 +1997,23 @@ void qmp_block_commit(const char *device,return ; } +/* Verify that 'base' is in the same chain as 'top' */ +if (!bdrv_chain_contains (top_bs, base_bs)) { +error_setg (errp,"'base' and 'top' are not in the same chain" ); +return ; + } + +/* This should technically be caught in commit_start, but +* check here for validation completeness */ +if (!bdrv_chain_contains (bs, top_bs)) { +error_setg (errp,"'%s' and 'top' are not in the same chain" , device); +return ; + } + +if (bdrv_op_is_blocked (bs,BLOCK_OP_TYPE_COMMIT , errp)) { +return ; + } +if (top_bs == bs) {commit_active_start (bs, base_bs, speed, on_error, block_job_cb, bs, &local_err);
qapi/block-core.json +27 -10
@@ -685,14 +685,29 @@ # Live commit of data from overlay image nodes into backing nodes - i.e., # writes data between 'top' and 'base' into 'base'. # -# @device: the name of the device +# @device: The name of the device. # -# @base: #optional The file name of the backing image to write data into. -# If not specified, this is the deepest backing image +# For 'base', either @base or @base-node-name may be set but not both. If +# neither is specified, this is the deepest backing image. # -# @top: #optional The file name of the backing image within the image chain, -# which contains the topmost data to be committed down. If -# not specified, this is the active layer. +# @base: #optional The file name of the backing image to write data +# into. +# +# @base-node-name #optional The name of the block driver state node of the +# backing image to write data into. +# (Since 2.1) +# +# For 'top', either @top or @top-node-name must be set but not both. If +# neither is specified, this is the active layer. +# +# @top: #optional The file name of the backing image within the image +# chain, which contains the topmost data to be +# committed down. +# +# @top-node-name: #optional The block driver state node name of the backing +# image within the image chain, which contains the +# topmost data to be committed down. +# (Since 2.1) # # If top == base, that is an error. # If top == active, the job will not be completed by itself,
@@ -711,17 +726,19 @@ # # Returns: Nothing on success # If commit or stream is already active on this device, DeviceInUse -# If @device does not exist, DeviceNotFound +# If @device does not exist or cannot be determined, DeviceNotFound # If image commit is not supported by this device, NotSupported -# If @base or @top is invalid, a generic error is returned +# If @base, @top, @base-node-name, @top-node-name invalid, GenericError # If @speed is invalid, InvalidParameter +# If both @base and @base-node-name are specified, GenericError +# If both @top and @top-node-name are specified, GenericError # # Since: 1.3 # ## { 'command': 'block-commit', - 'data': { 'device': 'str', '*base': 'str', '*top': 'str', - '*speed': 'int' } } + 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', + '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } } ## # @drive-backup
qmp-commands.hx +23 -8
@@ -985,7 +985,7 @@ EQMP { .name = "block-commit", - .args_type = "device:B,base:s?,top:s?,speed:o?", + .args_type = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?", .mhandler.cmd_new = qmp_marshal_input_block_commit, },
@@ -998,13 +998,28 @@ data between 'top' and 'base' into 'base'. Arguments: -- "device": The device's ID, must be unique (json-string) -- "base": The file name of the backing image to write data into. - If not specified, this is the deepest backing image - (json-string, optional) -- "top": The file name of the backing image within the image chain, - which contains the topmost data to be committed down. If - not specified, this is the active layer. (json-string, optional) +- "device": The device's ID, must be unique (json-string) + +For 'base', either base or base-node-name may be set but not both. If +neither is specified, this is the deepest backing image + +- "base": The file name of the backing image to write data into. + (json-string, optional) +- "base-node-name": The name of the block driver state node of the + backing image to write data into. + (json-string, optional) (Since 2.1) + +For 'top', either top or top-node-name must be set but not both. If +neither is specified, this is the active layer + +- "top": The file name of the backing image within the image chain, + which contains the topmost data to be committed down. + (json-string, optional) + +- "top-node-name": The block driver state node name of the backing + image within the image chain, which contains the + topmost data to be committed down. + (json-string, optional) (Since 2.1) If top == base, that is an error. If top == active, the job will not be completed by itself,
Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH v5 05/10] block: Accept node-name arguments for block-commit
Quoted 21 lines · Jeff Cody
This modifies the block operation block-commit so that it will accept node-name arguments for either 'top' or 'base' BDS.
void qmp_block_commit(const char *device,
boolhas_base, const char *base,blockdev.c +5
-2+ bool has_base_node_name, const char *base_node_name, bool has_top, const char *top, + bool has_top_node_name, const char *top_node_name, bool has_speed, int64_t speed, Error **errp) { - BlockDriverState *bs; -BlockDriverState *base_bs, *top_bs; + BlockDriverState *bs = NULL; +BlockDriverState *base_bs =NULL ; +BlockDriverState *top_bs =NULL ;Error *local_err =NULL ;/* This will be part of the QMP command, if/when the * BlockdevOnError change for blkmirror makes it in
blockdev.c void qmp_block_commit(const char *device, · line 1945
Quoted 6 lines
blockdev.c +0
-3@@ -1940,20 +1943,33 @@ void qmp_block_commit(const char *device,/* drain all i/o before commits */ bdrv_drain_all (); - bs =bdrv_find (device); -if (!bs) { -error_set (errp,QERR_DEVICE_NOT_FOUND , device);
On IRC, we discussed that libvirt would like to rely on QERR_DEVICE_NOT_FOUND (aka "class":"DeviceNotFound") as a witness probe for whether qemu supports optional 'top' (which is a rough approximation for whether qemu supports active commit, other than it mis-diagnoses qemu 2.0; but libvirt probably wants to just require qemu 2.1 for active commit anyways). Compare:
on 2.1: {"execute":"block-commit","arguments":{"device":"no-such"}} {"error": {"class": "DeviceNotFound", "desc": "Device 'no-such' not found"}} on 1.7: {"execute":"block-commit","arguments":{"device":"no-such"}} {"error": {"class": "GenericError", "desc": "Parameter 'top' is missing"}}
It might be worth a comment in the code that the device not found error from a failed bdrv_find() should occur as early as possible, at least in the case where all optional parameters are omitted, to serve as a libvirt probe point (it is okay for the code to have various GenericError sanity checks occur before the bdrv_find, so long as those checks can only be triggered when optional parameters are present - but again, worth mentioning in a comment added to the code).
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH v5 6/10 block: extend block-commit to accept a string for the backing file
In reply to Jeff CodyOn some image chains, QEMU may not always be able to resolve the filenames properly, when updating the backing file of an image after a block commit.
For instance, certain relative pathnames may fail, or drives may have been specified originally by file descriptor (e.g. /dev/fd/???), or a relative protocol pathname may have been used.
In these instances, QEMU may lack the information to be able to make the correct choice, but the user or management layer most likely does have that knowledge.
With this extension to the block-commit api, the user is able to change the backing file of the overlay image as part of the block-commit operation.
This allows the change to be 'safe', in the sense that if the attempt to write the overlay image metadata fails, then the block-commit operation returns failure, without disrupting the guest.
If the commit top is the active layer, then specifying the backing file string will be treated as an error (there is no overlay image to modify in that case).
If a backing file string is not specified in the command, the backing file string to use is determined in the same manner as it was previously.
--- block.c | 8 ++++++-- block/commit.c | 9 ++++++--- blockdev.c | 8 +++++++- include/block/block.h | 3 ++- include/block/block_int.h | 3 ++- qapi/block-core.json | 23 +++++++++++++++++++++-- qmp-commands.hx | 19 ++++++++++++++++++- 7 files changed, 62 insertions(+), 11 deletions(-)
block.c +6 -2
@@ -2616,12 +2616,15 @@ typedef struct BlkIntermediateStates {* * base <- active * +* If backing_file_str is non-NULL, it will be used when modifying top's +* overlay image metadata. +* * Error conditions: * if active == top, that is considered an error * */ int bdrv_drop_intermediate (BlockDriverState *active,BlockDriverState *top, -BlockDriverState *base) +BlockDriverState *base,const char *backing_file_str) {BlockDriverState *intermediate;BlockDriverState *base_bs =NULL ;
@@ -2673,7 +2676,8 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, }/* success - we can delete the intermediate states, and link top->base */ - ret =bdrv_change_backing_file (new_top_bs, base_bs->filename, + backing_file_str = backing_file_str ? backing_file_str : base_bs->filename; + ret =bdrv_change_backing_file (new_top_bs, backing_file_str, base_bs->drv ? base_bs->drv->format_name :"" );if (ret) { goto exit;
block/commit.c +6 -3
@@ -37,6 +37,7 @@ typedef struct CommitBlockJob {BlockdevOnError on_error;int base_flags;int orig_overlay_flags; +char *backing_file_str; } CommitBlockJob; static int coroutine_fn commit_populate(BlockDriverState *bs,
@@ -141,7 +142,7 @@ wait:if (!block_job_is_cancelled (&s->common) && sector_num == end) {/* success */ - ret =bdrv_drop_intermediate (active, top, base); + ret =bdrv_drop_intermediate (active, top, base, s->backing_file_str); } exit_free_buf:
@@ -158,7 +159,7 @@ exit_restore_reopen:if (overlay_bs && s->orig_overlay_flags !=bdrv_get_flags (overlay_bs)) {bdrv_reopen (overlay_bs, s->orig_overlay_flags,NULL ); } - +g_free (s->backing_file_str);block_job_completed (&s->common, ret); }
@@ -182,7 +183,7 @@ static const BlockJobDriver commit_job_driver = {void commit_start (BlockDriverState *bs,BlockDriverState *base,BlockDriverState *top,int64_t speed,BlockdevOnError on_error,BlockDriverCompletionFunc *cb, -void *opaque,Error **errp) +void *opaque,const char *backing_file_str,Error **errp) {CommitBlockJob *s;BlockReopenQueue *reopen_queue =NULL ;
@@ -244,6 +245,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base, s->base_flags = orig_base_flags; s->orig_overlay_flags = orig_overlay_flags; + s->backing_file_str =g_strdup (backing_file_str); + s->on_error = on_error; s->common.co =qemu_coroutine_create (commit_run);
blockdev.c +7 -1
@@ -1924,6 +1924,7 @@ void qmp_block_commit(const char *device, bool has_base_node_name, const char *base_node_name, bool has_top, const char *top, bool has_top_node_name, const char *top_node_name, + bool has_backing_file, const char *backing_file, bool has_speed, int64_t speed, Error **errp) {
@@ -2015,11 +2016,16 @@ void qmp_block_commit(const char *device, }if (top_bs == bs) { +if (has_backing_file) { +error_setg (errp,"'backing-file' specified," +" but 'top' is the active layer" ); +return ; + }commit_active_start (bs, base_bs, speed, on_error, block_job_cb, bs, &local_err); }else {commit_start (bs, base_bs, top_bs, speed, on_error, block_job_cb, bs, - &local_err); + has_backing_file ? backing_file :NULL , &local_err); }if (local_err !=NULL ) {error_propagate (errp, local_err);
include/block/block.h +2 -1
@@ -289,7 +289,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file, const char *backing_fmt);void bdrv_register (BlockDriver *bdrv);int bdrv_drop_intermediate (BlockDriverState *active,BlockDriverState *top, -BlockDriverState *base); +BlockDriverState *base, +const char *backing_file_str);BlockDriverState *bdrv_find_overlay (BlockDriverState *active,BlockDriverState *bs);BlockDriverState *bdrv_find_base (BlockDriverState *bs);
include/block/block_int.h +2 -1
@@ -465,13 +465,14 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,* @on_error: The action to take upon error. * @cb: Completion function for the job. * @opaque: Opaque pointer value passed to @cb. +* @backing_file_str: String to use as the backing file in @top's overlay * @errp: Error object. * */ void commit_start (BlockDriverState *bs,BlockDriverState *base,BlockDriverState *top,int64_t speed,BlockdevOnError on_error,BlockDriverCompletionFunc *cb, -void *opaque,Error **errp); +void *opaque,const char *backing_file_str,Error **errp);/** * commit_active_start: * @bs: Active block device to be committed.
qapi/block-core.json +21 -2
@@ -709,6 +709,23 @@ # topmost data to be committed down. # (Since 2.1) # +# @backing-file: #optional The backing file string to write into the overlay +# image of 'top'. If 'top' is the active layer, +# specifying a backing file string is an error. This +# filename is not validated. +# +# If a pathname string is such that it cannot be +# resolved by QEMU, that means that subsequent QMP or +# HMP commands must use node-names for the image in +# question, as filename lookup methods will fail. +# +# If not specified, QEMU will automatically determine +# the backing file string to use, or error out if +# there is no obvious choice. Care should be taken +# when specifying the string, to specify a valid +# filename or protocol. +# (Since 2.1) +# # If top == base, that is an error. # If top == active, the job will not be completed by itself, # user needs to complete the job with the block-job-complete
@@ -721,7 +738,6 @@ # size of the smaller top, you can safely truncate it # yourself once the commit operation successfully completes. # -# # @speed: #optional the maximum speed, in bytes per second # # Returns: Nothing on success
@@ -732,13 +748,16 @@ # If @speed is invalid, InvalidParameter # If both @base and @base-node-name are specified, GenericError # If both @top and @top-node-name are specified, GenericError +# If @top or @top-node-name is the active layer, and @backing-file is +# specified, GenericError # # Since: 1.3 # ## { 'command': 'block-commit', 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', - '*top': 'str', '*top-node-name': 'str', '*speed': 'int' } } + '*top': 'str', '*top-node-name': 'str', '*backing-file': 'str', + '*speed': 'int' } } ## # @drive-backup
qmp-commands.hx +18 -1
@@ -985,7 +985,7 @@ EQMP { .name = "block-commit", - .args_type = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,speed:o?", + .args_type = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,backing-file:s?,speed:o?", .mhandler.cmd_new = qmp_marshal_input_block_commit, },
@@ -1021,6 +1021,23 @@ neither is specified, this is the active layer topmost data to be committed down. (json-string, optional) (Since 2.1) +- backing-file: The backing file string to write into the overlay + image of 'top'. If 'top' is the active layer, + specifying a backing file string is an error. This + filename is not validated. + + If a pathname string is such that it cannot be + resolved by QEMU, that means that subsequent QMP or + HMP commands must use node-names for the image in + question, as filename lookup methods will fail. + + If not specified, QEMU will automatically determine + the backing file string to use, or error out if + there is no obvious choice. Care should be taken + when specifying the string, to specify a valid + filename or protocol. + (json-string, optional) (Since 2.1) + If top == base, that is an error. If top == active, the job will not be completed by itself, user needs to complete the job with the block-job-complete
Signature · 1 line
1.8.3.1
PATCH v5 7/10 block: add ability for block-stream to use node-name
In reply to Jeff CodyThis adds the ability for block-stream to use node-name arguments for base, to specify the backing image to stream from.
Both 'base' and 'base-node-name' are optional, but mutually exclusive. Either can be specified, but not both together.
--- blockdev.c | 48 +++++++++++++++++++++++++++++++++++++++--------- hmp.c | 2 +- qapi/block-core.json | 16 ++++++++++++---- qmp-commands.hx | 2 +- 4 files changed, 53 insertions(+), 15 deletions(-)
blockdev.c +39 -9
@@ -1878,38 +1878,68 @@ static void block_job_cb(void *opaque, int ret)bdrv_put_ref_bh_schedule (bs); } -void qmp_block_stream (const char *device,bool has_base, -const char *base,bool has_speed,int64_t speed, +void qmp_block_stream (const char *device, +bool has_base,const char *base, +bool has_base_node_name,const char *base_node_name, +bool has_speed,int64_t speed,bool has_on_error,BlockdevOnError on_error,Error **errp) { -BlockDriverState *bs; +BlockDriverState *bs =NULL ;BlockDriverState *base_bs =NULL ; +BlockDriverState *tmp_bs;Error *local_err =NULL ; +const char *base_name =NULL ;if (!has_on_error) { on_error =BLOCKDEV_ON_ERROR_REPORT ; } +if (has_base && has_base_node_name) { +error_setg (errp,"'base' and 'base-node-name' are mutually exclusive" ); +return ; + } + bs =bdrv_find (device);if (!bs) {error_set (errp,QERR_DEVICE_NOT_FOUND , device);return ; } +if (has_base_node_name) { + base_bs =bdrv_lookup_bs (NULL , base_node_name, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } + tmp_bs =bdrv_find_overlay (bs, base_bs); +if (tmp_bs) { + base_name = tmp_bs->backing_file; + } + } +if (bdrv_op_is_blocked (bs,BLOCK_OP_TYPE_STREAM , errp)) {return ; } -if (base) { +if (has_base) { base_bs =bdrv_find_backing_image (bs, base); -if (base_bs ==NULL ) { -error_set (errp,QERR_BASE_NOT_FOUND , base); -return ; - } + base_name = base; + } + +if (base_bs ==NULL && (has_base || has_base_node_name)) { +error_set (errp,QERR_BASE_NOT_FOUND , base); +return ; + } + +/* Verify that 'base' is in the same chain as 'bs', if 'base' was +* specified */ +if (base_bs && !bdrv_chain_contains (bs, base_bs)) { +error_setg (errp,"'device' and 'base' are not in the same chain" ); +return ; } -stream_start (bs, base_bs, base, has_speed ? speed :0 , +stream_start (bs, base_bs, base_name, has_speed ? speed :0 , on_error, block_job_cb, bs, &local_err);if (local_err) {error_propagate (errp, local_err);
hmp.c +1 -1
@@ -1173,7 +1173,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)int64_t speed =qdict_get_try_int (qdict,"speed" ,0 );qmp_block_stream (device, base !=NULL , base, -qdict_haskey (qdict,"speed" ), speed, + false,NULL ,qdict_haskey (qdict,"speed" ), speed, true,BLOCKDEV_ON_ERROR_REPORT , &error);hmp_handle_error (mon, &error);
qapi/block-core.json +12 -4
@@ -904,9 +904,17 @@ # On successful completion the image file is updated to drop the backing file # and the BLOCK_JOB_COMPLETED event is emitted. # -# @device: the device name +# @device: The device name. +# +# For 'base', either @base or @base-node-name may be set but not both. If +# neither is specified, the entire chain will be streamed into the active image, +# and the chain will consist of a single image (the current active layer) with +# no backing file. # -# @base: #optional the common backing file name +# @base: #optional the common backing file name +# +# @base-node-name: #optional the block driver state node name of the +# common backing file. (Since 2.1) # # @speed: #optional the maximum speed, in bytes per second #
@@ -920,8 +928,8 @@ # Since: 1.1 ## { 'command': 'block-stream', - 'data': { 'device': 'str', '*base': 'str', '*speed': 'int', - '*on-error': 'BlockdevOnError' } } + 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', + '*speed': 'int', '*on-error': 'BlockdevOnError' } } ## # @block-job-set-speed:
qmp-commands.hx +1 -1
@@ -979,7 +979,7 @@ EQMP { .name = "block-stream", - .args_type = "device:B,base:s?,speed:o?,on-error:s?", + .args_type = "device:B,base:s?,base-node-name:s?,speed:o?,on-error:s?", .mhandler.cmd_new = qmp_marshal_input_block_stream, },
Signature · 1 line
1.8.3.1
PATCH v5 8/10 block: add backing-file option to block-stream
In reply to Jeff CodyOn some image chains, QEMU may not always be able to resolve the filenames properly, when updating the backing file of an image after a block job.
For instance, certain relative pathnames may fail, or drives may have been specified originally by file descriptor (e.g. /dev/fd/???), or a relative protocol pathname may have been used.
In these instances, QEMU may lack the information to be able to make the correct choice, but the user or management layer most likely does have that knowledge.
With this extension to the block-stream api, the user is able to change the backing file of the active layer as part of the block-stream operation.
This allows the change to be 'safe', in the sense that if the attempt to write the active image metadata fails, then the block-stream operation returns failure, without disrupting the guest.
If a backing file string is not specified in the command, the backing file string to use is determined in the same manner as it was previously.
--- block/stream.c | 11 +++++------ blockdev.c | 12 ++++++++++++ hmp.c | 3 ++- qapi/block-core.json | 18 +++++++++++++++++- qmp-commands.hx | 2 +- 5 files changed, 37 insertions(+), 9 deletions(-)
block/stream.c +5 -6
@@ -32,7 +32,7 @@ typedef struct StreamBlockJob {RateLimit limit;BlockDriverState *base;BlockdevOnError on_error; -char backing_file_id[1024 ]; +char *backing_file_str; } StreamBlockJob; static int coroutine_fn stream_populate(BlockDriverState *bs,
@@ -186,7 +186,7 @@ wait:if (!block_job_is_cancelled (&s->common) && sector_num == end && ret ==0 ) {const char *base_id =NULL , *base_fmt =NULL ;if (base) { - base_id = s->backing_file_id; + base_id = s->backing_file_str;if (base->drv) { base_fmt = base->drv->format_name; }
@@ -196,6 +196,7 @@ wait: }qemu_vfree (buf); +g_free (s->backing_file_str);block_job_completed (&s->common, ret); }
@@ -217,7 +218,7 @@ static const BlockJobDriver stream_job_driver = { };void stream_start (BlockDriverState *bs,BlockDriverState *base, -const char *base_id,int64_t speed, +const char *backing_file_str,int64_t speed,BlockdevOnError on_error,BlockDriverCompletionFunc *cb,void *opaque,Error **errp)
@@ -237,9 +238,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base, } s->base = base; -if (base_id) { -pstrcpy (s->backing_file_id,sizeof (s->backing_file_id), base_id); - } + s->backing_file_str =g_strdup (backing_file_str); s->on_error = on_error; s->common.co =qemu_coroutine_create (stream_run);
blockdev.c +12 -0
@@ -1881,6 +1881,7 @@ static void block_job_cb(void *opaque, int ret)void qmp_block_stream (const char *device,bool has_base,const char *base,bool has_base_node_name,const char *base_node_name, +bool has_backing_file,const char *backing_file,bool has_speed,int64_t speed,bool has_on_error,BlockdevOnError on_error,Error **errp)
@@ -1932,6 +1933,9 @@ void qmp_block_stream(const char *device,return ; } +/* backing_file string overrides base bs filename */ + base_name = has_backing_file ? backing_file : base_name; +/* Verify that 'base' is in the same chain as 'bs', if 'base' was * specified */ if (base_bs && !bdrv_chain_contains (bs, base_bs)) {
@@ -1939,6 +1943,14 @@ void qmp_block_stream(const char *device,return ; } +/* if we are streaming the entire chain, the result will have no backing +* file, and specifying one is therefore an error */ +if (base_bs ==NULL && has_backing_file) { +error_setg (errp,"backing file specified, but streaming the " +"entire chain" ); +return ; + } +stream_start (bs, base_bs, base_name, has_speed ? speed :0 , on_error, block_job_cb, bs, &local_err);if (local_err) {
hmp.c +2 -1
@@ -1173,7 +1173,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)int64_t speed =qdict_get_try_int (qdict,"speed" ,0 );qmp_block_stream (device, base !=NULL , base, - false,NULL ,qdict_haskey (qdict,"speed" ), speed, + false,NULL , false,NULL , +qdict_haskey (qdict,"speed" ), speed, true,BLOCKDEV_ON_ERROR_REPORT , &error);hmp_handle_error (mon, &error);
qapi/block-core.json +17 -1
@@ -916,6 +916,21 @@ # @base-node-name: #optional the block driver state node name of the # common backing file. (Since 2.1) # +# @backing-file: #optional The backing file string to write into the active +# layer. This filename is not validated. +# +# If a pathname string is such that it cannot be +# resolved by QEMU, that means that subsequent QMP or +# HMP commands must use node-names for the image in +# question, as filename lookup methods will fail. +# +# If not specified, QEMU will automatically determine +# the backing file string to use, or error out if there +# is no obvious choice. Care should be taken when +# specifying the string, to specify a valid filename or +# protocol. +# (Since 2.1) +# # @speed: #optional the maximum speed, in bytes per second # # @on-error: #optional the action to take on an error (default report).
@@ -929,7 +944,8 @@ ## { 'command': 'block-stream', 'data': { 'device': 'str', '*base': 'str', '*base-node-name': 'str', - '*speed': 'int', '*on-error': 'BlockdevOnError' } } + '*backing-file': 'str', '*speed': 'int', + '*on-error': 'BlockdevOnError' } } ## # @block-job-set-speed:
qmp-commands.hx +1 -1
@@ -979,7 +979,7 @@ EQMP { .name = "block-stream", - .args_type = "device:B,base:s?,base-node-name:s?,speed:o?,on-error:s?", + .args_type = "device:B,base:s?,base-node-name:s?,speed:o?,backing-file:s?,on-error:s?", .mhandler.cmd_new = qmp_marshal_input_block_stream, },
Signature · 1 line
1.8.3.1
PATCH v5 9/10 block: Add QMP documentation for block-stream
In reply to Jeff CodyThe QMP command 'block-stream' was missing QMP documentation. Add that documentation.
--- qmp-commands.hx | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
qmp-commands.hx +58 -0
@@ -983,6 +983,64 @@ EQMP .mhandler.cmd_new = qmp_marshal_input_block_stream, }, +SQMP +block-stream +------------ + +Copy data from a backing file into a block device. + +The block streaming operation is performed in the background until the entire +backing file has been copied. This command returns immediately once streaming +has started. The status of ongoing block streaming operations can be checked +with query-block-jobs. The operation can be stopped before it has completed +using the block-job-cancel command. + +If a base file is specified then sectors are not copied from that base file and +its backing chain. When streaming completes the image file will have the base +file as its backing file. This can be used to stream a subset of the backing +file chain instead of flattening the entire image. + +On successful completion the image file is updated to drop the backing file +and the BLOCK_JOB_COMPLETED event is emitted. + +- "device": The device name. + (json-string) + +For base, either 'base' or 'base-node-name' may be set but not both. If +neither is specified, the entire chain will be streamed into the active image, +and the chain will consist of a single image (the current active layer) with +no backing file. + +- "base": The common backing file name. + (json-string, optional) + +- "base-node-name": The block driver state node name of the common backing file. + (json-string, optional) (Since 2.1) + +- "backing-file": The backing file string to write into the active layer. + This filename is not validated. + + If a pathname string is such that it cannot be resolved by + QEMU, that means that subsequent QMP or HMP commands must + use node-names for the image in question, as filename + lookup methods will fail. + + If not specified, QEMU will automatically determine the + backing file string to use, or error out if there is no + obvious choice. Care should be taken when specifying the + string, to specify a valid filename or protocol. + (json-string, optional) + (Since 2.1) + +- "speed": The maximum speed, in bytes per second. + (json-int, optional) + +- "on-error": The action to take on an error (default report). + 'stop' and 'enospc' can only be used if the block device + supports io-status (see BlockInfo). + (json-enum, optional) (Since 1.3) +EQMP + { .name = "block-commit", .args_type = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,backing-file:s?,speed:o?",
Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH v5 09/10] block: Add QMP documentation for block-stream
Quoted 82 lines · Jeff Cody
The QMP command 'block-stream' was missing QMP documentation. Add that documentation.
Reviewed-by Eric Blake <[email protected]>Signed-off-by Jeff Cody <[email protected]>--- qmp-commands.hx | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)qmp-commands.hx +58
-0@@ -983,6 +983,64 @@ EQMP .mhandler.cmd_new = qmp_marshal_input_block_stream, }, +SQMP +block-stream +------------ + +Copy data from a backing file into a block device. + +The block streaming operation is performed in the background until the entire +backing file has been copied. This command returns immediately once streaming +has started. The status of ongoing block streaming operations can be checked +with query-block-jobs. The operation can be stopped before it has completed +using the block-job-cancel command. + +If a base file is specified then sectors are not copied from that base file and +its backing chain. When streaming completes the image file will have the base +file as its backing file. This can be used to stream a subset of the backing +file chain instead of flattening the entire image. + +On successful completion the image file is updated to drop the backing file +and the BLOCK_JOB_COMPLETED event is emitted. + +- "device": The device name. + (json-string) + +For base, either 'base' or 'base-node-name' may be set but not both. If +neither is specified, the entire chain will be streamed into the active image, +and the chain will consist of a single image (the current active layer) with +no backing file. + +- "base": The common backing file name. + (json-string, optional) + +- "base-node-name": The block driver state node name of the common backing file. + (json-string, optional) (Since 2.1) + +- "backing-file": The backing file string to write into the active layer. + This filename is not validated. + + If a pathname string is such that it cannot be resolved by + QEMU, that means that subsequent QMP or HMP commands must + use node-names for the image in question, as filename + lookup methods will fail. + + If not specified, QEMU will automatically determine the + backing file string to use, or error out if there is no + obvious choice. Care should be taken when specifying the + string, to specify a valid filename or protocol. + (json-string, optional) + (Since 2.1) + +- "speed": The maximum speed, in bytes per second. + (json-int, optional) + +- "on-error": The action to take on an error (default report). + 'stop' and 'enospc' can only be used if the block device + supports io-status (see BlockInfo). + (json-enum, optional) (Since 1.3) +EQMP + { .name = "block-commit", .args_type = "device:B,base:s?,base-node-name:s?,top:s?,top-node-name:s?,backing-file:s?,speed:o?",Signature · 1 line
1.8.3.1
PATCH v5 10/10 block: add QAPI command to allow live backing file change
In reply to Jeff CodyThis allows a user to make a live change to the backing file recorded in an open image.
The image file to modify can be specified 2 ways:
1) image filename
2) image node-name
Note: this does not cause the backing file itself to be reopened; it merely changes the backing filename in the image file structure, and in internal BDS structures.
It is the responsibility of the user to pass a filename string that can be resolved when the image chain is reopened, and the filename string is not validated.
A good analogy for this command is that it is a live version of 'qemu-img rebase -u', with respect to changing the backing file string.
--- blockdev.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 60 ++++++++++++++++++++++++++++++ qmp-commands.hx | 74 +++++++++++++++++++++++++++++++++++++ 3 files changed, 236 insertions(+)
blockdev.c +102 -0
@@ -2414,6 +2414,108 @@ void qmp_block_job_complete(const char *device, Error **errp)block_job_complete (job, errp); } +void qmp_change_backing_file (const char *device, +bool has_image,const char *image, +bool has_image_node_name, +const char *image_node_name, +const char *backing_file, +Error **errp) +{ +BlockDriverState *bs =NULL ; +BlockDriverState *image_bs =NULL ; +Error *local_err =NULL ; +bool ro; +int open_flags; +int ret; + +/* validate argument combinations */ +if (has_image && has_image_node_name) { +error_setg (errp,"'image' and 'image-node-name' " +"are mutually exclusive" ); +return ; + } + +/* find the top layer BDS of the chain */ + bs =bdrv_find (device); +if (!bs) { +error_set (errp,QERR_DEVICE_NOT_FOUND , device); +return ; + } + +if (has_image_node_name) { + image_bs =bdrv_lookup_bs (NULL , image_node_name, &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } + } + +if (has_image) { +if (!strcmp (bs->filename, image)) { + image_bs = bs; + }else { + image_bs =bdrv_find_backing_image (bs, image); + } + } + +if (!has_image && !has_image_node_name) { + image_bs = bs; + } + +if (!image_bs) { +error_setg (errp,"image file not found" ); +return ; + } + +if (bdrv_find_base (image_bs) == image_bs) { +error_setg (errp,"not allowing backing file change on an image " +"without a backing file" ); +return ; + } + +/* even though we are not necessarily operating on bs, we need it to +* determine if block ops are currently prohibited on the chain */ +if (bdrv_op_is_blocked (bs,BLOCK_OP_TYPE_CHANGE , errp)) { +return ; + } + +/* final sanity check */ +if (!bdrv_chain_contains (bs, image_bs)) { +error_setg (errp,"'%s' and image file are not in the same chain" , + device); +return ; + } + +/* if not r/w, reopen to make r/w */ + open_flags = image_bs->open_flags; + ro =bdrv_is_read_only (image_bs); + +if (ro) { +bdrv_reopen (image_bs, open_flags |BDRV_O_RDWR , &local_err); +if (local_err) { +error_propagate (errp, local_err); +return ; + } + } + + ret =bdrv_change_backing_file (image_bs, backing_file, + image_bs->drv ? image_bs->drv->format_name :"" ); + +if (ret <0 ) { +error_setg_errno (errp, -ret,"Could not change backing file to '%s'" , + backing_file); +/* don't exit here, so we can try to restore open flags if +* appropriate */ + } + +if (ro) { +bdrv_reopen (image_bs, open_flags, &local_err); +if (local_err) { +error_propagate (errp, local_err);/* will preserve prior errp */ + } + } +} +void qmp_blockdev_add (BlockdevOptions *options,Error **errp) {QmpOutputVisitor *ov =qmp_output_visitor_new ();
qapi/block-core.json +60 -0
66 lines not on this page · raw message
qmp-commands.hx +74 -0
80 lines not on this page · raw message
Signature · 1 line
1.8.3.1
Quoted 7 lines · Jeff Cody
Changes from
v4->v5:* Rebased on master
* Fixed commit log typos / stale paragraphs (Eric)
* Fixed comment typo (Eric)
* Added Eric's remaining R-b's
On IRC, we found another bug that needs repairing:
# cd /tmp
# echo tmp > tmp
# qemu-kvm -qmp stdio -drive file=/tmp/tmp
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 2},
"package": ""}, "capabilities": []}}
{"execute":"qmp_capabilities"}
{"return": {}}
{"execute":"block-commit","arguments":{"device":"ide0-hd0"}}
{"return": {}}
{"timestamp": {"seconds": 1402972048, "microseconds": 153175}, "event":
"BLOCK_JOB_READY", "data": {"device": "ide0-hd0", "len": 512, "offset":
512, "speed": 0, "type": "commit"}}
{"execute":"block-job-complete","arguments":{"device":"ide0-hd0"}}
{"return": {}}
qemu-system-x86_64: block.c:2034: bdrv_swap: Assertion
`bs_new->device_name[0] == '\0'' failed.
Aborted (core dumped)Attempts to commit-to-self should be rejected at block-commit time, rather than causing an assertion failure down the road.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 29 lines · Eric Blake
Quoted 7 lines · Jeff Cody
Changes from
v4->v5:* Rebased on master
* Fixed commit log typos / stale paragraphs (Eric)
* Fixed comment typo (Eric)
* Added Eric's remaining R-b'sOn IRC, we found another bug that needs repairing:
# cd /tmp # echo tmp > tmp # qemu-kvm -qmp stdio -drive file=/tmp/tmp {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 2}, "package": ""}, "capabilities": []}} {"execute":"qmp_capabilities"} {"return": {}} {"execute":"block-commit","arguments":{"device":"ide0-hd0"}} {"return": {}} {"timestamp": {"seconds": 1402972048, "microseconds": 153175}, "event": "BLOCK_JOB_READY", "data": {"device": "ide0-hd0", "len": 512, "offset": 512, "speed": 0, "type": "commit"}} {"execute":"block-job-complete","arguments":{"device":"ide0-hd0"}} {"return": {}} qemu-system-x86_64: block.c:2034: bdrv_swap: Assertion `bs_new->device_name[0] == '\0'' failed. Aborted (core dumped)
Quoted 3 lines
Attempts to commit-to-self should be rejected at block-commit time, rather than causing an assertion failure down the road.
Thanks... spinning a v6 to prevent this scenario.
Quoted 29 lines · Eric Blake
Quoted 7 lines · Jeff Cody
Changes from
v4->v5:* Rebased on master
* Fixed commit log typos / stale paragraphs (Eric)
* Fixed comment typo (Eric)
* Added Eric's remaining R-b'sOn IRC, we found another bug that needs repairing:
# cd /tmp # echo tmp > tmp # qemu-kvm -qmp stdio -drive file=/tmp/tmp {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 2}, "package": ""}, "capabilities": []}} {"execute":"qmp_capabilities"} {"return": {}} {"execute":"block-commit","arguments":{"device":"ide0-hd0"}} {"return": {}} {"timestamp": {"seconds": 1402972048, "microseconds": 153175}, "event": "BLOCK_JOB_READY", "data": {"device": "ide0-hd0", "len": 512, "offset": 512, "speed": 0, "type": "commit"}} {"execute":"block-job-complete","arguments":{"device":"ide0-hd0"}} {"return": {}} qemu-system-x86_64: block.c:2034: bdrv_swap: Assertion `bs_new->device_name[0] == '\0'' failed. Aborted (core dumped)
Quoted 3 lines
Attempts to commit-to-self should be rejected at block-commit time, rather than causing an assertion failure down the road.
I put the changes to fix this in patch 4/10 in the v6 (along with the additional comment). I mean to drop your R-b in the patch so you could review it again, but I forgot before sending it (sorry) - but if you want to review it, it is all contained in patch 4.
Jeff