Re: [Qemu-devel] [PATCH V7 06/14] block: add image info query function bdrv_query_image_info()

[Markus Armbruster]

Wenchao Xia <address@hidden> writes:

>   This patch add function bdrv_query_image_info(), which will return
> image info in qmp object format. The implementation code are based
> on the code moved from qemu-img.c, but use block layer function to get
> snapshot info.
>   A check with bdrv_can_read_snapshot(), was done before collecting
> snapshot info.
>
> Signed-off-by: Wenchao Xia <address@hidden>
> ---
>  block.c               |   38 ++++++++++++++++++++++++++++++++------
>  include/block/block.h |    5 +----
>  qemu-img.c            |   13 +++++--------
>  3 files changed, 38 insertions(+), 18 deletions(-)
>
> diff --git a/block.c b/block.c
> index 8d0145a..71fc9e7 100644
> --- a/block.c
> +++ b/block.c
> @@ -2880,15 +2880,33 @@ SnapshotInfoList 
> *bdrv_query_snapshot_infolist(BlockDriverState *bs,
>      return head;
>  }
>  
> -void collect_image_info(BlockDriverState *bs,
> -                        ImageInfo *info,
> -                        const char *filename)
> +/* collect all internal snapshot info in a image for ImageInfo */
> +static void collect_snapshots_info(BlockDriverState *bs,
> +                                   ImageInfo *info,
> +                                   Error **errp)
> +{
> +    SnapshotInfoList *info_list;
> +
> +    if (!bdrv_can_read_snapshot(bs)) {
> +        return;
> +    }
> +    info_list = bdrv_query_snapshot_infolist(bs, NULL, NULL, errp);

Suggest to store straight into info_list->snapshots, like you did in the
previous patch.

> +    if (info_list != NULL) {
> +        info->has_snapshots = true;
> +        info->snapshots = info_list;
> +    }
> +}
> +
> +static void collect_image_info(BlockDriverState *bs,
> +                               ImageInfo *info)
>  {
>      uint64_t total_sectors;
> -    char backing_filename[1024];
> +    const char *backing_filename;
>      char backing_filename2[1024];
>      BlockDriverInfo bdi;
> +    const char *filename;
>  
> +    filename = bs->filename;
>      bdrv_get_geometry(bs, &total_sectors);
>  
>      info->filename        = g_strdup(filename);
> @@ -2908,8 +2926,8 @@ void 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') {
>          info->backing_filename = g_strdup(backing_filename);
>          info->has_backing_filename = true;
>          bdrv_get_full_backing_filename(bs, backing_filename2,
> @@ -2928,6 +2946,14 @@ void collect_image_info(BlockDriverState *bs,
>      }
>  }
>  
> +ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp)
> +{
> +    ImageInfo *info = g_new0(ImageInfo, 1);
> +    collect_image_info(bs, info);
> +    collect_snapshots_info(bs, info, errp);
> +    return info;
> +}

Please return NULL on error.

> +
>  BlockInfo *bdrv_query_info(BlockDriverState *bs)
>  {
>      BlockInfo *info = g_malloc0(sizeof(*info));
> diff --git a/include/block/block.h b/include/block/block.h
> index 51a14f2..f033807 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -326,6 +326,7 @@ SnapshotInfoList 
> *bdrv_query_snapshot_infolist(BlockDriverState *bs,
>                                                 SnapshotFilterFunc filter,
>                                                 void *opaque,
>                                                 Error **errp);
> +ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp);
>  BlockInfo *bdrv_query_info(BlockDriverState *s);
>  BlockStats *bdrv_query_stats(const BlockDriverState *bs);
>  bool bdrv_can_read_snapshot(BlockDriverState *bs);
> @@ -456,8 +457,4 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const 
> char *event,
>                             const char *tag);
>  int bdrv_debug_resume(BlockDriverState *bs, const char *tag);
>  bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag);
> -
> -void collect_image_info(BlockDriverState *bs,
> -                        ImageInfo *info,
> -                        const char *filename);
>  #endif
> diff --git a/qemu-img.c b/qemu-img.c
> index 1034cc5..90f4bf4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1257,6 +1257,7 @@ static ImageInfoList *collect_image_info_list(const 
> char *filename,
>      ImageInfoList *head = NULL;
>      ImageInfoList **last = &head;
>      GHashTable *filenames;
> +    Error *err = NULL;
>  
>      filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, 
> NULL);
>  
> @@ -1278,14 +1279,10 @@ static ImageInfoList *collect_image_info_list(const 
> char *filename,
>              goto err;
>          }
>  
> -        info = g_new0(ImageInfo, 1);
> -        collect_image_info(bs, info, filename);
> -        if (bdrv_can_read_snapshot(bs)) {
> -            info->snapshots = bdrv_query_snapshot_infolist(bs, NULL,
> -                                                           NULL, NULL);
> -            if (info->snapshots) {
> -                info->has_snapshots = true;
> -            }
> +        info = bdrv_query_image_info(bs, &err);
> +        if (error_is_set(&err)) {
> +            bdrv_delete(bs);
> +            goto err;

Leaks info.  Easy error to make, because returning non-null on error is
rather surprising.  That's why I want you to return NULL then.

And then this can be simplified to

           info = bdrv_query_image_info(bs, NULL);
           if (!info) {

>          }
>  
>          elem = g_new0(ImageInfoList, 1);