Re: [Qemu-devel] [PATCH V11 07/17] block: add image info query function bdrv_query_image_info()

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

>   This patch adds function bdrv_query_image_info(), which will
> retrieve image info in qmp object format. The implementation is
> based on the code moved from qemu-img.c, but uses block layer
> function to get snapshot info.
>
> Signed-off-by: Wenchao Xia <address@hidden>
> ---
>  block/qapi.c         |   41 ++++++++++++++++++++++++++++++++++-------
>  include/block/qapi.h |    6 +++---
>  qemu-img.c           |    8 ++------
>  3 files changed, 39 insertions(+), 16 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 19d4d93..176a479 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -122,18 +122,22 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>      return 0;
>  }
>  
> -void bdrv_collect_image_info(BlockDriverState *bs,
> -                             ImageInfo *info,
> -                             const char *filename)
> +/* return 0 on success, and @p_info will be set only on success. */
> +int bdrv_query_image_info(BlockDriverState *bs,
> +                          ImageInfo **p_info,
> +                          Error **errp)
>  {
>      uint64_t total_sectors;
> -    char backing_filename[1024];
> +    const char *backing_filename;
>      char backing_filename2[1024];
>      BlockDriverInfo bdi;
> +    int ret;
> +    Error *err = NULL;
> +    ImageInfo *info = g_new0(ImageInfo, 1);
>  
>      bdrv_get_geometry(bs, &total_sectors);
>  
> -    info->filename        = g_strdup(filename);
> +    info->filename        = g_strdup(bs->filename);
>      info->format          = g_strdup(bdrv_get_format_name(bs));
>      info->virtual_size    = total_sectors * 512;
>      info->actual_size     = bdrv_get_allocated_file_size(bs);
> @@ -150,8 +154,8 @@ void bdrv_collect_image_info(BlockDriverState *bs,
>          info->dirty_flag = bdi.is_dirty;
>          info->has_dirty_flag = true;
>      }
> -    bdrv_get_backing_filename(bs, backing_filename, 
> sizeof(backing_filename));
> -    if (backing_filename[0] != '\0') {
> +    backing_filename = bs->backing_file;
> +    if (backing_filename && backing_filename[0] != '\0') {

backing_filename can't possibly be null here, because bs->backing_file
is an array.

>          info->backing_filename = g_strdup(backing_filename);
>          info->has_backing_filename = true;
>          bdrv_get_full_backing_filename(bs, backing_filename2,
> @@ -168,4 +172,27 @@ void bdrv_collect_image_info(BlockDriverState *bs,
>              info->has_backing_filename_format = true;
>          }
>      }
> +
> +    ret = bdrv_query_snapshot_info_list(bs, &info->snapshots, false, &err);
> +    switch (ret) {
> +    case 0:
> +        if (info->snapshots) {
> +            info->has_snapshots = true;
> +        }
> +        break;
> +    /* recoverable error */
> +    case -ENOMEDIUM:
> +        error_free(err);
> +        break;
> +    case -ENOTSUP:
> +        error_free(err);
> +        break;

Suggest

       case -ENOMEDIUM:
       case -ENOTSUP:
           /* no snapshots on this device */
           error_free(err);
           break;

for clarity.

> +    default:
> +        error_propagate(errp, err);
> +        qapi_free_ImageInfo(info);
> +        return ret;
> +    }
> +
> +    *p_info = info;
> +    return 0;
>  }
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index fe66053..2c62fdf 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -32,7 +32,7 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>                                    SnapshotInfoList **p_list,
>                                    bool vm_snapshot,
>                                    Error **errp);
> -void bdrv_collect_image_info(BlockDriverState *bs,
> -                             ImageInfo *info,
> -                             const char *filename);
> +int bdrv_query_image_info(BlockDriverState *bs,
> +                          ImageInfo **p_info,
> +                          Error **errp);
>  #endif
> diff --git a/qemu-img.c b/qemu-img.c
> index 261c277..1dd0a60 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1733,12 +1733,8 @@ static ImageInfoList *collect_image_info_list(const 
> char *filename,
>              goto err;
>          }
>  
> -        info = g_new0(ImageInfo, 1);
> -        bdrv_collect_image_info(bs, info, filename);
> -        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
> -                                           false, NULL) &&
> -            info->snapshots) {
> -            info->has_snapshots = true;
> +        if (bdrv_query_image_info(bs, &info, NULL)) {
> +            goto err;
>          }
>  
>          elem = g_new0(ImageInfoList, 1);