Patchwork [04/11] qemu-img: move image retrieving function to block layer

login
register
mail settings
Submitter Wayne Xia
Date Dec. 29, 2012, 8:45 a.m.
Message ID <1356770725-4804-5-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/208623/
State New
Headers show

Comments

Wayne Xia - Dec. 29, 2012, 8:45 a.m.
This patch moves collect_image_info() and collect_snapshot()
to general block layer and encapsulate them as bdrv_query_image_info()
and bdrv_query_snapshot_infolist(), as mirror function to brdv_query_info().
The called function in qemu-img.c is switched to bdrv_query_image_info().
To help filter out snapshot info not needed, a call back function is
added in bdrv_query_snapshot_infolist().

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |  118 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |    9 ++++
 qemu-img.c            |   88 +-----------------------------------
 3 files changed, 129 insertions(+), 86 deletions(-)
Eric Blake - Jan. 4, 2013, 5:02 p.m.
On 12/29/2012 01:45 AM, Wenchao Xia wrote:
>   This patch moves collect_image_info() and collect_snapshot()
> to general block layer and encapsulate them as bdrv_query_image_info()
> and bdrv_query_snapshot_infolist(), as mirror function to brdv_query_info().
> The called function in qemu-img.c is switched to bdrv_query_image_info().
> To help filter out snapshot info not needed, a call back function is
> added in bdrv_query_snapshot_infolist().
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block.c               |  118 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |    9 ++++
>  qemu-img.c            |   88 +-----------------------------------
>  3 files changed, 129 insertions(+), 86 deletions(-)
> 

> +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) {
> +        if (errp != NULL) {
> +            error_setg(errp, "bdrv_snapshot_list: error %d\n", sn_count);
> +        }
> +        return NULL;
> +    }
> +
> +    for (i = 0; i < sn_count; i++) {
> +        if ((filter != NULL) && (filter(&sn_tab[i], opaque) != 0)) {

My brief glance through qemu sources makes me think you
over-parenthesized this, and that:

if (filter && filter(&sn_tab[i], opaque) != 0) {

would be sufficient.


> +    bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
> +    if (backing_filename[0] != '\0') {
> +        info->backing_filename = g_strdup(backing_filename);
> +        info->has_backing_filename = true;
> +        bdrv_get_full_backing_filename(bs, backing_filename2,
> +                                       sizeof(backing_filename2));
> +
> +        if (strcmp(backing_filename, backing_filename2) != 0) {
> +            info->full_backing_filename =
> +                        g_strdup(backing_filename2);

I know it's just code motion, but why the line wrap here?

> +++ b/include/block/block.h
> @@ -316,8 +316,17 @@ 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 openned, use g_free to free returned value. */

s/openned/opened/

Patch

diff --git a/block.c b/block.c
index 5f95da5..d39da3d 100644
--- a/block.c
+++ b/block.c
@@ -2846,6 +2846,124 @@  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) {
+        if (errp != NULL) {
+            error_setg(errp, "bdrv_snapshot_list: error %d\n", sn_count);
+        }
+        return NULL;
+    }
+
+    for (i = 0; i < sn_count; i++) {
+        if ((filter != NULL) && (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;
+}
+
+/* collect all internal snapshot info in a image for ImageInfo */
+static void collect_snapshots_info(BlockDriverState *bs,
+                                   ImageInfo *info,
+                                   Error **errp)
+{
+    SnapshotInfoList *info_list;
+
+    info_list = bdrv_query_snapshot_infolist(bs, NULL, NULL, errp);
+    if (info_list != NULL) {
+        info->has_snapshots = true;
+        info->snapshots = info_list;
+    }
+    return;
+}
+
+static void collect_image_info(BlockDriverState *bs,
+                               ImageInfo *info)
+{
+    uint64_t total_sectors;
+    char backing_filename[1024];
+    char backing_filename2[1024];
+    BlockDriverInfo bdi;
+    const char *filename;
+
+    filename = bdrv_get_filename(bs);
+    bdrv_get_geometry(bs, &total_sectors);
+
+    info->filename        = g_strdup(filename);
+    info->format          = g_strdup(bdrv_get_format_name(bs));
+    info->virtual_size    = total_sectors * 512;
+    info->actual_size     = bdrv_get_allocated_file_size(bs);
+    info->has_actual_size = info->actual_size >= 0;
+    if (bdrv_is_encrypted(bs)) {
+        info->encrypted = true;
+        info->has_encrypted = true;
+    }
+    if (bdrv_get_info(bs, &bdi) >= 0) {
+        if (bdi.cluster_size != 0) {
+            info->cluster_size = bdi.cluster_size;
+            info->has_cluster_size = true;
+        }
+        info->dirty_flag = bdi.is_dirty;
+        info->has_dirty_flag = true;
+    }
+    bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
+    if (backing_filename[0] != '\0') {
+        info->backing_filename = g_strdup(backing_filename);
+        info->has_backing_filename = true;
+        bdrv_get_full_backing_filename(bs, backing_filename2,
+                                       sizeof(backing_filename2));
+
+        if (strcmp(backing_filename, backing_filename2) != 0) {
+            info->full_backing_filename =
+                        g_strdup(backing_filename2);
+            info->has_full_backing_filename = true;
+        }
+
+        if (bs->backing_format[0]) {
+            info->backing_filename_format = g_strdup(bs->backing_format);
+            info->has_backing_filename_format = true;
+        }
+    }
+}
+
+ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp)
+{
+    ImageInfo *info = g_new0(ImageInfo, 1);
+    collect_image_info(bs, info);
+    collect_snapshots_info(bs, info, errp);
+    return info;
+}
+
 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 cf20257..33d7b7c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -316,8 +316,17 @@  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 openned, use g_free to free returned value. */
+SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
+                                               SnapshotFilterFunc filter,
+                                               void *opaque,
+                                               Error **errp);
+ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp);
 BlockInfo *bdrv_query_info(BlockDriverState *s);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
+
 int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);
diff --git a/qemu-img.c b/qemu-img.c
index d70435f..6a62c68 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1130,39 +1130,6 @@  static void dump_json_image_info_list(ImageInfoList *list)
     QDECREF(str);
 }
 
-static void collect_snapshots(BlockDriverState *bs , ImageInfo *info)
-{
-    int i, sn_count;
-    QEMUSnapshotInfo *sn_tab = NULL;
-    SnapshotInfoList *info_list, *cur_item = NULL;
-    sn_count = bdrv_snapshot_list(bs, &sn_tab);
-
-    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;
-
-        /* XXX: waiting for the qapi to support qemu-queue.h types */
-        if (!cur_item) {
-            info->snapshots = cur_item = info_list;
-        } else {
-            cur_item->next = info_list;
-            cur_item = info_list;
-        }
-
-    }
-
-    g_free(sn_tab);
-}
-
 static void dump_json_image_info(ImageInfo *info)
 {
     Error *errp = NULL;
@@ -1180,56 +1147,6 @@  static void dump_json_image_info(ImageInfo *info)
     QDECREF(str);
 }
 
