Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block

[Kevin Wolf <kwolf@redhat.com>]

Am 02.04.2013 um 13:47 hat Wenchao Xia geschrieben:
>   Now image info will be retrieved as an embbed json object inside
> BlockDeviceInfo, backing chain info and all related internal snapshot
> info can be got in the enhanced recursive structure of ImageInfo.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qapi.c         |   38 ++++++++++++++++++++++++++--
>  include/block/qapi.h |    4 ++-
>  qapi-schema.json     |    5 +++-
>  qmp-commands.hx      |   67 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 108 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index fa61c85..e98a028 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -202,9 +202,14 @@ int bdrv_query_image_info(BlockDriverState *bs,
>      return 0;
>  }
>  
> -BlockInfo *bdrv_query_info(BlockDriverState *bs)
> +/* @p_info will be set only on success. */
> +void bdrv_query_info(BlockDriverState *bs,
> +                     BlockInfo **p_info,
> +                     Error **errp)
>  {
>      BlockInfo *info = g_malloc0(sizeof(*info));
> +    BlockDriverState *bs0;
> +    ImageInfo **p_image_info;
>      info->device = g_strdup(bs->device_name);
>      info->type = g_strdup("unknown");
>      info->locked = bdrv_dev_is_medium_locked(bs);
> @@ -258,8 +263,28 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs)
>              info->inserted->iops_wr =
>                             bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
>          }
> +
> +        bs0 = bs;
> +        p_image_info = &info->inserted->image;
> +        while (1) {
> +            if (bdrv_query_image_info(bs0, p_image_info, errp)) {
> +                goto err;
> +            }
> +            if (bs0->drv && bs0->backing_hd) {
> +                bs0 = bs0->backing_hd;
> +                (*p_image_info)->has_backing_image = true;
> +                p_image_info = &((*p_image_info)->backing_image);
> +            } else {
> +                break;
> +            }
> +        }
>      }
> -    return info;
> +
> +    *p_info = info;
> +    return;
> +
> + err:
> +    qapi_free_BlockInfo(info);
>  }
>  
>  SnapshotInfoList *qmp_query_snapshots(Error **errp)
> @@ -286,11 +311,18 @@ BlockInfoList *qmp_query_block(Error **errp)
>  
>      while ((bs = bdrv_next(bs))) {
>          BlockInfoList *info = g_malloc0(sizeof(*info));
> -        info->value = bdrv_query_info(bs);
> +        bdrv_query_info(bs, &info->value, errp);
> +        if (error_is_set(errp)) {
> +            goto err;
> +        }

This doesn't do what you think it does: The caller can pass NULL as
errp, and then this becomes never true. I think you need to use a
local_err and propagate it to errp in error cases.

>  
>          *p_next = info;
>          p_next = &info->next;
>      }
>  
>      return head;
> +
> + err:
> +    qapi_free_BlockInfoList(head);
> +    return NULL;
>  }

Kevin