Patchwork [V5,04/13] block: add snapshot info query function bdrv_query_snapshot_infolist()

login
register
mail settings
Submitter Wayne Xia
Date Jan. 24, 2013, 2:57 a.m.
Message ID <1358996283-32441-5-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/215190/
State New
Headers show

Comments

Wayne Xia - Jan. 24, 2013, 2:57 a.m.
This patch add function bdrv_query_snapshot_infolist(), which will
return snapshot info of an image in qmp object format. The implementation
code are mostly copied from qemu-img.c with modification to fit more
for qmp based block layer API.
  To help filter out snapshot info not needed, a call back function is
added in bdrv_query_snapshot_infolist().
  bdrv_can_read_snapshot() should be called before call this function,
to avoid got *errp set unexpectedly.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c               |   60 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |    7 +++++
 2 files changed, 67 insertions(+), 0 deletions(-)
Kevin Wolf - Jan. 29, 2013, 12:48 p.m.
Am 24.01.2013 03:57, schrieb Wenchao Xia:
>   This patch add function bdrv_query_snapshot_infolist(), which will
> return snapshot info of an image in qmp object format. The implementation
> code are mostly copied from qemu-img.c with modification to fit more
> for qmp based block layer API.
>   To help filter out snapshot info not needed, a call back function is
> added in bdrv_query_snapshot_infolist().
>   bdrv_can_read_snapshot() should be called before call this function,
> to avoid got *errp set unexpectedly.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c               |   60 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |    7 +++++
>  2 files changed, 67 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 934bb3f..7cdb6c6 100644
> --- a/block.c
> +++ b/block.c
> @@ -2842,6 +2842,66 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
>      return 0;
>  }
>  
> +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
> +                                               SnapshotFilterFunc filter,

This filter things looks overengineered to me, and no patch in this
series really uses it. Do we really need it?


> +                                               void *opaque,
> +                                               Error **errp)
> +{
> +    int i, sn_count;
> +    QEMUSnapshotInfo *sn_tab = NULL;
> +    SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
> +
> +    sn_count = bdrv_snapshot_list(bs, &sn_tab);
> +    if (sn_count < 0) {
> +        /* Now bdrv_snapshot_list() use negative value to tip error, so a check
> +         * of it was done here. In future errp can be set by that function
> +         * itself, by changing the call back functions in c files in ./block.
> +         */
> +        const char *dev = bdrv_get_device_name(bs);
> +        if (sn_count == -ENOMEDIUM) {
> +            error_setg(errp, "Device '%s' is not inserted.", dev);
> +        } else if (sn_count == -ENOTSUP) {
> +            error_setg(errp,
> +                       "Device '%s' does not support internal snapshot.",
> +                       dev);
> +        } else {
> +            error_setg(errp,
> +                       "Device '%s' got %d for bdrv_snapshot_list().",
> +                       dev, sn_count);

strerror(-sn_count) would make the error message more useful.

You could also consider using switch instead of an if/else if chain.

> +        }
> +        return NULL;
> +    }
> +
> +    for (i = 0; i < sn_count; i++) {
> +        if (filter && filter(&sn_tab[i], opaque) != 0) {
> +            continue;
> +        }
> +
> +        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;

Did I already mention somewhere that I'm a fan of compound literals?
Looks so much nicer than repeating info_list->value-> in each line. ;-)

Kevin
Wayne Xia - Jan. 31, 2013, 9:03 a.m.
于 2013-1-29 20:48, Kevin Wolf 写道:
> Am 24.01.2013 03:57, schrieb Wenchao Xia:
>>    This patch add function bdrv_query_snapshot_infolist(), which will
>> return snapshot info of an image in qmp object format. The implementation
>> code are mostly copied from qemu-img.c with modification to fit more
>> for qmp based block layer API.
>>    To help filter out snapshot info not needed, a call back function is
>> added in bdrv_query_snapshot_infolist().
>>    bdrv_can_read_snapshot() should be called before call this function,
>> to avoid got *errp set unexpectedly.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   block.c               |   60 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   include/block/block.h |    7 +++++
>>   2 files changed, 67 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 934bb3f..7cdb6c6 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2842,6 +2842,66 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
>>       return 0;
>>   }
>>
>> +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
>> +                                               SnapshotFilterFunc filter,
>
> This filter things looks overengineered to me, and no patch in this
> series really uses it. Do we really need it?
>
>
   It is used later, which filter out the internal ones not exist
