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

[Wenchao Xia]

于 2013-3-28 17:54, 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.
  My bad, I forgot to set its value, the interface is intend to return
negative error number and errp both on fail.

> >       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;
> > +            }
  Sorry ret is missing here, it should be:
            ret = bdrv_query_image_info(bs0, p_image_info, errp));
            if (ret) {
                goto err;
            }
  I'll correct it.

> > +            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


--
Best Regards

Wenchao Xia