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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 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 <xiawenc@linux.vnet.ibm.com>
>>>> ---
>>>>   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