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

[Eric Blake <eblake@redhat.com>]

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