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

[Kevin Wolf]

Am 22.03.2013 um 15:19 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 <address@hidden>
> ---
>  block/qapi.c         |   39 ++++++++++++++++++++++++++--
>  include/block/qapi.h |    4 ++-
>  qapi-schema.json     |    5 +++-
>  qmp-commands.hx      |   67 
> +++++++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 109 insertions(+), 6 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index df73f5b..9051947 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -200,9 +200,15 @@ int bdrv_query_image_info(BlockDriverState *bs,
>      return 0;
>  }
>  
> -BlockInfo *bdrv_query_info(BlockDriverState *bs)
> +/* return 0 on success, and @p_info will be set only on success. */
> +int bdrv_query_info(BlockDriverState *bs,
> +                    BlockInfo **p_info,
> +                    Error **errp)
>  {
>      BlockInfo *info = g_malloc0(sizeof(*info));
> +    BlockDriverState *bs0;
> +    ImageInfo **p_image_info;
> +    int ret = 0;

ret is never changed, so this function always returns 0. I would suggest
to drop ret and make the function return type void.

>      info->device = g_strdup(bs->device_name);
>      info->type = g_strdup("unknown");
>      info->locked = bdrv_dev_is_medium_locked(bs);
> @@ -256,8 +262,29 @@ 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 0;
> +
> + err:
> +    qapi_free_BlockInfo(info);
> +    return ret;
>  }
>  
>  SnapshotInfoList *qmp_query_snapshots(Error **errp)
> @@ -284,11 +311,17 @@ BlockInfoList *qmp_query_block(Error **errp)
>  
>      while ((bs = bdrv_next(bs))) {
>          BlockInfoList *info = g_malloc0(sizeof(*info));
> -        info->value = bdrv_query_info(bs);
> +        if (bdrv_query_info(bs, &info->value, errp)) {
> +            goto err;
> +        }

Consequently, you've got the error handling wrong here, the if condition
is never true. It should look more or less like this (the syntax details
may be wrong):

Error *local_err;
bdrv_query_info(bs, &info->value, &local_err);
if (error_is_set(local_err)) {
    error_propagate(err, local_err);
    goto err;
}

Kevin