[Top][All Lists]

[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: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list()
Date: Wed, 03 Apr 2013 13:24:07 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130307 Thunderbird/17.0.4

于 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


+ * 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 

          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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]