* Re: [Qemu-devel] [PATCH] quorum: Implement bdrv_get_specific_info
2016-03-24 3:17 [Qemu-devel] [PATCH] quorum: Implement bdrv_get_specific_info Wen Congyang
@ 2016-03-24 3:26 ` Fam Zheng
2016-03-24 12:52 ` Alberto Garcia
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Fam Zheng @ 2016-03-24 3:26 UTC (permalink / raw)
To: Wen Congyang
Cc: Kevin Wolf, Changlong Xie, Alberto Garcia, qemu-devl,
Dr. David Alan Gilbert, Stefan Hajnoczi, Max Reitz
On Thu, 03/24 11:17, Wen Congyang wrote:
> 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 <wency@cn.fujitsu.com>
> ---
> block/quorum.c | 27 +++++++++++++++++++++++++++
> qapi/block-core.json | 15 ++++++++++++++-
> 2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index da15465..afe6c3f 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -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)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b1cf77d..bd3e12d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -75,6 +75,18 @@
> } }
>
> ##
> +# @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']
> + } }
> +
> +##
> # @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'
> } }
>
> ##
> --
> 2.5.5
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] quorum: Implement bdrv_get_specific_info
2016-03-24 3:17 [Qemu-devel] [PATCH] quorum: Implement bdrv_get_specific_info Wen Congyang
2016-03-24 3:26 ` Fam Zheng
@ 2016-03-24 12:52 ` Alberto Garcia
2016-03-24 16:55 ` Eric Blake
2016-03-24 17:02 ` Max Reitz
3 siblings, 0 replies; 5+ messages in thread
From: Alberto Garcia @ 2016-03-24 12:52 UTC (permalink / raw)
To: Wen Congyang, qemu-devl, Max Reitz, Dr. David Alan Gilbert,
Kevin Wolf
Cc: Changlong Xie, Stefan Hajnoczi
On Thu 24 Mar 2016 04:17:12 AM CET, Wen Congyang wrote:
> 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 <wency@cn.fujitsu.com>
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.
> + 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.
> ##
> +# @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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] quorum: Implement bdrv_get_specific_info
2016-03-24 3:17 [Qemu-devel] [PATCH] quorum: Implement bdrv_get_specific_info Wen Congyang
2016-03-24 3:26 ` Fam Zheng
2016-03-24 12:52 ` Alberto Garcia
@ 2016-03-24 16:55 ` Eric Blake
2016-03-24 17:02 ` Max Reitz
3 siblings, 0 replies; 5+ messages in thread
From: Eric Blake @ 2016-03-24 16:55 UTC (permalink / raw)
To: Wen Congyang, qemu-devl, Max Reitz, Alberto Garcia,
Dr. David Alan Gilbert, Kevin Wolf
Cc: Changlong Xie, Stefan Hajnoczi
[-- Attachment #1: Type: text/plain, Size: 2974 bytes --]
On 03/23/2016 09:17 PM, Wen Congyang wrote:
> 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 <wency@cn.fujitsu.com>
> ---
> 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?
>
> diff --git a/block/quorum.c b/block/quorum.c
> index da15465..afe6c3f 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -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...
> + strList **next;
> +
> + *spec_info = (ImageInfoSpecific){
> + .type = IMAGE_INFO_SPECIFIC_KIND_QUORUM,
...you are assigning all fields here.
> + .u = {
> + .quorum.data = g_new0(ImageInfoSpecificQuorum, 1),
> + },
I think you could just directly do:
.u.quorum.data = ...
instead of nesting {}.
> + };
> +
> + 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.
> + next = &(*next)->next;
> + }
> +
> + return spec_info;
> +}
> +
> +++ b/qapi/block-core.json
> @@ -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
> +#
> +# 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?
> +##
> +{ 'struct': 'ImageInfoSpecificQuorum',
> + 'data': {
> + 'child-name': ['str']
> + } }
Other than the naming, it looks okay.
> @@ -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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] quorum: Implement bdrv_get_specific_info
2016-03-24 3:17 [Qemu-devel] [PATCH] quorum: Implement bdrv_get_specific_info Wen Congyang
` (2 preceding siblings ...)
2016-03-24 16:55 ` Eric Blake
@ 2016-03-24 17:02 ` Max Reitz
3 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2016-03-24 17:02 UTC (permalink / raw)
To: Wen Congyang, qemu-devl, Alberto Garcia, Dr. David Alan Gilbert,
Kevin Wolf
Cc: Changlong Xie, Stefan Hajnoczi
[-- Attachment #1.1: Type: text/plain, Size: 3630 bytes --]
On 24.03.2016 04:17, Wen Congyang wrote:
> 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 <wency@cn.fujitsu.com>
> ---
> block/quorum.c | 27 +++++++++++++++++++++++++++
> qapi/block-core.json | 15 ++++++++++++++-
> 2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/block/quorum.c b/block/quorum.c
> index da15465..afe6c3f 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -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)
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b1cf77d..bd3e12d 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -75,6 +75,18 @@
> } }
>
> ##
> +# @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
> +
> +##
> # @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'
> } }
>
> ##
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread