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

[Wenchao Xia]

于 2013-4-3 9:17, Eric Blake 写道:
> 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 <address@hidden>
> > ---
> >   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.
>
> > + */
> > +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 <address@hidden>
>
> > +++ 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.
>
> But I don't know if any later patches in the series start to care about
> which error was returned.
  Yes it would be cared in ImageInfo retrieving later. This section in
qemu-img.c would be replaced later so did not check strictly the
returned value.


--
Best Regards

Wenchao Xia