quorum: Implement bdrv_get_specific_info
PATCH 1/1 quorum: Implement bdrv_get_specific_info
The monitor command 'query-block' or 'info block' will output the format specific information. So we can get each child's child-name after this patch. This useful for dynamic reconfiguration.
--- block/quorum.c | 27 +++++++++++++++++++++++++++ qapi/block-core.json | 15 ++++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-)
block/quorum.c +27 -0
@@ -1054,6 +1054,31 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) bs->full_open_options = opts; } +static ImageInfoSpecific *quorum_get_specific_info (BlockDriverState *bs) +{ +int i; +BDRVQuorumState *s = bs->opaque; +ImageInfoSpecific *spec_info =g_new0 (ImageInfoSpecific,1 ); +strList **next; + + *spec_info = (ImageInfoSpecific ){ + .type =IMAGE_INFO_SPECIFIC_KIND_QUORUM , + .u = { + .quorum.data =g_new0 (ImageInfoSpecificQuorum,1 ), + }, + }; + + next = &spec_info->u.quorum.data->child_name; +for (i =0 ; i < s->num_children; i++) { + *next =g_new0 (strList,1 ); + (*next)->value =g_strdup (s->children[i]->name); + (*next)->next =NULL ; + next = &(*next)->next; + } + +return spec_info; +} +static BlockDriver bdrv_quorum = { .format_name ="quorum" , .protocol_name ="quorum" ,
@@ -1077,6 +1102,8 @@ static BlockDriver bdrv_quorum = { .is_filter = true, .bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter, + + .bdrv_get_specific_info = quorum_get_specific_info, };static void bdrv_quorum_init (void )
qapi/block-core.json +14 -1
@@ -75,6 +75,18 @@ } } ## +# @ImageInfoSpecificQuorum: +# +# @child-name: List of child name +# +# Since: 2.7 +## +{ 'struct': 'ImageInfoSpecificQuorum', + 'data': { + 'child-name': ['str'] + } } + +## # @ImageInfoSpecific: # # A discriminated record of image format specific information structures.
@@ -85,7 +97,8 @@ { 'union': 'ImageInfoSpecific', 'data': { 'qcow2': 'ImageInfoSpecificQCow2', - 'vmdk': 'ImageInfoSpecificVmdk' + 'vmdk': 'ImageInfoSpecificVmdk', + 'quorum': 'ImageInfoSpecificQuorum' } } ##
Signature · 1 line
2.5.5
Quoted 62 lines · Wen Congyang
The monitor command 'query-block' or 'info block' will output the format specific information. So we can get each child's child-name after this patch. This useful for dynamic reconfiguration.
Signed-off-by Wen Congyang <[email protected]>--- block/quorum.c | 27 +++++++++++++++++++++++++++ qapi/block-core.json | 15 ++++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-)block/quorum.c +27
-0@@ -1054,6 +1054,31 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) bs->full_open_options = opts; } +static ImageInfoSpecific *quorum_get_specific_info (BlockDriverState *bs) +{ +int i; +BDRVQuorumState *s = bs->opaque; +ImageInfoSpecific *spec_info =g_new0 (ImageInfoSpecific,1 ); +strList **next; + + *spec_info = (ImageInfoSpecific ){ + .type =IMAGE_INFO_SPECIFIC_KIND_QUORUM , + .u = { + .quorum.data =g_new0 (ImageInfoSpecificQuorum,1 ), + }, + }; + + next = &spec_info->u.quorum.data->child_name; +for (i =0 ; i < s->num_children; i++) { + *next =g_new0 (strList,1 ); + (*next)->value =g_strdup (s->children[i]->name); + (*next)->next =NULL ; + next = &(*next)->next; + } + +return spec_info; +} +static BlockDriver bdrv_quorum = { .format_name ="quorum" , .protocol_name ="quorum" ,@@ -1077,6 +1102,8 @@ static BlockDriver bdrv_quorum = { .is_filter = true, .bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter, + + .bdrv_get_specific_info = quorum_get_specific_info, };static void bdrv_quorum_init (void )qapi/block-core.json +0
-0@@ -75,6 +75,18 @@ } }
qapi/block-core.json line 86
Quoted 10 lines
qapi/block-core.json +9
-0## +# @ImageInfoSpecificQuorum: +# +# @child-name: List of child name +# +# Since: 2.7 +## +{ 'struct': 'ImageInfoSpecificQuorum', + 'data': { + 'child-name': ['str']
I think a plural key name is better here:
'children': ['str']
Quoted 21 lines
qapi/block-core.json +5
-1+ } } + +## # @ImageInfoSpecific: # # A discriminated record of image format specific information structures.@@ -85,7 +97,8 @@ { 'union': 'ImageInfoSpecific', 'data': { 'qcow2': 'ImageInfoSpecificQCow2', - 'vmdk': 'ImageInfoSpecificVmdk' + 'vmdk': 'ImageInfoSpecificVmdk', + 'quorum': 'ImageInfoSpecificQuorum' } } ##Signature · 1 line
2.5.5
Quoted 5 lines · Wen Congyang
The monitor command 'query-block' or 'info block' will output the format specific information. So we can get each child's child-name after this patch. This useful for dynamic reconfiguration.
Signed-off-by Wen Congyang <[email protected]>
The patch seems correct (but see the comments below), but I'm still wondering why we need to use child_name for this at all. If I rememeber correctly the previous discussion we cannot simply use node-name because a Quorum could have several children with the same name, but I'm still unsure about how that would happen and what's the use case.
block/quorum.c static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) · line 1069
Quoted 9 lines
block/quorum.c +9
-0+ImageInfoSpecific *spec_info =g_new0 (ImageInfoSpecific,1 ); +strList **next; + + *spec_info = (ImageInfoSpecific ){ + .type =IMAGE_INFO_SPECIFIC_KIND_QUORUM , + .u = { + .quorum.data =g_new0 (ImageInfoSpecificQuorum,1 ), + }, + };
I don't think you need to use g_new0() instead of g_new() if you're immediately doing that assignment afterwards.
qapi/block-core.json line 87
Quoted 11 lines
##
qapi/block-core.json +10
-0+# @ImageInfoSpecificQuorum: +# +# @child-name: List of child name +# +# Since: 2.7 +## +{ 'struct': 'ImageInfoSpecificQuorum', + 'data': { + 'child-name': ['str'] + } }
I think it's better in plural: 'children' or 'children-names'
Berto
Quoted 9 lines · Wen Congyang
The monitor command 'query-block' or 'info block' will output the format specific information. So we can get each child's child-name after this patch. This useful for dynamic reconfiguration.
Signed-off-by Wen Congyang <[email protected]>--- block/quorum.c | 27 +++++++++++++++++++++++++++ qapi/block-core.json | 15 ++++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-)
Can you add an example QMP session with the new information included, as part of the commit message, to make it easier to see in context what you are adding?
Quoted 6 lines
block/quorum.c +0
-0
block/quorum.c static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) · line 1061
Quoted 8 lines
block/quorum.c +5
-0@@ -1054,6 +1054,31 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) bs->full_open_options = opts; } +static ImageInfoSpecific *quorum_get_specific_info (BlockDriverState *bs) +{ +int i; +BDRVQuorumState *s = bs->opaque; +ImageInfoSpecific *spec_info =g_new0 (ImageInfoSpecific,1 );
Others have pointed out that this can be g_new(), since...
block/quorum.c static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) · line 1065
Quoted 4 lines
block/quorum.c +4
-0+strList **next; + + *spec_info = (ImageInfoSpecific ){ + .type =IMAGE_INFO_SPECIFIC_KIND_QUORUM ,
...you are assigning all fields here.
block/quorum.c static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) · line 1068
Quoted 3 lines
block/quorum.c +3
-0+ .u = { + .quorum.data = g_new0(ImageInfoSpecificQuorum, 1), + },
I think you could just directly do:
.u.quorum.data = ...
instead of nesting {}.
block/quorum.c static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) · line 1075
Quoted 7 lines
block/quorum.c +7
-0+ }; + + next = &spec_info->u.quorum.data->child_name; +for (i =0 ; i < s->num_children; i++) { + *next =g_new0 (strList,1 ); + (*next)->value =g_strdup (s->children[i]->name); + (*next)->next =NULL ;
Dead assignment, thanks to the g_new0() above.
Quoted 9 lines
block/quorum.c +7
-0+ next = &(*next)->next; + } + +return spec_info; +} + +++ b/qapi/block-core.json
block/quorum.c line 80
Quoted 6 lines
block/quorum.c +3
-0@@ -75,6 +75,18 @@ } } ## +# @ImageInfoSpecificQuorum: +# +# @child-name: List of child name
As others have pointed out, I'd prefer:
@children: list of children's names
block/quorum.c line 82
Quoted 2 lines
block/quorum.c +2
-0+# +# Since : 2.7
Is this information needed in 2.6 (basically, a bug fix to finish an incomplete feature addition), or are you really okay deferring it to 2.7?
block/quorum.c line 87
Quoted 5 lines
block/quorum.c +5
-0+## +{ 'struct': 'ImageInfoSpecificQuorum', + 'data': { + 'child-name': ['str'] + } }
Other than the naming, it looks okay.
block/quorum.c line 101
Quoted 7 lines
block/quorum.c +2
-1@@ -85,7 +97,8 @@ { 'union': 'ImageInfoSpecific', 'data': { 'qcow2': 'ImageInfoSpecificQCow2', - 'vmdk': 'ImageInfoSpecificVmdk' + 'vmdk': 'ImageInfoSpecificVmdk', + 'quorum': 'ImageInfoSpecificQuorum'
Worth keeping this list sorted? QAPI doesn't care, but as the list gets longer, sorted is easier to maintain.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Quoted 63 lines · Wen Congyang
The monitor command 'query-block' or 'info block' will output the format specific information. So we can get each child's child-name after this patch. This useful for dynamic reconfiguration.
Signed-off-by Wen Congyang <[email protected]>--- block/quorum.c | 27 +++++++++++++++++++++++++++ qapi/block-core.json | 15 ++++++++++++++- 2 files changed, 41 insertions(+), 1 deletion(-)block/quorum.c +27
-0@@ -1054,6 +1054,31 @@ static void quorum_refresh_filename(BlockDriverState *bs, QDict *options) bs->full_open_options = opts; } +static ImageInfoSpecific *quorum_get_specific_info (BlockDriverState *bs) +{ +int i; +BDRVQuorumState *s = bs->opaque; +ImageInfoSpecific *spec_info =g_new0 (ImageInfoSpecific,1 ); +strList **next; + + *spec_info = (ImageInfoSpecific ){ + .type =IMAGE_INFO_SPECIFIC_KIND_QUORUM , + .u = { + .quorum.data =g_new0 (ImageInfoSpecificQuorum,1 ), + }, + }; + + next = &spec_info->u.quorum.data->child_name; +for (i =0 ; i < s->num_children; i++) { + *next =g_new0 (strList,1 ); + (*next)->value =g_strdup (s->children[i]->name); + (*next)->next =NULL ; + next = &(*next)->next; + } + +return spec_info; +} +static BlockDriver bdrv_quorum = { .format_name ="quorum" , .protocol_name ="quorum" ,@@ -1077,6 +1102,8 @@ static BlockDriver bdrv_quorum = { .is_filter = true, .bdrv_recurse_is_first_non_filter = quorum_recurse_is_first_non_filter, + + .bdrv_get_specific_info = quorum_get_specific_info, };static void bdrv_quorum_init (void )qapi/block-core.json +0
-0@@ -75,6 +75,18 @@ } } ##
qapi/block-core.json line 87
Quoted 10 lines
qapi/block-core.json +10
-0+# @ImageInfoSpecificQuorum: +# +# @child-name: List of child name +# +# Since: 2.7 +## +{ 'struct': 'ImageInfoSpecificQuorum', + 'data': { + 'child-name': ['str'] + } }
I'd rather make this a generic struct than a str and rename the key to 'children', as Fam suggested, because in the future we may want to emit more information about each child.
In any case, while it is true that I suggested this solution, I also said that I think a generic solution for the whole block layer would make sense.
Because I generally think that general solutions are better than specialized ones (if both do the same thing, basically), I'd prefer the general solution. Is there a reason why you chose to go for the specialized quorum-only implementation, other than because it's easier to do?
(I admit that introducing the graph querying command I suggested would probably be subjected to more discussion than the design chosen in this patch. But I personally think it's worth it.)
Max
Quoted 16 lines
qapi/block-core.json +4
-1+ +## # @ImageInfoSpecific: # # A discriminated record of image format specific information structures.@@ -85,7 +97,8 @@ { 'union': 'ImageInfoSpecific', 'data': { 'qcow2': 'ImageInfoSpecificQCow2', - 'vmdk': 'ImageInfoSpecificVmdk' + 'vmdk': 'ImageInfoSpecificVmdk', + 'quorum': 'ImageInfoSpecificQuorum' } } ##