Re: [Qemu-devel] [PATCH] vmdk: implment bdrv_get_info and bdrv_get_specific_info

[Kevin Wolf <kwolf@redhat.com>]

Am 10.10.2013 um 09:07 hat Fam Zheng geschrieben:
> .bdrv_get_info reports cluster_size if it's a monolithic image.
> 
> .bdrv_get_specific_info reports the image version (if applicable) and
> extent file name list.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>

Would it be useful to include the subformat as well?

> diff --git a/block/vmdk.c b/block/vmdk.c
> index 5d56e31..ff9bdac 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1814,6 +1814,48 @@ static int vmdk_has_zero_init(BlockDriverState *bs)
>      return 1;
>  }
>  
> +static int vmdk_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
> +{
> +    BDRVVmdkState *s = bs->opaque;
> +    /* Normally the cluster sizes for all the extents in a vmdk image are the
> +     * same, but we don't bother to check for this here and only report the
> +     * value for the monolithic case. */
> +    if (s->num_extents == 1 && !s->extents[0].flat) {
> +        bdi->cluster_size = s->extents[0].cluster_sectors * 512;
> +    }
> +    return 0;
> +}
> +
> +static ImageInfoSpecific *vmdk_get_specific_info(BlockDriverState *bs)
> +{
> +    int i;
> +    BDRVVmdkState *s = bs->opaque;
> +    ImageInfoSpecific *spec_info = g_new(ImageInfoSpecific, 1);
> +    strList **next;
> +
> +    *spec_info = (ImageInfoSpecific){
> +        .kind  = IMAGE_INFO_SPECIFIC_KIND_VMDK,
> +        .vmdk = g_new(ImageInfoSpecificVmdk, 1),
> +    };

The first line has different spacing than the second one, so that the
'=' signs aren't aligned to the same column. Probably not intentional?

> +
> +    next = &spec_info->vmdk->extents;
> +    for (i = 0; i < s->num_extents; i++) {
> +        *next = g_new(strList, 1);
> +        **next = (strList){
> +            .value = g_strdup(s->extents[i].file->filename),
> +            .next = NULL,
> +        };
> +        next = &(*next)->next;
> +    }
> +
> +    if (s->num_extents == 1) {
> +        spec_info->vmdk->version = s->extents[0].version;
> +        spec_info->vmdk->has_version = true;
> +    }
> +
> +    return spec_info;
> +}
> +
>  static QEMUOptionParameter vmdk_create_options[] = {
>      {
>          .name = BLOCK_OPT_SIZE,
> @@ -1866,6 +1908,8 @@ static BlockDriver bdrv_vmdk = {
>      .bdrv_co_get_block_status     = vmdk_co_get_block_status,
>      .bdrv_get_allocated_file_size = vmdk_get_allocated_file_size,
>      .bdrv_has_zero_init           = vmdk_has_zero_init,
> +    .bdrv_get_info                = vmdk_get_info,
> +    .bdrv_get_specific_info       = vmdk_get_specific_info,
>  
>      .create_options               = vmdk_create_options,
>  };
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a1a81a4..b1e74b3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -225,6 +225,17 @@
>    } }
>  
>  ##
> +# @ImageInfoSpecificVmdk:
> +#
> +# Since: 1.7
> +##
> +{ 'type': 'ImageInfoSpecificVmdk',
> +  'data': {
> +      '*version': 'int',
> +      'extents': ['str']
> +  } }

Is the file name really the only relevant information about an extent?

Above it looks like each extent has its version, and it also has its
own subformat type, so perhaps making it a struct would make sense.

> +##
>  # @ImageInfoSpecific:
>  #
>  # A discriminated record of image format specific information structures.
> @@ -234,7 +245,8 @@
>  
>  { 'union': 'ImageInfoSpecific',
>    'data': {
> -      'qcow2': 'ImageInfoSpecificQCow2'
> +      'qcow2': 'ImageInfoSpecificQCow2',
> +      'vmdk': 'ImageInfoSpecificVmdk'
>    } }

Kevin