Patchwork [V11,05/17] block: add snapshot info query function bdrv_query_snapshot_info_list()

login
register
mail settings
Submitter Wayne Xia
Date April 2, 2013, 11:47 a.m.
Message ID <1364903250-10429-6-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/232974/
State New
Headers show

Comments

Wayne Xia - April 2, 2013, 11:47 a.m.
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 <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c         |   55 ++++++++++++++++++++++++++++++++++++++-----------
 include/block/qapi.h |    4 ++-
 qemu-img.c           |    5 +++-
 3 files changed, 49 insertions(+), 15 deletions(-)
Eric Blake - April 3, 2013, 1:17 a.m.
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 <xiawenc@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.

> + */
> +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 <eblake@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.

But I don't know if any later patches in the series start to care about
which error was returned.
Wayne Xia - April 3, 2013, 5:24 a.m.
于 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 <xiawenc@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.
>
>> + */
>> +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 <eblake@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.
>
> 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.
Markus Armbruster - April 10, 2013, 3:11 p.m.
Eric Blake <eblake@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 <xiawenc@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?

>
>> + */
>> +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 <eblake@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 :)
Wayne Xia - April 11, 2013, 3:19 a.m.
于 2013-4-10 23:11, Markus Armbruster 写道:
> Eric Blake <eblake@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 <xiawenc@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 <eblake@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 :)
>

Patch

diff --git a/block/qapi.c b/block/qapi.c
index e2b1b6b..03369a5 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -25,29 +25,56 @@ 
 #include "block/qapi.h"
 #include "block/block_int.h"
 
-void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info)
+/*
+ * return 0 on success, @p_list will be set only on success, and caller need to
+ * check *p_list on success.
+ */
+int bdrv_query_snapshot_info_list(BlockDriverState *bs,
+                                  SnapshotInfoList **p_list,
+                                  Error **errp)
 {
     int i, sn_count;
     QEMUSnapshotInfo *sn_tab = NULL;
-    SnapshotInfoList *info_list, *cur_item = NULL;
+    SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
+    SnapshotInfo *info;
+
     sn_count = bdrv_snapshot_list(bs, &sn_tab);
+    if (sn_count < 0) {
+        const char *dev = bdrv_get_device_name(bs);
+        switch (sn_count) {
+        case -ENOMEDIUM:
+            error_setg(errp, "Device '%s' is not inserted", dev);
+            break;
+        case -ENOTSUP:
+            error_setg(errp,
+                       "Device '%s' does not support internal snapshots",
+                       dev);
+            break;
+        default:
+            error_setg_errno(errp, -sn_count,
+                             "Can't list snapshots of device '%s'", dev);
+            break;
+        }
+        return sn_count;
+    }
 
     for (i = 0; i < sn_count; i++) {
-        info->has_snapshots = true;
-        info_list = g_new0(SnapshotInfoList, 1);
 
-        info_list->value                = g_new0(SnapshotInfo, 1);
-        info_list->value->id            = g_strdup(sn_tab[i].id_str);
-        info_list->value->name          = g_strdup(sn_tab[i].name);
-        info_list->value->vm_state_size = sn_tab[i].vm_state_size;
-        info_list->value->date_sec      = sn_tab[i].date_sec;
-        info_list->value->date_nsec     = sn_tab[i].date_nsec;
-        info_list->value->vm_clock_sec  = sn_tab[i].vm_clock_nsec / 1000000000;
-        info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000;
+        info = g_new0(SnapshotInfo, 1);
+        info->id            = g_strdup(sn_tab[i].id_str);
+        info->name          = g_strdup(sn_tab[i].name);
+        info->vm_state_size = sn_tab[i].vm_state_size;
+        info->date_sec      = sn_tab[i].date_sec;
+        info->date_nsec     = sn_tab[i].date_nsec;
+        info->vm_clock_sec  = sn_tab[i].vm_clock_nsec / 1000000000;
+        info->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000;
+
+        info_list = g_new0(SnapshotInfoList, 1);
+        info_list->value = info;
 
         /* XXX: waiting for the qapi to support qemu-queue.h types */
         if (!cur_item) {
-            info->snapshots = cur_item = info_list;
+            head = cur_item = info_list;
         } else {
             cur_item->next = info_list;
             cur_item = info_list;
@@ -56,6 +83,8 @@  void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info)
     }
 
     g_free(sn_tab);
+    *p_list = head;
+    return 0;
 }
 
 void bdrv_collect_image_info(BlockDriverState *bs,
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 4586578..91dc41b 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -28,7 +28,9 @@ 
 #include "qapi-types.h"
 #include "block/block.h"
 
-void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info);
+int bdrv_query_snapshot_info_list(BlockDriverState *bs,
+                                  SnapshotInfoList **p_list,
+                                  Error **errp);
 void bdrv_collect_image_info(BlockDriverState *bs,
                              ImageInfo *info,
                              const char *filename);
diff --git a/qemu-img.c b/qemu-img.c
index a020ccc..51043ef 100644
--- a/qemu-img.c
+++ 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;
+        }
 
         elem = g_new0(ImageInfoList, 1);
         elem->value = info;