-/* Assume bs is already openned. */
-static void collect_image_info(BlockDriverState *bs,
-                               ImageInfo *info)
-{
-    uint64_t total_sectors;
-    char backing_filename[1024];
-    char backing_filename2[1024];
-    BlockDriverInfo bdi;
-    const char *filename;
-
-    filename = bdrv_get_filename(bs);
-    bdrv_get_geometry(bs, &total_sectors);
-
-    info->filename        = g_strdup(filename);
-    info->format          = g_strdup(bdrv_get_format_name(bs));
-    info->virtual_size    = total_sectors * 512;
-    info->actual_size     = bdrv_get_allocated_file_size(bs);
-    info->has_actual_size = info->actual_size >= 0;
-    if (bdrv_is_encrypted(bs)) {
-        info->encrypted = true;
-        info->has_encrypted = true;
-    }
-    if (bdrv_get_info(bs, &bdi) >= 0) {
-        if (bdi.cluster_size != 0) {
-            info->cluster_size = bdi.cluster_size;
-            info->has_cluster_size = true;
-        }
-        info->dirty_flag = bdi.is_dirty;
-        info->has_dirty_flag = true;
-    }
-    bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
-    if (backing_filename[0] != '\0') {
-        info->backing_filename = g_strdup(backing_filename);
-        info->has_backing_filename = true;
-        bdrv_get_full_backing_filename(bs, backing_filename2,
-                                       sizeof(backing_filename2));
-
-        if (strcmp(backing_filename, backing_filename2) != 0) {
-            info->full_backing_filename =
-                        g_strdup(backing_filename2);
-            info->has_full_backing_filename = true;
-        }
-
-        if (bs->backing_format[0]) {
-            info->backing_filename_format = g_strdup(bs->backing_format);
-            info->has_backing_filename_format = true;
-        }
-    }
-}
-
 static void dump_human_image_info(ImageInfo *info)
 {
     char size_buf[128], dsize_buf[128];
@@ -1336,6 +1253,7 @@  static ImageInfoList *collect_image_info_list(const char *filename,
     ImageInfoList *head = NULL;
     ImageInfoList **last = &head;
     GHashTable *filenames;
+    Error *err = NULL;
 
     filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
 
@@ -1357,9 +1275,7 @@  static ImageInfoList *collect_image_info_list(const char *filename,
             goto err;
         }
 
-        info = g_new0(ImageInfo, 1);
-        collect_image_info(bs, info);
-        collect_snapshots(bs, info);
+        info = bdrv_query_image_info(bs, &err);
 
         elem = g_new0(ImageInfoList, 1);
         elem->value = info;