[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query functi
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list() |
Date: |
Tue, 02 Apr 2013 19:17:35 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 |
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info, Wenchao Xia, 2013/04/02
- [Qemu-devel] [PATCH V11 01/17] block: move bdrv_snapshot_find() to block/snapshot.c, Wenchao Xia, 2013/04/02
- [Qemu-devel] [PATCH V11 03/17] qemu-img: remove unused parameter in collect_image_info(), Wenchao Xia, 2013/04/02
- [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot(), Wenchao Xia, 2013/04/02
- [Qemu-devel] [PATCH V11 04/17] block: move collect_snapshots() and collect_image_info() to block/qapi.c, Wenchao Xia, 2013/04/02
- [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list(), Wenchao Xia, 2013/04/02
- Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list(),
Eric Blake <=
- [Qemu-devel] [PATCH V11 06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list(), Wenchao Xia, 2013/04/02
- [Qemu-devel] [PATCH V11 08/17] block: move qmp_query_block() and bdrv_query_info() to block/qapi.c, Wenchao Xia, 2013/04/02
- [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots, Wenchao Xia, 2013/04/02