Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list()

[Wenchao Xia]

于 2013-4-10 23:11, Markus Armbruster 写道:
> Eric Blake <ebl...@redhat.com> writes:
> 
>> On 04/02/2013 05:47 AM, Wenchao Xia wrote:
>>>    This patch adds function bdrv_query_snapshot_info_list(), which will
>>> retrieve snapshot info of an image in qmp object format. The implementation
>>> is based on the code moved from qemu-img.c with modification to fit more
>>> for qmp based block layer API.
>>>
>>> Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com>
>>> ---
>>>   block/qapi.c         |   55 
>>> ++++++++++++++++++++++++++++++++++++++-----------
>>>   include/block/qapi.h |    4 ++-
>>>   qemu-img.c           |    5 +++-
>>>   3 files changed, 49 insertions(+), 15 deletions(-)
>>
>>> +/*
>>> + * return 0 on success, @p_list will be set only on success, and caller 
>>> need to
>>
>> s/need/needs/
>>
>>> + * check *p_list on success.
>>
>> I wonder if this wording would be any better:
>>
>> Returns 0 on success, with *p_list either set to describe snapshot
>> information, or NULL because there are no snapshots.  Returns -1 on
>> error, with *p_list untouched.
> 
> It actually returns -errno then, doesn't it?
> 
  Yes, I forgot change the comments in the version changing, thank you
for the carefully review.

>>
>>> + */
>>> +int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>>> +                                  SnapshotInfoList **p_list,
>>> +                                  Error **errp)
>>>   {
>>
>> At any rate, my only commentary was on grammar and a possible wording
>> for a comment, while the code itself is fine from my viewpoint; so feel
>> free to add:
>>
>> Reviewed-by: Eric Blake <ebl...@redhat.com>
>>
>>> +++ b/qemu-img.c
>>> @@ -1735,7 +1735,10 @@ static ImageInfoList
>>> *collect_image_info_list(const char *filename,
>>>   
>>>           info = g_new0(ImageInfo, 1);
>>>           bdrv_collect_image_info(bs, info, filename);
>>> -        bdrv_collect_snapshots(bs, info);
>>> +        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
>>> +            info->snapshots) {
>>> +            info->has_snapshots = true;
>>> +        }
>>
>> Hmm.  info->snapshots starts life as NULL (thanks to g_new0), and is
>> untouched on error.  Since you are ignoring any errors, you technically
>> could write:
>>
>> bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL);
>> if (info->snapshots) {
>>      info->has_snapshots = true;
>> }
>>
>> for the same semantics.  That means that as of this commit, no caller
>> cares about the return value of bdrv_query_snapshot_info_list (they only
>> care about whether info->snapshots was changed to non-null), so it could
>> return void for a slightly simpler implementation.
> 
> Return the list, or NULL.
> 
>> But I don't know if any later patches in the series start to care about
>> which error was returned.
> 
> Me neither :)
> 


-- 
Best Regards

Wenchao Xia