diff mbox

[V8,06/20] block: move collect_snapshots() and collect_image_info() to block/qapi.c

Message ID 1362636445-7188-7-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

Wayne Xia March 7, 2013, 6:07 a.m. UTC 
This patch is just for making review easier, those two functions will
be modified and renamed later.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c         |   82 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/qapi.h |    8 +++++
 qemu-img.c           |   86 ++------------------------------------------------
 3 files changed, 93 insertions(+), 83 deletions(-)

Comments

Eric Blake March 8, 2013, 10:04 p.m. UTC | #1 
On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This patch is just for making review easier, those two functions will
> be modified and renamed later.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---

> +
> +void bdrv_collect_image_info(BlockDriverState *bs,
> +                             ImageInfo *info,
> +                             const char *fmt)
> +{

Three arguments here...

> +
> +void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info);
> +void bdrv_collect_image_info(BlockDriverState *bs,
> +                             ImageInfo *info,
> +                             const char *filename,
> +                             const char *fmt);

...but four here...

>  
> -static void collect_image_info(BlockDriverState *bs,
> -                   ImageInfo *info,
> -                   const char *filename)

...and moved from three arguments here...

>          info = g_new0(ImageInfo, 1);
> -        collect_image_info(bs, info, filename);
> -        collect_snapshots(bs, info);
> +        bdrv_collect_image_info(bs, info, filename, fmt);

...and your call site changes from 3 to 4 arguments.

How did you compile this?  Code motion must NOT make any semantic
changes - you should have exactly three arguments, preferably with the
same name, and save the addition of a fourth fmt argument until a later
patch.

Hint - a code motion patch should be easy to inspect with:
$ diff -u <(sed -n 's/^-//p' patch) <(sed -n 's/^\+//p' patch)

It's okay to have differences (such as 'static void collect_image_info'
becoming exported 'void bdrv_collect_image_info', and to see
reindentation to line up to the new function name), but the differences
should be trivially correct, and not a change between number of parameters.
Wayne Xia March 9, 2013, 4:20 a.m. UTC | #2 
于 2013-3-9 6:04, Eric Blake 写道:
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    This patch is just for making review easier, those two functions will
>> be modified and renamed later.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>
>> +
>> +void bdrv_collect_image_info(BlockDriverState *bs,
>> +                             ImageInfo *info,
>> +                             const char *fmt)
>> +{
>
> Three arguments here...
>
>> +
>> +void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info);
>> +void bdrv_collect_image_info(BlockDriverState *bs,
>> +                             ImageInfo *info,
>> +                             const char *filename,
>> +                             const char *fmt);
>
> ...but four here...
>
>>
>> -static void collect_image_info(BlockDriverState *bs,
>> -                   ImageInfo *info,
>> -                   const char *filename)
>
> ...and moved from three arguments here...
>
>>           info = g_new0(ImageInfo, 1);
>> -        collect_image_info(bs, info, filename);
>> -        collect_snapshots(bs, info);
>> +        bdrv_collect_image_info(bs, info, filename, fmt);
>
> ...and your call site changes from 3 to 4 arguments.
>
> How did you compile this?  Code motion must NOT make any semantic
> changes - you should have exactly three arguments, preferably with the
> same name, and save the addition of a fourth fmt argument until a later
> patch.
>
> Hint - a code motion patch should be easy to inspect with:
> $ diff -u <(sed -n 's/^-//p' patch) <(sed -n 's/^\+//p' patch)
>
> It's okay to have differences (such as 'static void collect_image_info'
> becoming exported 'void bdrv_collect_image_info', and to see
> reindentation to line up to the new function name), but the differences
> should be trivially correct, and not a change between number of parameters.
>

   My bad, I was dizzy in rebasing the patches, will correct it.
diff mbox

Patch

diff --git a/block/qapi.c b/block/qapi.c
index 3ac3590..643839c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -12,3 +12,85 @@ 
  */
 
 #include "block/qapi.h"
+#include "block/block_int.h"
+
+void bdrv_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);
+}
+
+void bdrv_collect_image_info(BlockDriverState *bs,
+                             ImageInfo *info,
+                             const char *fmt)
+{
+    uint64_t total_sectors;
+    char backing_filename[1024];
+    char backing_filename2[1024];
+    BlockDriverInfo bdi;
+
+    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;
+        }
+    }
+}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index e1c0967..a126768 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -1,4 +1,12 @@ 
 #ifndef BLOCK_QAPI_H
 #define BLOCK_QAPI_H
 
+#include "qapi-types.h"
+#include "block/block.h"
+
+void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info);
+void bdrv_collect_image_info(BlockDriverState *bs,
+                             ImageInfo *info,
+                             const char *filename,
+                             const char *fmt);
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index f4e5d90..bbc8f5b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -30,6 +30,7 @@ 
 #include "qemu/osdep.h"
 #include "sysemu/sysemu.h"
 #include "block/block_int.h"
+#include "block/qapi.h"
 #include <getopt.h>
 #include <stdio.h>
 #include <stdarg.h>
@@ -1588,39 +1589,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;
@@ -1638,54 +1606,6 @@  static void dump_json_image_info(ImageInfo *info)
     QDECREF(str);
 }
 
-static void collect_image_info(BlockDriverState *bs,
-                   ImageInfo *info,
-                   const char *filename)
-{
-    uint64_t total_sectors;
-    char backing_filename[1024];
-    char backing_filename2[1024];
-    BlockDriverInfo bdi;
-
-    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];
@@ -1814,8 +1734,8 @@  static ImageInfoList *collect_image_info_list(const char *filename,
         }
 
         info = g_new0(ImageInfo, 1);
-        collect_image_info(bs, info, filename);
-        collect_snapshots(bs, info);
+        bdrv_collect_image_info(bs, info, filename, fmt);
+        bdrv_collect_snapshots(bs, info);
 
         elem = g_new0(ImageInfoList, 1);
         elem->value = info;