Patchwork [V8,03/20] block: move bdrv_snapshot_find() to block/snapshot.c

login
register
mail settings
Submitter Wayne Xia
Date March 7, 2013, 6:07 a.m.
Message ID <1362636445-7188-4-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/225730/
State New
Headers show

Comments

Wayne Xia - March 7, 2013, 6:07 a.m.
This patch also fix small code style error reported by check script.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/snapshot.c         |   23 +++++++++++++++++++++++
 include/block/snapshot.h |    9 +++++++++
 savevm.c                 |   23 +----------------------
 3 files changed, 33 insertions(+), 22 deletions(-)
Eric Blake - March 8, 2013, 8:27 p.m.
On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This patch also fix small code style error reported by check script.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/snapshot.c         |   23 +++++++++++++++++++++++
>  include/block/snapshot.h |    9 +++++++++
>  savevm.c                 |   23 +----------------------
>  3 files changed, 33 insertions(+), 22 deletions(-)
> 

> +++ b/include/block/snapshot.h
> @@ -1,4 +1,13 @@
>  #ifndef SNAPSHOT_H
>  #define SNAPSHOT_H
>  
> +#include "qemu-common.h"
> +/*
> + * block.h is needed for QEMUSnapshotInfo, it can be removed when define is
> + * moved here.
> + */
> +#include "block.h"

Why not move QEMUSnapshotInfo here as part of this patch, and/or reorder
the series to do the code motion of that type before you move the function?

That said, this looks like an accurate code motion patch.  But see my
comments earlier in the series about merging this with 1/20.
Wayne Xia - March 9, 2013, 4:17 a.m.
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    This patch also fix small code style error reported by check script.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/snapshot.c         |   23 +++++++++++++++++++++++
>>   include/block/snapshot.h |    9 +++++++++
>>   savevm.c                 |   23 +----------------------
>>   3 files changed, 33 insertions(+), 22 deletions(-)
>>
>
>> +++ b/include/block/snapshot.h
>> @@ -1,4 +1,13 @@
>>   #ifndef SNAPSHOT_H
>>   #define SNAPSHOT_H
>>
>> +#include "qemu-common.h"
>> +/*
>> + * block.h is needed for QEMUSnapshotInfo, it can be removed when define is
>> + * moved here.
>> + */
>> +#include "block.h"
>
> Why not move QEMUSnapshotInfo here as part of this patch, and/or reorder
> the series to do the code motion of that type before you move the function?
>
   This is along serial which modify the file several times, so I
placed the important parts first, to avoid trouble if later patch need
modification. You can see patches 1 to 16 are clear and closely related
small patches, each of which do one step to archieve the goal, but 17
to 20 are "cleaning" patches, I'd rather drop them to make the serial
shorter, instead of move them front.

> That said, this looks like an accurate code motion patch.  But see my
> comments earlier in the series about merging this with 1/20.
>

Patch

diff --git a/block/snapshot.c b/block/snapshot.c
index c65519b..8de73b4 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -12,3 +12,26 @@ 
  */
 
 #include "block/snapshot.h"
+
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i, ret;
+
+    ret = -ENOENT;
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        return ret;
+    }
+    for (i = 0; i < nb_sns; i++) {
+        sn = &sn_tab[i];
+        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+            *sn_info = *sn;
+            ret = 0;
+            break;
+        }
+    }
+    g_free(sn_tab);
+    return ret;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 278e064..369e047 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -1,4 +1,13 @@ 
 #ifndef SNAPSHOT_H
 #define SNAPSHOT_H
 
+#include "qemu-common.h"
+/*
+ * block.h is needed for QEMUSnapshotInfo, it can be removed when define is
+ * moved here.
+ */
+#include "block.h"
+
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name);
 #endif
diff --git a/savevm.c b/savevm.c
index a8a53ef..95f19ca 100644
--- a/savevm.c
+++ b/savevm.c
@@ -39,6 +39,7 @@ 
 #include "qmp-commands.h"
 #include "trace.h"
 #include "qemu/bitops.h"
+#include "block/snapshot.h"
 
 #define SELF_ANNOUNCE_ROUNDS 5
 
@@ -2060,28 +2061,6 @@  out:
     return ret;
 }
 
-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                              const char *name)
-{
-    QEMUSnapshotInfo *sn_tab, *sn;
-    int nb_sns, i, ret;
-
-    ret = -ENOENT;
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0)
-        return ret;
-    for(i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
-            *sn_info = *sn;
-            ret = 0;
-            break;
-        }
-    }
-    g_free(sn_tab);
-    return ret;
-}
-
 /*
  * Deletes snapshots of a given name in all opened images.
  */