block: Modify block-commit to use node-names
cover block: Modify block-commit to use node-names
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 is the conversion of block-commit to allow use of node-names. Also, it allows the user to specify the string for the backing_file name to use in the overlay image.
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 node-names.
Subsequent patches will convert the remaining block operations (stream, backup, mirror)
These patches can also be seen at:
https://github.com/codyprime/qemu-kvm-jtc.git, tag block-commit-node-v1a
Jeff Cody (5):
block: Auto-generate node_names for each BDS entry
block: add helper function to determine if a BDS is in a chain
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.c | 33 ++++++++++++++++++++++++++++--- block/commit.c | 9 ++++++--- blockdev.c | 46 +++++++++++++++++++++++++++++++++++++++---- include/block/block.h | 4 +++- include/block/block_int.h | 3 ++- qapi-schema.json | 50 ++++++++++++++++++++++++++++++++++++++--------- qmp-commands.hx | 40 +++++++++++++++++++++++++++++++------ tests/qemu-iotests/040 | 28 ++++++++++++++++---------- 8 files changed, 176 insertions(+), 37 deletions(-)
Signature · 1 line
1.8.3.1
PATCH 1/5 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 filename pathing (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
@@ -838,12 +838,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 1/5] block: Auto-generate node_names for each BDS entry
Quoted 36 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 filename pathing (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.
Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)block.c +0
-0
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 847
Quoted 10 lines
block.c +3
-0@@ -838,12 +838,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 ];
The room for the '\0' string termination seems to be missing:
char gen_node_name[GEN_NODE_NAME_MAX_LEN + 1];
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 858
Quoted 12 lines
block.c +10
-1+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' ); + }
Is this code generating only 7 random chars instead of 8 ?
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 859
Quoted 1 line
block.c +1
-0+ gen_node_name[GEN_NODE_NAME_MAX_LEN -1 ] ='\0' ;
Could be:
gen_node_name[GEN_NODE_NAME_MAX_LEN] = '\0';
if the array is properly declared.
Quoted 7 lines
block.c +1
-0+ node_name = gen_node_name; }/* empty string node name is invalid */ Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
Quoted 50 lines · Benoît Canet
Quoted 36 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 filename pathing (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.
Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)block.c +0
-0block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 847
Quoted 10 lines
block.c +3
-0@@ -838,12 +838,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 ];The room for the '\0' string termination seems to be missing:
Quoted 2 lines
char gen_node_name[GEN_NODE_NAME_MAX_LEN + 1];
The array includes room for it, note the use of 'sizeof()':
#define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
sizeof() includes the '\0' in the length, while strlen() does not; e.g.:
sizeof("four") = 5
strlen("four") = 4Quoted 13 lines
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 858
Quoted 12 lines
block.c +10
-1+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' ); + }
Quoted 2 lines
Is this code generating only 7 random chars instead of 8 ?
It generates 8 random characters (the sample node-name strings in the commit message were pulled straight from the QMP command 'query-named-block-nodes')
Quoted 6 lines
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 859
Quoted 1 line
block.c +1
-0+ gen_node_name[GEN_NODE_NAME_MAX_LEN -1 ] ='\0' ;Could be:
gen_node_name[GEN_NODE_NAME_MAX_LEN] = '\0';if the array is properly declared.
That would go over the array bounds by 1.
Quoted 7 lines
Quoted 7 lines
block.c +1
-0+ node_name = gen_node_name; }/* empty string node name is invalid */ Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
Quoted 82 lines · Jeff Cody
Quoted 50 lines · Benoît Canet
Quoted 36 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 filename pathing (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.
Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)block.c +0
-0block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 847
Quoted 10 lines
block.c +3
-0@@ -838,12 +838,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 ];The room for the '\0' string termination seems to be missing:
Quoted 2 lines
char gen_node_name[GEN_NODE_NAME_MAX_LEN + 1];The array includes room for it, note the use of '
sizeof()':#define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
sizeof()includes the '\0' in the length, whilestrlen()does not; e.g.:sizeof("four") = 5 strlen("four") = 4Quoted 13 lines
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 858
Quoted 12 lines
block.c +10
-1+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' ); + }Quoted 2 lines
Is this code generating only 7 random chars instead of 8 ?
It generates 8 random characters (the sample node-name strings in the commit message were pulled straight from the QMP command 'query-named-block-nodes')
Quoted 8 lines
Quoted 6 lines
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 859
Quoted 1 line
block.c +1
-0+ gen_node_name[GEN_NODE_NAME_MAX_LEN -1 ] ='\0' ;Could be:
gen_node_name[GEN_NODE_NAME_MAX_LEN] = '\0';if the array is properly declared.
That would go over the array bounds by 1.
Yes I missed the use of sizeof().
I am happy to have learnt that.
Sorry for the noise.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
block.c +1
-0+ node_name = gen_node_name; }/* empty string node name is invalid */ Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
Quoted 90 lines · Benoît Canet
Quoted 89 lines · Jeff Cody
Quoted 50 lines · Benoît Canet
Quoted 36 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 filename pathing (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.
Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)block.c +0
-0block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 847
Quoted 10 lines
block.c +3
-0@@ -838,12 +838,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 ];The room for the '\0' string termination seems to be missing:
Quoted 2 lines
char gen_node_name[GEN_NODE_NAME_MAX_LEN + 1];The array includes room for it, note the use of '
sizeof()':#define GEN_NODE_NAME_MAX_LEN (sizeof(GEN_NODE_NAME_PREFIX) + 8 + 8)
sizeof()includes the '\0' in the length, whilestrlen()does not; e.g.:sizeof("four") = 5 strlen("four") = 4Quoted 13 lines
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 858
Quoted 12 lines
block.c +10
-1+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' ); + }Quoted 2 lines
Is this code generating only 7 random chars instead of 8 ?
It generates 8 random characters (the sample node-name strings in the commit message were pulled straight from the QMP command 'query-named-block-nodes')
Quoted 6 lines
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 859
Quoted 1 line
block.c +1
-0+ gen_node_name[GEN_NODE_NAME_MAX_LEN -1 ] ='\0' ;Could be:
gen_node_name[GEN_NODE_NAME_MAX_LEN] = '\0';if the array is properly declared.
Quoted 6 lines
Quoted 1 line
That would go over the array bounds by 1.
Yes I missed the use of
sizeof().
I am happy to have learnt that.
Sorry for the noise.
It wasn't noise :) Thanks for the reviews.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
block.c +1
-0+ node_name = gen_node_name; }/* empty string node name is invalid */ Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
In reply to Jeff CodyQuoted 7 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 filename pathing
"pathing" sounds weird; a non-native speaker may get confused since it is not a real word. Maybe:
This eliminates ambiguity in resolving filenames
Quoted 26 lines
(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.
Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-){
block.c static int bdrv_open_flags(BlockDriverState *bs, int flags) · line 855
Quoted 10 lines
block.c +8
-1+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);
Why not just len = snprintf(), and avoid the strlen()?
But that micro-optimization is not on the hot path, so:
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
In reply to Jeff CodyQuoted 26 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 filename pathing (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.
Quoted 4 lines
Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
This patch feels incomplete. We probably also ought to modify qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by unconditional (as an output-only struct, changing from optional to mandatory is a safe change for back-compat API considerations); which implies further modifying code to get rid of has_node_name arguments in all places that generate BlockDeviceInfo details.
See also my thoughts on patch 5/5 - can this patch be rebased to be LAST in the series, rather than first, so that it serves as a reliable witness that everything else related to block operations on node-names is complete?
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
Quoted 32 lines · Eric Blake
Quoted 26 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 filename pathing (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.
Quoted 4 lines
Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
Quoted 6 lines
This patch feels incomplete. We probably also ought to modify qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by unconditional (as an output-only struct, changing from optional to mandatory is a safe change for back-compat API considerations); which implies further modifying code to get rid of
has_node_namearguments in all places that generate BlockDeviceInfo details.
I think it should be a separate patch (but still in this series). Strictly speaking, this patch doesn't force the argument to be mandatory, the value just happens to always be populated now.
Quoted 5 lines
See also my thoughts on patch 5/5 - can this patch be rebased to be LAST in the series, rather than first, so that it serves as a reliable witness that everything else related to block operations on node-names is complete?
How about this becomes the penultimate patch, and the patch to make BlockDeviceInfo's node-name field to be unconditional becomes the last patch?
The only thing I don't like about moving this further back in the patch series is it makes the earlier patches untestable; I can't easily test the usage of the node-names for various intermediate BDS because they don't have node-names set. So that means I'll just need to rebase the patches prior to sending. So let me revise my above statement - how about this patch stays at the beginning, which makes development / testing easier, with the patch that modifies BlockDeviceInfo as the final (not including tests) patch?
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
Quoted 17 lines · Jeff Cody
Quoted 10 lines
Quoted 9 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.
Signed-off-by Jeff Cody <[email protected]>--- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)Quoted 6 lines
This patch feels incomplete. We probably also ought to modify qapi-schema.json to make BlockDeviceInfo's output of 'node-name' by unconditional (as an output-only struct, changing from optional to mandatory is a safe change for back-compat API considerations); which implies further modifying code to get rid of
has_node_namearguments in all places that generate BlockDeviceInfo details.
Quoted 3 lines
I think it should be a separate patch (but still in this series). Strictly speaking, this patch doesn't force the argument to be mandatory, the value just happens to always be populated now.
True, keeping it as two patches is cleaner.
Quoted 9 lines
Quoted 6 lines
See also my thoughts on patch 5/5 - can this patch be rebased to be LAST in the series, rather than first, so that it serves as a reliable witness that everything else related to block operations on node-names is complete?
How about this becomes the penultimate patch, and the patch to make BlockDeviceInfo's node-name field to be unconditional becomes the last patch?
Or, if we go the route of adding a new command for setting the backing name in isolation, then _that_ patch should be last, at which point leaving this patch early in the series no longer hurts, because it is no longer the witness that libvirt would be relying on.
Quoted 9 lines
The only thing I don't like about moving this further back in the patch series is it makes the earlier patches untestable; I can't easily test the usage of the node-names for various intermediate BDS because they don't have node-names set. So that means I'll just need to rebase the patches prior to sending. So let me revise my above statement - how about this patch stays at the beginning, which makes development / testing easier, with the patch that modifies BlockDeviceInfo as the final (not including tests) patch?
Yes, keeping this patch first sounds reasonable after all. The patch modifying BlockDeviceInfo isn't introspectible (there's nothing libvirt can probe that says whether the QMP code switched from optional to mandatory - all libvirt can probe is whether a name gets generated - and that is THIS patch, not the BlockDeviceInfo patch). But adding a new command is easier to use than probing whether a name was generated.
Quoted 2 lines
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
In reply to Jeff CodyAm 15.05.2014 um 20:41 hat Jeff Cody geschrieben:
Quoted 5 lines
The only thing I don't like about moving this further back in the patch series is it makes the earlier patches untestable; I can't easily test the usage of the node-names for various intermediate BDS because they don't have node-names set. So that means I'll just need to rebase the patches prior to sending.
I don't quite follow. Can't you always manually assign node-names? This is how libvirt is supposed to use the interface.
I'm not totally sure whether automatically generated node-names are a good idea, but I can see how they are useful with human monitor users which may not specify a node-name everywhere (I've used device_add without an ID often enough, only to find that I can't remove the device any more). We should just make sure that they are really only used by human users.
Kevin
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
Quoted 10 lines · Kevin Wolf
Am 15.05.2014 um 20:41 hat Jeff Cody geschrieben:
Quoted 5 lines
The only thing I don't like about moving this further back in the patch series is it makes the earlier patches untestable; I can't easily test the usage of the node-names for various intermediate BDS because they don't have node-names set. So that means I'll just need to rebase the patches prior to sending.
I don't quite follow. Can't you always manually assign node-names? This is how libvirt is supposed to use the interface.
How does libvirt assign node-names to all the backing images in a qcow2 chain, for example?
Quoted 7 lines
I'm not totally sure whether automatically generated node-names are a good idea, but I can see how they are useful with human monitor users which may not specify a node-name everywhere (I've used
device_addwithout an ID often enough, only to find that I can't remove the device any more). We should just make sure that they are really only used by human users.
I don't understand. What would be the downsides of having an automatic guaranteed unique id assigned to each BDS? And why restrict that to human users only?
If you are worried about node-names potentially being undesired by the user / management layer for some reason, how about this: we add a drive option, to either enable or disable automatic node-name generation for a particular drive?
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
Quoted 3 lines · Jeff Cody
How does libvirt assign node-names to all the backing images in a qcow2 chain, for example?
We have the ability to do both command-line and QMP hotplug addition of drives where you can set backing.file.node-name (or is it backing.node-name?) for any element in a backing chain; libvirt just needs to be taught to use it.
Quoted 10 lines
Quoted 9 lines
I'm not totally sure whether automatically generated node-names are a good idea, but I can see how they are useful with human monitor users which may not specify a node-name everywhere (I've used
device_addwithout an ID often enough, only to find that I can't remove the device any more). We should just make sure that they are really only used by human users.
Quoted 3 lines
I don't understand. What would be the downsides of having an automatic guaranteed unique id assigned to each BDS? And why restrict that to human users only?
I'm not seeing downsides.
Quoted 5 lines
If you are worried about node-names potentially being undesired by the user / management layer for some reason, how about this: we add a drive option, to either enable or disable automatic node-name generation for a particular drive?
No, this is silly. If it defaults to off, then libvirt has to be taught to enable it - but if you are going to modify libvirt, you might as well modify it to set node names itself rather than to enable the option for automatic node names. If it defaults to on, no one has an incentive to disable it.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 1/5] block: Auto-generate node_names for each BDS entry
Am 16.05.2014 um 14:47 hat Eric Blake geschrieben:
Quoted 10 lines · Jeff Cody
Quoted 4 lines
How does libvirt assign node-names to all the backing images in a qcow2 chain, for example?
We have the ability to do both command-line and QMP hotplug addition of drives where you can set backing.file.node-name (or is it backing.node-name?) for any element in a backing chain; libvirt just needs to be taught to use it.
backing.node-name and backing.file.node-name usually both exist, and they are different nodes (one is the qcow2 layer, the other on the raw-posix layer).
Kevin
PATCH 2/5 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 | 9 +++++++++ include/block/block.h | 1 + 2 files changed, 10 insertions(+)
block.c +9 -0
@@ -3734,6 +3734,15 @@ BlockDriverState *bdrv_lookup_bs(const char *device,return NULL ; } +bool bdrv_is_in_chain (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
@@ -381,6 +381,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void);BlockDriverState *bdrv_lookup_bs (const char *device,const char *node_name,Error **errp); +bool bdrv_is_in_chain (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
Re: [Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain
Quoted 47 lines · Jeff Cody
This 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.
Signed-off-by Jeff Cody <[email protected]>--- block.c | 9 +++++++++ include/block/block.h | 1 + 2 files changed, 10 insertions(+)block.c +9
-0@@ -3734,6 +3734,15 @@ BlockDriverState *bdrv_lookup_bs(const char *device,return NULL ; } +bool bdrv_is_in_chain (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@@ -381,6 +381,7 @@ BlockDeviceInfoList *bdrv_named_nodes_list(void);BlockDriverState *bdrv_lookup_bs (const char *device,const char *node_name,Error **errp); +bool bdrv_is_in_chain (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
Re: [Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain
In reply to Jeff CodyQuoted 7 lines · Jeff Cody
This 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.
Signed-off-by Jeff Cody <[email protected]>
Quoted 8 lines
--- block.c | 9 +++++++++ include/block/block.h | 1 + 2 files changed, 10 insertions(+)+bool
bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)
No doc comments inline, and not everyone has the commit message handy. Which means someone trying to learn what this command does has to read the function. Maybe copy the commit message into code comments as well.
Bikeshedding: If I'm reading code, and see bdrv_is_in_chain(a, b), my first inclination would be to read that as "return true if node a is in chain b". But if I were to see bdrv_chain_contains(a, b), I would parse that as "return true if chain a contains node b". I think you either want to swap argument order, or rename the function.
The function itself looks useful, though, once we agree on the naming.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain
Am 15.05.2014 um 16:16 hat Eric Blake geschrieben:
Quoted 21 lines · Jeff Cody
Quoted 7 lines
This 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.
Signed-off-by Jeff Cody <[email protected]>Quoted 8 lines
--- block.c | 9 +++++++++ include/block/block.h | 1 + 2 files changed, 10 insertions(+)+bool
bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)No doc comments inline, and not everyone has the commit message handy. Which means someone trying to learn what this command does has to read the function. Maybe copy the commit message into code comments as well.
Quoted 5 lines
Bikeshedding: If I'm reading code, and see
bdrv_is_in_chain(a, b), my first inclination would be to read that as "return true if node a is in chain b". But if I were to seebdrv_chain_contains(a, b), I would parse that as "return true if chain a contains node b". I think you either want to swap argument order, or rename the function.
I haven't read the patch yet, so I can't say much about the context, but just from seeing the function names I think I agree with you.
Kevin
Re: [Qemu-devel] [PATCH 2/5] block: add helper function to determine if a BDS is in a chain
In reply to Eric BlakeQuoted 15 lines · Eric Blake
Quoted 14 lines · Jeff Cody
This 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.
Signed-off-by Jeff Cody <[email protected]>--- block.c | 9 +++++++++ include/block/block.h | 1 + 2 files changed, 10 insertions(+)
Quoted 6 lines
Quoted 1 line
+bool
bdrv_is_in_chain(BlockDriverState *top, BlockDriverState *base)No doc comments inline, and not everyone has the commit message handy. Which means someone trying to learn what this command does has to read the function. Maybe copy the commit message into code comments as well.
OK
Quoted 5 lines
Bikeshedding: If I'm reading code, and see
bdrv_is_in_chain(a, b), my first inclination would be to read that as "return true if node a is in chain b". But if I were to seebdrv_chain_contains(a, b), I would parse that as "return true if chain a contains node b". I think you either want to swap argument order, or rename the function.
I like the bdrv_chain_contains() as the function name, it is clearer. I'll use that.
Quoted 3 lines
The function itself looks useful, though, once we agree on the naming.
PATCH 3/5 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 optional, with the default being the active layer in the device chain.
--- blockdev.c | 3 ++- qapi-schema.json | 7 ++++--- qmp-commands.hx | 5 +++-- tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- 4 files changed, 27 insertions(+), 16 deletions(-)
blockdev.c +2 -1
@@ -1868,7 +1868,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) {
qapi-schema.json +4 -3
@@ -2099,8 +2099,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,
@@ -2128,7 +2129,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
Re: [Qemu-devel] [PATCH 3/5] block: make 'top' argument to block-commit optional
Quoted 4 lines · Jeff Cody
Now that active layer block-commit is supported, the 'top' argument no longer needs to be mandatory.
Change it optional, with the default being the active layer in the
Do you mean "Change it to optional" or "Make it optional" ?
Quoted 134 lines
device chain.
Signed-off-by Jeff Cody <[email protected]>--- blockdev.c | 3 ++- qapi-schema.json | 7 ++++--- qmp-commands.hx | 5 +++-- tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- 4 files changed, 27 insertions(+), 16 deletions(-)blockdev.c +2
-1@@ -1868,7 +1868,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) {qapi-schema.json +4
-3@@ -2099,8 +2099,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,@@ -2128,7 +2129,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 * 1024Signature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 3/5] block: make 'top' argument to block-commit optional
Quoted 8 lines · Benoît Canet
Quoted 4 lines · Jeff Cody
Now that active layer block-commit is supported, the 'top' argument no longer needs to be mandatory.
Change it optional, with the default being the active layer in the
Do you mean "Change it to optional" or "Make it optional" ?
Thanks - forgot the "to".
Quoted 135 lines
Quoted 134 lines
device chain.
Signed-off-by Jeff Cody <[email protected]>--- blockdev.c | 3 ++- qapi-schema.json | 7 ++++--- qmp-commands.hx | 5 +++-- tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- 4 files changed, 27 insertions(+), 16 deletions(-)blockdev.c +2
-1@@ -1868,7 +1868,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) {qapi-schema.json +4
-3@@ -2099,8 +2099,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,@@ -2128,7 +2129,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 * 1024Signature · 1 line
1.8.3.1Reviewed-by Benoit Canet <[email protected]>
Re: [Qemu-devel] [PATCH 3/5] block: make 'top' argument to block-commit optional
In reply to Jeff CodyQuoted 5 lines · Jeff Cody
Now that active layer block-commit is supported, the 'top' argument no longer needs to be mandatory.
Change it optional, with the default being the active layer in the device chain.
Good, this is an API-compatible change (doesn't break old clients that always provide 'top'). I'm definitely in favor of it.
Quoted 12 lines
Signed-off-by Jeff Cody <[email protected]>--- blockdev.c | 3 ++- qapi-schema.json | 7 ++++--- qmp-commands.hx | 5 +++-- tests/qemu-iotests/040 | 28 ++++++++++++++++++---------- 4 files changed, 27 insertions(+), 16 deletions(-)+++
b/qapi-schema.json
qapi-schema.json line 2104
Quoted 8 lines
qapi-schema.json +3
-2@@ -2099,8 +2099,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.
We have not done the conversion from mandatory to optional very often; I wonder if it is worth an annotation of when the conversion happened (so someone reading just this version of the schema but planning on targeting older qemu knows to always supply the argument). Something like:
@top: since 1.3, #optional since 2.1; The file name...
But that feels rather long. It's not a show-stopper to me, particularly since we still don't have any automated conversion from these comments into any other documentation formats. That is, if we decide it is worth adding hints about when something changed from mandatory to optional, it would be its own patch and cover the work we've done on other recent changes like block_passwd.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH 4/5 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 | 35 +++++++++++++++++++++++++++++++++-- qapi-schema.json | 35 ++++++++++++++++++++++++++--------- qmp-commands.hx | 29 ++++++++++++++++++++++------- 3 files changed, 81 insertions(+), 18 deletions(-)
blockdev.c +33 -2
@@ -1869,7 +1869,9 @@ 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) {
@@ -1888,6 +1890,15 @@ void qmp_block_commit(const char *device,/* drain all i/o before commits */ bdrv_drain_all (); +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 ; + } + bs =bdrv_find (device);if (!bs) {error_set (errp,QERR_DEVICE_NOT_FOUND , device);
@@ -1897,7 +1908,14 @@ void qmp_block_commit(const char *device,/* default top_bs is the active layer */ top_bs = bs; -if (top) { +/* Find the 'top' image file for the commit */ +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); }
@@ -1908,7 +1926,14 @@ void qmp_block_commit(const char *device,return ; } -if (has_base && base) { +/* Find the 'base' image file for the commit */ +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);
@@ -1919,6 +1944,12 @@ void qmp_block_commit(const char *device,return ; } +/* Verify that 'base' is in the same chain as 'top' */ +if (!bdrv_is_in_chain (top_bs, base_bs)) { +error_setg (errp,"'base' and 'top' are not in the same chain" ); +return ; + } +if (top_bs == bs) {commit_active_start (bs, base_bs, speed, on_error, block_job_cb, bs, &local_err);
qapi-schema.json +26 -9
@@ -2096,12 +2096,27 @@ # # @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. +# +# @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. +# +# @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,
@@ -2120,17 +2135,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 +22 -7
@@ -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, },
@@ -999,12 +999,27 @@ 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) + +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. + +- "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) + +- "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 4/5] block: Accept node-name arguments for block-commit
Quoted 193 lines · Jeff Cody
This 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_bsis in the chain oftop_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.Signed-off-by Jeff Cody <[email protected]>--- blockdev.c | 35 +++++++++++++++++++++++++++++++++-- qapi-schema.json | 35 ++++++++++++++++++++++++++--------- qmp-commands.hx | 29 ++++++++++++++++++++++------- 3 files changed, 81 insertions(+), 18 deletions(-)blockdev.c +33
-2@@ -1869,7 +1869,9 @@ 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) {@@ -1888,6 +1890,15 @@ void qmp_block_commit(const char *device,/* drain all i/o before commits */ bdrv_drain_all (); +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 ; + } + bs =bdrv_find (device);if (!bs) {error_set (errp,QERR_DEVICE_NOT_FOUND , device);@@ -1897,7 +1908,14 @@ void qmp_block_commit(const char *device,/* default top_bs is the active layer */ top_bs = bs; -if (top) { +/* Find the 'top' image file for the commit */ +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); }@@ -1908,7 +1926,14 @@ void qmp_block_commit(const char *device,return ; } -if (has_base && base) { +/* Find the 'base' image file for the commit */ +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);@@ -1919,6 +1944,12 @@ void qmp_block_commit(const char *device,return ; } +/* Verify that 'base' is in the same chain as 'top' */ +if (!bdrv_is_in_chain (top_bs, base_bs)) { +error_setg (errp,"'base' and 'top' are not in the same chain" ); +return ; + } +if (top_bs == bs) {commit_active_start (bs, base_bs, speed, on_error, block_job_cb, bs, &local_err);qapi-schema.json +26
-9@@ -2096,12 +2096,27 @@ # # @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. +# +# @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. +# +# @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,@@ -2120,17 +2135,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-backupqmp-commands.hx +12
-7@@ -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, },@@ -999,12 +999,27 @@ 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) + +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.
qmp-commands.hx data between 'top' and 'base' into 'base'. · line 1022
Quoted 10 lines
qmp-commands.hx +10
-0+ +- "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) + +- "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)
^ (cosmetic) this block is not aligned with the upper ones.
Quoted 6 lines
qmp-commands.hx +0
-0If 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 4/5] block: Accept node-name arguments for block-commit
In reply to Jeff CodyQuoted 7 lines · Jeff Cody
This 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)
Hmm - we have a bug in existing commands for NOT forcing mutual exclusion even though the schema says they are exclusive. For example, qmp_block_passwd() blindly calls:
bs = bdrv_lookup_bs(has_device ? device : NULL,
has_node_name ? node_name : NULL, &local_err);
and _that_ function blindly favors device name over node name, rather than erroring out if both are supplied. I think we should fix that first - I'd rather that bdrv_lookup_bs either errors out if both names are supplied (rather than making each caller repeat the work), or that it checks that if both names are supplied then it resolves to the same bs.
Hmm - if I have device "foo" with chain "base <- top", would bdrv_lookup_bs("foo", "base") be okay ("base" is a node-name that resides within "device"'s chain) or an error ("base" resolves to a different bs than "device")? Again, all the more reason to have a common function decide the semantics we want, then all other clients automatically pick up on those semantics.
Quoted 8 lines
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_bsis in the chain oftop_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.
I like this addition.
Quoted 12 lines
Signed-off-by Jeff Cody <[email protected]>--- blockdev.c | 35 +++++++++++++++++++++++++++++++++-- qapi-schema.json | 35 ++++++++++++++++++++++++++--------- qmp-commands.hx | 29 ++++++++++++++++++++++------- 3 files changed, 81 insertions(+), 18 deletions(-)
blockdev.c void qmp_block_commit(const char *device, · line 1913
Quoted 4 lines
blockdev.c +3
-1-if (top) { +/* Find the 'top' image file for the commit */ +if (has_top_node_name) { + top_bs =bdrv_lookup_bs (NULL , top_node_name, &local_err);
Hmm. Shouldn't you ALSO verify that 'top_bs' belongs to the chain owned by 'device'? Later on, you verify that 'base_bs' belongs to 'top_bs', but if I pass node names, those names could have found a top unrelated to 'device'; and base related to that top.
Maybe that gives more weight to my idea of possibly allowing bdrv_lookup_bs(device, top_node_name), where the lookup succeeds even when device and top_node_name resolve to different bs, but where top_node_name is a node in the chain of device.
blockdev.c void qmp_block_commit(const char *device, · line 1936
Quoted 10 lines
blockdev.c +8
-1-if (has_base && base) { +/* Find the 'base' image file for the commit */ +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) {
Here, it doesn't matter whether you pass device or NULL for the device name...
blockdev.c void qmp_block_commit(const char *device, · line 1950
Quoted 5 lines
blockdev.c +4
-0+/* Verify that 'base' is in the same chain as 'top' */ +if (!bdrv_is_in_chain (top_bs, base_bs)) { +error_setg (errp,"'base' and 'top' are not in the same chain" ); +return ;
...because this check is still mandatory to prove that a user with chain:
base <- sn1 <- sn2 <- active
is not calling 'device':'active', 'top-node-name':'sn1', 'base-node-name':'sn2' (all that bdrv_lookup_bs can do is verify that base-node-name belongs to device, not that it _also_ belongs to top_bs).
qapi-schema.json line 2100
Quoted 4 lines
qapi-schema.json +2
-2-# @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
I like how you factored th is out up front...
qapi-schema.json line 2109
Quoted 12 lines
#
qapi-schema.json +8
-3-# @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.
...but here, you were not consistent. I'd add "If neither is specified, this is the active image" here...
qapi-schema.json line 2114
Quoted 5 lines
qapi-schema.json +5
-0+# +# @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.
...and drop the second sentence from here.
Quoted 10 lines
qapi-schema.json +5
-0+# +# @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,@@ -2120,17 +2135,19 @@ #
qapi-schema.json line 2143
Quoted 10 lines
qapi-schema.json +4
-2# 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
These last two are arguably sub-conditions of the earlier statement about @top and @top-node-name being invalid (since being invalid can include when both strings are used at once). It wouldn't hurt my feelings to reduce the docs by two lines.
qapi-schema.json line 2150
Quoted 8 lines
qapi-schema.json +2
-2# 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' } }
I'm okay with keeping 'device' mandatory, even though technically the use of a node-name could imply which device is intended. That is, as long as the code correctly errors out when device and top-node-name don't resolve to the same chain :)
qmp-commands.hx data between 'top' and 'base' into 'base'. · line 1017
Quoted 7 lines
qmp-commands.hx +7
-0+ +For 'top', either top or top-node-name must be set but not both. + +- "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)
Same blurb about hoisting the 'not specified => active' sentence to the common text, rather than leaving it in the 'top' text.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 4/5] block: Accept node-name arguments for block-commit
Quoted 11 lines · Eric Blake
Quoted 7 lines · Jeff Cody
This 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)Hmm - we have a bug in existing commands for NOT forcing mutual exclusion even though the schema says they are exclusive.
Those are a slightly different scenario, however. In this patch, we are dealing with a filename string, and a node-name string. There is more of a 1:1 relationship between those two (both identify a particular image). The other existing commands, however, deal with a device name string (identifying a drive chain) or a node-name string (identifying a particular image).
That said, I think the other commands should be modified to enforce the documentation, but it doesn't really relate to this patch.
Quoted 9 lines
For example,
qmp_block_passwd()blindly calls:bs = bdrv_lookup_bs(has_device ? device : NULL,
has_node_name?node_name: NULL,&local_err);and _that_ function blindly favors device name over node name, rather than erroring out if both are supplied.
I find this an odd function to begin with, because node_name uniquely identifies any given BDS from any chain. Device name will uniquely identify a whole chain, by returning its top-most BDS.
The function seems confused.
Quoted 5 lines
I think we should fix that
first - I'd rather thatbdrv_lookup_bseither errors out if both names are supplied (rather than making each caller repeat the work), or that it checks that if both names are supplied then it resolves to the same bs.
I disagree on this part, at least partially. It think it needs to be changed, but I don't think it needs to be part of this series. I think that can be a separate series, to address that across the other QAPI commands that accept node-name and device name
Quoted 35 lines
Hmm - if I have device "foo" with chain "base <- top", would
bdrv_lookup_bs("foo", "base")be okay ("base" is a node-name that resides within "device"'s chain) or an error ("base" resolves to a different bs than "device")? Again, all the more reason to have a common function decide the semantics we want, then all other clients automatically pick up on those semantics.Quoted 8 lines
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_bsis in the chain oftop_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.I like this addition.
Quoted 12 lines
Signed-off-by Jeff Cody <[email protected]>--- blockdev.c | 35 +++++++++++++++++++++++++++++++++-- qapi-schema.json | 35 ++++++++++++++++++++++++++--------- qmp-commands.hx | 29 ++++++++++++++++++++++------- 3 files changed, 81 insertions(+), 18 deletions(-)blockdev.c void qmp_block_commit(const char *device, · line 1913
Quoted 4 lines
blockdev.c +3
-1-if (top) { +/* Find the 'top' image file for the commit */ +if (has_top_node_name) { + top_bs =bdrv_lookup_bs (NULL , top_node_name, &local_err);
Quoted 5 lines
Hmm. Shouldn't you ALSO verify that '
top_bs' belongs to the chain owned by 'device'? Later on, you verify that 'base_bs' belongs to 'top_bs', but if I pass node names, those names could have found a top unrelated to 'device'; and base related to that top.
It is not needed - it is not relevant for the active commit case (the current check will catch that, since top_bs == bs). And for the non-active commit case, commit_start() will return an error if 'top_bs' is not in 'bs', so the case of 'base_bs' or 'top_bs' not being in 'device' is already covered.
Quoted 16 lines
Maybe that gives more weight to my idea of possibly allowing
bdrv_lookup_bs(device, top_node_name), where the lookup succeeds even when device andtop_node_nameresolve to different bs, but wheretop_node_nameis a node in the chain of device.blockdev.c void qmp_block_commit(const char *device, · line 1936
Quoted 10 lines
blockdev.c +8
-1-if (has_base && base) { +/* Find the 'base' image file for the commit */ +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) {
Quoted 3 lines
Here, it doesn't matter whether you pass device or NULL for the device name...
This conditional is just to determine if we are finding the backing image referenced by the string "base" or just using the default bottom-most base image.
Quoted 21 lines
blockdev.c void qmp_block_commit(const char *device, · line 1950
Quoted 5 lines
blockdev.c +4
-0+/* Verify that 'base' is in the same chain as 'top' */ +if (!bdrv_is_in_chain (top_bs, base_bs)) { +error_setg (errp,"'base' and 'top' are not in the same chain" ); +return ;...because this check is still mandatory to prove that a user with chain:
base <- sn1 <- sn2 <- activeis not calling 'device':'active', 'top-node-name':'sn1', 'base-node-name':'sn2' (all that
bdrv_lookup_bscan do is verify that base-node-name belongs to device, not that it _also_ belongs totop_bs).qapi-schema.json line 2100
Quoted 4 lines
qapi-schema.json +2
-2-# @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 imageI like how you factored th is out up front...
Quoted 16 lines
qapi-schema.json line 2109
Quoted 12 lines
#
qapi-schema.json +8
-3-# @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....but here, you were not consistent. I'd add "If neither is specified, this is the active image" here...
OK, thanks.
Quoted 8 lines
qapi-schema.json line 2114
Quoted 5 lines
qapi-schema.json +5
-0+# +# @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....and drop the second sentence from here.
OK, thanks.
Quoted 17 lines
Quoted 17 lines
qapi-schema.json +7
-2+# +# @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,@@ -2120,17 +2135,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
Quoted 9 lines
qapi-schema.json line 2143
Quoted 3 lines
qapi-schema.json +2
-0# If @speed is invalid, InvalidParameter +# If both @base and @base-node-name are specified, GenericError +# If both @top and @top-node-name are specified, GenericErrorThese last two are arguably sub-conditions of the earlier statement about @top and @top-node-name being invalid (since being invalid can include when both strings are used at once). It wouldn't hurt my feelings to reduce the docs by two lines.
OK. I called it out explicitly, since it is currently different behavior from the other functions.
Quoted 9 lines
qapi-schema.json line 2150
Quoted 8 lines
qapi-schema.json +2
-2# 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' } }
Quoted 5 lines
I'm okay with keeping 'device' mandatory, even though technically the use of a node-name could imply which device is intended. That is, as long as the code correctly errors out when device and top-node-name don't resolve to the same chain :)
I had thought about making it optional, but I do think it is better as mandatory - it makes sure the user knows which chain they are operating on, so it acts as a (small) check against user mistakes.
Quoted 12 lines
qmp-commands.hx data between 'top' and 'base' into 'base'. · line 1017
Quoted 8 lines
qmp-commands.hx +7
-0+ +For 'top', either top or top-node-name must be set but not both. + +- "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)Same blurb about hoisting the 'not specified => active' sentence to the common text, rather than leaving it in the 'top' text.
OK, thanks.
PATCH 5/5 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-schema.json | 18 ++++++++++++++++-- qmp-commands.hx | 14 +++++++++++++- 7 files changed, 52 insertions(+), 11 deletions(-)
block.c +6 -2
@@ -2547,12 +2547,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 ;
@@ -2604,7 +2607,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
@@ -1872,6 +1872,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) {
@@ -1951,11 +1952,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); + backing_file, &local_err); }if (local_err !=NULL ) {error_propagate (errp, local_err);
include/block/block.h +2 -1
@@ -266,7 +266,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
@@ -428,13 +428,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-schema.json +16 -2
@@ -2118,6 +2118,18 @@ # 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 +# backing file string is only written into the the +# image file metadata - internal structures inside +# QEMU are not updated, and the string is not validated. +# If not specified, QEMU will automatically determine +# the backing file string to use. 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
@@ -2130,7 +2142,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
@@ -2141,13 +2152,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 +13 -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,18 @@ For 'top', either top or top-node-name must be set but not both. 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 + backing file string is only written into the the + image file metadata - internal structures inside + QEMU are not updated, and the string is not validated. + If not specified, QEMU will automatically determine + the backing file string to use. 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
Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file
Quoted 135 lines · Jeff Cody
On 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.
Signed-off-by Jeff Cody <[email protected]>--- block.c | 8 ++++++-- block/commit.c | 9 ++++++--- blockdev.c | 8 +++++++- include/block/block.h | 3 ++- include/block/block_int.h | 3 ++- qapi-schema.json | 18 ++++++++++++++++-- qmp-commands.hx | 14 +++++++++++++- 7 files changed, 52 insertions(+), 11 deletions(-)block.c +6
-2@@ -2547,12 +2547,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 ;@@ -2604,7 +2607,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 +2
-0@@ -1872,6 +1872,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) {@@ -1951,11 +1952,16 @@ void qmp_block_commit(const char *device, }if (top_bs == bs) { +if (has_backing_file) {
blockdev.c void qmp_block_commit(const char *device, · line 1964
Quoted 10 lines
blockdev.c +5
-1+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); + backing_file, &local_err);
I don't know QAPI well enough to be sure but are we certain that if has_backing_file == false then backing_file == NULL and not some random pointer ?
If am thinking to add has_backing_file ? backing_file : NULL here.
Quoted 22 lines
blockdev.c +0
-0}if (local_err !=NULL ) {error_propagate (errp, local_err);include/block/block.h +2
-1@@ -266,7 +266,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 +0
-0
include/block/block_int.h void stream_start(BlockDriverState *bs, BlockDriverState *base, · line 431
Quoted 4 lines
include/block/block_int.h +1
-0@@ -428,13 +428,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
Miss a final dot to be like the other lines.
Quoted 35 lines
include/block/block_int.h +1
-1* @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-schema.json +12
-0@@ -2118,6 +2118,18 @@ # 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 +# backing file string is only written into the the +# image file metadata - internal structures inside +# QEMU are not updated, and the string is not validated. +# If not specified, QEMU will automatically determine +# the backing file string to use. 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
qapi-schema.json line 2133
Quoted 4 lines
qapi-schema.json +0
-1@@ -2130,7 +2142,6 @@ # size of the smaller top, you can safely truncate it # yourself once the commit operation successfully completes. # -#
Spurious line removal.
Quoted 57 lines
qapi-schema.json +4
-1# @speed: #optional the maximum speed, in bytes per second # # Returns: Nothing on success@@ -2141,13 +2152,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-backupqmp-commands.hx +13
-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,18 @@ For 'top', either top or top-node-name must be set but not both. 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 + backing file string is only written into the the + image file metadata - internal structures inside + QEMU are not updated, and the string is not validated. + If not specified, QEMU will automatically determine + the backing file string to use. 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-completeSignature · 1 line
1.8.3.1
Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file
Quoted 11 lines · Benoît Canet
blockdev.c void qmp_block_commit(const char *device, · line 1964
Quoted 9 lines · Jeff Cody
On 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.
} else {
commit_start(bs,base_bs,top_bs, speed,on_error,block_job_cb, bs,blockdev.c +1
-1- &local_err); + backing_file, &local_err);
Quoted 5 lines
I don't know QAPI well enough to be sure but are we certain that if
has_backing_file== false thenbacking_file== NULL and not some random pointer ?If am thinking to add
has_backing_file?backing_file: NULL here.
We are moving towards having qapi guarantee sane defaults for FOO when has_FOO is false; but aren't there yet. You are correct that this needs to guarantee that we aren't passing random memory.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file
Quoted 16 lines · Eric Blake
Quoted 15 lines · Benoît Canet
blockdev.c void qmp_block_commit(const char *device, · line 1964
Quoted 9 lines · Jeff Cody
On 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.
} else {
commit_start(bs,base_bs,top_bs, speed,on_error,block_job_cb, bs,blockdev.c +1
-1- &local_err); + backing_file, &local_err);I don't know QAPI well enough to be sure but are we certain that if
has_backing_file== false thenbacking_file== NULL and not some random pointer ?
Quoted 6 lines
Quoted 1 line
If am thinking to add
has_backing_file?backing_file: NULL here.We are moving towards having qapi guarantee sane defaults for FOO when
has_FOOis false; but aren't there yet. You are correct that this needs to guarantee that we aren't passing random memory.
The QAPI code generator for the QMP input marshaller initializes all pointers to NULL, and all bools to false. If has_ is false, then the associated pointer will also be NULL, so it is safe to just pass backing_file.
Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file
Quoted 10 lines · Jeff Cody
Quoted 5 lines
We are moving towards having qapi guarantee sane defaults for FOO when
has_FOOis false; but aren't there yet. You are correct that this needs to guarantee that we aren't passing random memory.The QAPI code generator for the QMP input marshaller initializes all pointers to NULL, and all bools to false. If
has_is false, then the associated pointer will also be NULL, so it is safe to just passbacking_file.
It didn't used to, and we still haven't documented that as being a reliable guarantee, nor does the testsuite cover that. It's still safer to not rely on that fact until we make it part of the contract of qapi.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file
In reply to Jeff CodyQuoted 3 lines · Jeff Cody
On 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.
Do we want to allow qemu to error out in situations where it might get things wrong (such as if a gluster protocol is backed by a local filename) - and mandate that the user supply a backing string in those situations? But this patch is a strict improvement even if you don't go that far, because it gives libvirt the flexibility to shift the burden of name generation up the stack rather than sticking qemu with that task.
Hmm - how will this be discoverable by libvirt? Maybe when libvirt is doing the 'qemu -m none' probing, it can hotplug a device pointing to /dev/null (libvirt _already_ does that to test if add-fd works), and intentionally omit a node name. If libvirt then queries the device, and sees that the __qemu##000NNNN node-name was auto-assigned, then it can be assumed that this qemu is new enough to provide node-names for ALL operations (but that means this series is incomplete unless we add node-name support to all remaining block commands, such as block-stream, drive-mirror, and drive-backup). This part is where I wonder if patch 1/5 should be rebased to be last in the series.
Quoted 3 lines
For instance, certain relative pathnames may fail, or drives may have been specified originally by file descriptor (e.g.
/dev/fd/???),
I'd document this as /dev/fdset/???, which is the magic string QMP uses with its add-fd command (/dev/fd/??? is platform-specific whether it will work, /dev/fdset/??? is guaranteed to work in all builds of qemu).
Quoted 14 lines
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.
Quoted 7 lines
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.
In short, this new command option allows the equivalent of 'qemu-img rebase -u' on a live image. Definitely a needed functionality.
Quoted 15 lines
Signed-off-by Jeff Cody <[email protected]>--- block.c | 8 ++++++-- block/commit.c | 9 ++++++--- blockdev.c | 8 +++++++- include/block/block.h | 3 ++- include/block/block_int.h | 3 ++- qapi-schema.json | 18 ++++++++++++++++-- qmp-commands.hx | 14 +++++++++++++- 7 files changed, 52 insertions(+), 11 deletions(-)} else {
commit_start(bs,base_bs,top_bs, speed,on_error,block_job_cb, bs,
blockdev.c void qmp_block_commit(const char *device, · line 1964
Quoted 2 lines
blockdev.c +1
-1- &local_err); + backing_file, &local_err);
See the rest of the thread about using 'has_backing_file ? backing_file : NULL' here,
qapi-schema.json line 2128
Quoted 8 lines
qapi-schema.json +8
-0+# @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 +# backing file string is only written into the the +# image file metadata - internal structures inside +# QEMU are not updated, and the string is not validated. +# If not specified, QEMU will automatically determine +# the backing file string to use. Care should be taken
Maybe "If not specified, QEMU will automatically determine a backing file string to use, or error out if there is no obvious choice", to allow us flexibility in erroring out on corner cases such as mixing gluster with local files.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file
Quoted 5 lines · Eric Blake
Quoted 3 lines · Jeff Cody
On 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.
Quoted 7 lines
Do we want to allow qemu to error out in situations where it might get things wrong (such as if a gluster protocol is backed by a local filename) - and mandate that the user supply a backing string in those situations? But this patch is a strict improvement even if you don't go that far, because it gives libvirt the flexibility to shift the burden of name generation up the stack rather than sticking qemu with that task.
Yes, I think it would be a good idea to change the language here to allow us to do that as needed. I like what you wrote at the end of your comments.
Quoted 11 lines
Hmm - how will this be discoverable by libvirt? Maybe when libvirt is doing the 'qemu -m none' probing, it can hotplug a device pointing to
/dev/null(libvirt _already_ does that to test if add-fd works), and intentionally omit a node name. If libvirt then queries the device, and sees that the __qemu##000NNNN node-name was auto-assigned, then it can be assumed that this qemu is new enough to provide node-names for ALL operations (but that means this series is incomplete unless we add node-name support to all remaining block commands, such as block-stream, drive-mirror, and drive-backup). This part is where I wonder if patch 1/5 should be rebased to be last in the series.
Ah... I had originally planned on submitting separate patches for each of the block jobs, to make reviewing easier. But your idea on how libvirt can discover this is a good one, and would mandate changing those commands all in one series to be effective. So this series will grow by a few patches. :)
If libvirt is going to use the autogenerated string format for decisions, we should also document the string format in the QAPI docs.
Quoted 8 lines
Quoted 3 lines
For instance, certain relative pathnames may fail, or drives may have been specified originally by file descriptor (e.g.
/dev/fd/???),I'd document this as
/dev/fdset/???, which is the magic string QMP uses with its add-fd command (/dev/fd/??? is platform-specific whether it will work,/dev/fdset/??? is guaranteed to work in all builds of qemu).
Sure, best to be consistent.
Quoted 18 lines
Quoted 18 lines
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).
Quoted 7 lines
Quoted 3 lines
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.
In short, this new command option allows the equivalent of 'qemu-img rebase -u' on a live image. Definitely a needed functionality.
Would it be useful to have a stand-alone QMP command to change the backing-file, as well? As this stands, it will only change the backing file if you are also merging data down the chain.
If you want/need the ability to do a true 'qemu-img rebase -u' on any given image without other chain modification, that needs a new command.
Quoted 22 lines
Quoted 15 lines
Signed-off-by Jeff Cody <[email protected]>--- block.c | 8 ++++++-- block/commit.c | 9 ++++++--- blockdev.c | 8 +++++++- include/block/block.h | 3 ++- include/block/block_int.h | 3 ++- qapi-schema.json | 18 ++++++++++++++++-- qmp-commands.hx | 14 +++++++++++++- 7 files changed, 52 insertions(+), 11 deletions(-)} else {
commit_start(bs,base_bs,top_bs, speed,on_error,block_job_cb, bs,blockdev.c void qmp_block_commit(const char *device, · line 1964
Quoted 2 lines
blockdev.c +1
-1- &local_err); + backing_file, &local_err);See the rest of the thread about using '
has_backing_file?backing_file: NULL' here,
Quoted 14 lines
qapi-schema.json line 2128
Quoted 8 lines
qapi-schema.json +8
-0+# @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 +# backing file string is only written into the the +# image file metadata - internal structures inside +# QEMU are not updated, and the string is not validated. +# If not specified, QEMU will automatically determine +# the backing file string to use. Care should be takenMaybe "If not specified, QEMU will automatically determine a backing file string to use, or error out if there is no obvious choice", to allow us flexibility in erroring out on corner cases such as mixing gluster with local files.
+1
Re: [Qemu-devel] [PATCH 5/5] block: extend block-commit to accept a string for the backing file
Quoted 12 lines · Jeff Cody
Quoted 11 lines
Hmm - how will this be discoverable by libvirt? Maybe when libvirt is doing the 'qemu -m none' probing, it can hotplug a device pointing to
/dev/null(libvirt _already_ does that to test if add-fd works), and intentionally omit a node name. If libvirt then queries the device, and sees that the __qemu##000NNNN node-name was auto-assigned, then it can be assumed that this qemu is new enough to provide node-names for ALL operations (but that means this series is incomplete unless we add node-name support to all remaining block commands, such as block-stream, drive-mirror, and drive-backup). This part is where I wonder if patch 1/5 should be rebased to be last in the series.
Quoted 8 lines
Ah... I had originally planned on submitting separate patches for each of the block jobs, to make reviewing easier. But your idea on how libvirt can discover this is a good one, and would mandate changing those commands all in one series to be effective. So this series will grow by a few patches. :)
If libvirt is going to use the autogenerated string format for decisions, we should also document the string format in the QAPI docs.
Adding a new command is much easier to probe for (a single query-commands, which we are already using) than requiring a sequence of operations (hotplug, then query to see if a name was assigned), and probably even more direct (if the standalone command exists, then so does the integrated use of setting backing names).
Quoted 5 lines
Quoted 4 lines
In short, this new command option allows the equivalent of 'qemu-img rebase -u' on a live image. Definitely a needed functionality.
Quoted 7 lines
Would it be useful to have a stand-alone QMP command to change the backing-file, as well? As this stands, it will only change the backing file if you are also merging data down the chain.
If you want/need the ability to do a true 'qemu-img rebase -u' on any given image without other chain modification, that needs a new command.
Yes, I think that's probably a wise idea to provide a dedicated command for just changing the name recorded in a backing file.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org