in every block device. I think this provide a clearer way, than
original code in savevm.c which use two "for" to get the correct
snapshots.

>> +                                               void *opaque,
>> +                                               Error **errp)
>> +{
>> +    int i, sn_count;
>> +    QEMUSnapshotInfo *sn_tab = NULL;
>> +    SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
>> +
>> +    sn_count = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (sn_count < 0) {
>> +        /* Now bdrv_snapshot_list() use negative value to tip error, so a check
>> +         * of it was done here. In future errp can be set by that function
>> +         * itself, by changing the call back functions in c files in ./block.
>> +         */
>> +        const char *dev = bdrv_get_device_name(bs);
>> +        if (sn_count == -ENOMEDIUM) {
>> +            error_setg(errp, "Device '%s' is not inserted.", dev);
>> +        } else if (sn_count == -ENOTSUP) {
>> +            error_setg(errp,
>> +                       "Device '%s' does not support internal snapshot.",
>> +                       dev);
>> +        } else {
>> +            error_setg(errp,
>> +                       "Device '%s' got %d for bdrv_snapshot_list().",
>> +                       dev, sn_count);
>
> strerror(-sn_count) would make the error message more useful.
>
> You could also consider using switch instead of an if/else if chain.
>
  OK.

>> +        }
>> +        return NULL;
>> +    }
>> +
>> +    for (i = 0; i < sn_count; i++) {
>> +        if (filter && filter(&sn_tab[i], opaque) != 0) {
>> +            continue;
>> +        }
>> +
>> +        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;
>
> Did I already mention somewhere that I'm a fan of compound literals?
> Looks so much nicer than repeating info_list->value-> in each line. ;-)
>
   Do you mean value=info_list->value and then value->***=***?


> Kevin
>

Patch

diff --git a/block.c b/block.c
index 934bb3f..7cdb6c6 100644
--- a/block.c
+++ b/block.c
@@ -2842,6 +2842,66 @@  int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
+SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
+                                               SnapshotFilterFunc filter,
+                                               void *opaque,
+                                               Error **errp)
+{
+    int i, sn_count;
+    QEMUSnapshotInfo *sn_tab = NULL;
+    SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
+
+    sn_count = bdrv_snapshot_list(bs, &sn_tab);
+    if (sn_count < 0) {
+        /* Now bdrv_snapshot_list() use negative value to tip error, so a check
+         * of it was done here. In future errp can be set by that function
+         * itself, by changing the call back functions in c files in ./block.
+         */
+        const char *dev = bdrv_get_device_name(bs);
+        if (sn_count == -ENOMEDIUM) {
+            error_setg(errp, "Device '%s' is not inserted.", dev);
+        } else if (sn_count == -ENOTSUP) {
+            error_setg(errp,
+                       "Device '%s' does not support internal snapshot.",
+                       dev);
+        } else {
+            error_setg(errp,
+                       "Device '%s' got %d for bdrv_snapshot_list().",
+                       dev, sn_count);
+        }
+        return NULL;
+    }
+
+    for (i = 0; i < sn_count; i++) {
+        if (filter && filter(&sn_tab[i], opaque) != 0) {
+            continue;
+        }
+
+        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;
+
+        /* XXX: waiting for the qapi to support qemu-queue.h types */
+        if (!cur_item) {
+            head = cur_item = info_list;
+        } else {
+            cur_item->next = info_list;
+            cur_item = info_list;
+        }
+
+    }
+
+    g_free(sn_tab);
+    return head;
+}
+
 BlockInfo *bdrv_query_info(BlockDriverState *bs)
 {
     BlockInfo *info = g_malloc0(sizeof(*info));
diff --git a/include/block/block.h b/include/block/block.h
index b4c1612..ce18d4b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -316,6 +316,13 @@  void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
 void bdrv_get_full_backing_filename(BlockDriverState *bs,
                                     char *dest, size_t sz);
+
+typedef int (*SnapshotFilterFunc)(const QEMUSnapshotInfo *sn, void *opaque);
+/* assume bs is already opened, use qapi_free_* to free returned value. */
+SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
+                                               SnapshotFilterFunc filter,
+                                               void *opaque,
+                                               Error **errp);
 BlockInfo *bdrv_query_info(BlockDriverState *s);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
 int bdrv_can_read_snapshot(BlockDriverState *bs);