Re: [Qemu-devel] [PATCH] quorum: Implement bdrv_get_specific_info

[Max Reitz <mreitz@redhat.com>]

[-- 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 --]