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

[Wenchao Xia]

于 2013-4-2 16:09, Kevin Wolf 写道:
> Am 29.03.2013 um 03:35 hat Wenchao Xia geschrieben:
> > 于 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.
>
> Why do you need two separate error reporting mechanisms? Shouldn't only
> errp be enough?
>
> Kevin
  Returned value can tell caller what error it is, like -ENOMEDIUM.
Although it is not used by caller now, but I feel better to have it
just like

+int bdrv_query_snapshot_info_list(BlockDriverState *bs,
+                                  SnapshotInfoList **p_list,
+                                  Error **errp)

in patch 5/17.

--
Best Regards

Wenchao Xia