All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info
@ 2013-03-07  6:07 Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 01/20] build: add block/snapshot.c Wenchao Xia
                   ` (20 more replies)
  0 siblings, 21 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  In the use of snapshot a way to retrieve related info at runtime is needed,
so this serial of patches will merge some code for qemu and qemu-img, and add
following interfaces for qemu:

1) qmp: query-images, show image info for a block device
Example:
-> { "execute": "query-images" }
<- {
      "return":[
         {
            "device":"ide0-hd0",
            "image":{
               "filename":"disks/test1.img",
               "format":"qcow2",
               "virtual-size":2048000,
               "snapshots":[
                  {
                     "id": "1",
                     "name": "snapshot1",
                     "vm-state-size": 0,
                     "date-sec": 10000200,
                     "date-nsec": 12,
                     "vm-clock-sec": 206,
                     "vm-clock-nsec": 30
                  }
               ]
            }
         }
      ]
   }

2) qmp: query-snapshots, show avaiable vm snapshot info which can be used
in loadvm.
Example:
-> { "execute": "query-snapshots" }
<- {
      "return":[
         {
            "id": "1",
            "name": "snapshot1",
            "vm-state-size": 0,
            "date-sec": 10000200,
            "date-nsec": 12,
            "vm-clock-sec": 206,
            "vm-clock-nsec": 30
         }
      ]
    }

3) hmp: info images, show what got in qmp query-images in monitor

  These patches follows the rule that use qmp to retieve information,
hmp layer just do a translation from qmp object it got.
  To make code graceful, snapshot and image info retrieving code in
qemu and qemu-img are merged into block layer, and some function name was
adjusted to make it tips better. For the part touch by the serial, it works as:

   qemu          qemu-img

dump_monitor    dump_stdout
     |--------------| 
            |
       block/qapi.c

v2:
  Rename and adjusted qmp interface according to comments from Eric.
  Spelling fix.
  Information retrieving function in block layer goes to seperated patch.
  Free qmp object after usage in hmp.
  Added counterpart in qmp-commands.hx.
  Better tips in qmp-schema.json.

v3:
  Spelling fix in commit message, patch 03/11.
  Spelling fix in code, patch 06/11.
  Add comments that vm-state-size is in bytes, and change size of it in
example to a reasonable number, patch 08/11.

v4:
  02/13: in bdrv_get_filename(), add const to parameter *bs.
  03/13: new added, in which the function correct the behavior in info
retrieving.
  04/13: in bdrv_query_snapshot_infolist(), remove NULL check before call
err_setg(), added TODO comments that let block layer function set error instead
of this layer to tip better for errors, Split out patch about image info to
patch 05/13.
  05/13: new splitted, and it checks *bs by calling bdrv_can_read_snapshot()
before collect internal snasphot info to avoid *err is set unexpectly now.
  06/13: check if error happens after calling bdrv_query_image_info().
  08/13: rename info to image in DeviceImageInfo and make it optional,
when device is not inserted it will be empty, added error handling code
when met error in calling block layer API.
  09/13: distinguish *id and *name in bdrv_find_snapshots(), caller
can choose what to search with. id_wellformed() should be called in
new snapshot creation interface above this function in the future.
  10/13: now this interface have addtional parameter *device, which
enable showing internal snapshots on a single device. Also use
bdrv_can_read_snapshot() instead of bdrv_can_snapshot() now.
  11/13: this function goes to hmp.c so hmp_handler_error is not exported
any more, split out patch that switch snapshot info function to patch 12/13.
  12/13: new splitted.
  13/13: use qmp API instead of directly calling block layer API, now
all hmp function have correspond qmp funtion in this serial.

v5:
  04/13: spelling fix in commit message, better comments and better
tips of error, after calling bdrv_snapshot_list().

v6:
  ==Address Kevin's comments==:
  04/14: seperate patch for code moving,
  05/14: use strerror() for error tipping after call of bdrv_snapshot_list(),
use "switch" instead of "if else" for error checking, replace
info_list->value->* by info->*.
  06/14: access bs->backing_file directly instead of calling of
bdrv_get_backing_filename in collect_image_info. Function switch patch
of last version is merged into this patch.
  08/14: API change, add optional parameter of "device" and "backing",
which allow showing all image info in the backing file chain of block
device, add related implemention.
  09/14: seperate patch for code moving.
  10/14: seperate patch for function change, return what got from
bdrv_snapshot_list() instead of -ENOENT.

  ==Address Eric's comments==:
  02/14: return bool instead of int for this new function.
  03/14: return bool instead of int for old function bdrv_can_snapshot().

V7:
  ==Address Eric's comments==:
  6/14: discard of static function bdrv_get_filename().
  8/14: better doc and comments, trailing '.' is removed.
  11/14: QMP API change: removed parameter *device, it only returns VM snapshot
now. Block device's internal snapshot info can be accessed by query-images.
  12/14: related caller code change for query-snapshots. Seperate internal
static function for VM snapshot info.
  14/14: HMP API change: added support of query internal snapshots of one image
in the backing file chain of a block device. use query-images instead of
query-snapshots. Seperate internal static mirror function for block snapshot
info.

v8:
  General change:
    discard bdrv_can_read_snapshot() for that return value can tip error. add
block/snapshot.c and block/qapi.c and function are moved there. Patches 17-20
are for clean and moved to tail.

  Address Markus's comments:
  3/20: function was moved to block/snapshot.c instead of block.c.
  7/20: better commit message. Remove comments for the return value of
bdrv_snapshot_list(), remove period suffix in error message, better error
message, remove the filter callback of it. remove comments for *bs must be
open. It return negative value for error check.
  8/20: add a flag parameter to filter out inconsistent vm snapshots instead
of a call back function.
  9/20: function reorganized to return negative error value, and qmp object
will be got only on success.
  10/20: better documents and remove suffix period.
  11/20: better documents and remove suffix period, explain optional member
in qmp-commands.hx.

  Address Stefan's comments:
  6/20: add prefix 'bdrv_' to the moved functions.
  7/20: remove the filter call back function. remove bdrv_can_reand_snapshot()
call in qemu-img. It return negative value for error check.
  8/20: add a flag parameter to filter out inconsistent vm snapshots instead
of a call back function.
  9/20: small trivial static function was removed.

  Address Eric's comments:
  6/20: add prefix 'bdrv_' to the moved functions. Added message in commit
about the patch's purpose.

  Other:
  14/20 - 16/20: new interface info image which show all image info
including internal snapshots for each device, share the dump code
with qemu-img.
  18/20: new clean up patch for block/snapshot.c.
  19/20: new clean up patch for block/qapi.c.

Wenchao Xia (20):
  1 build: add block/snapshot.c
  2 build: add block/qapi.c
  3 block: move bdrv_snapshot_find() to block/snapshot.c
  4 block: distinguish id and name in bdrv_find_snapshot()
  5 qemu-img: remove unused parameter in collect_image_info()
  6 block: move collect_snapshots() and collect_image_info() to block/qapi.c
  7 block: add snapshot info query function bdrv_query_snapshot_info_list()
  8 block: add filter for vm snapshot in bdrv_query_snapshot_info_list()
  9 block: add image info query function bdrv_query_image_info()
  10 qmp: add interface query-snapshots
  11 qmp: add interface query-images
  12 hmp: add function hmp_info_snapshots()
  13 hmp: switch snapshot info function to qmp based one
  14 block: move dump_human_image_info() to block/qapi.c
  15 block: dump to buffer for bdrv_image_info_dump()
  16 hmp: add command info images
  17 block: return bool for bdrv_can_snapshot()
  18 block: move snapshot related functions to block/snapshot.c
  19 block: move bdrv_snapshot_dump() to block/qapi.c
  20 block: rename bdrv_query_info() to bdrv_query_block_info()

 block.c                   |  142 +---------------
 block/Makefile.objs       |    1 +
 block/qapi.c              |  416 +++++++++++++++++++++++++++++++++++++++++++++
 block/snapshot.c          |  181 ++++++++++++++++++++
 hmp.c                     |   80 +++++++++
 hmp.h                     |    2 +
 include/block/block.h     |   26 +---
 include/block/block_int.h |    1 +
 include/block/qapi.h      |   17 ++
 include/block/snapshot.h  |   31 ++++
 monitor.c                 |    9 +-
 qapi-schema.json          |   47 +++++
 qemu-img.c                |  162 +-----------------
 qmp-commands.hx           |  141 +++++++++++++++
 savevm.c                  |   95 +----------
 15 files changed, 943 insertions(+), 408 deletions(-)
 create mode 100644 block/qapi.c
 create mode 100644 block/snapshot.c
 create mode 100644 include/block/qapi.h
 create mode 100644 include/block/snapshot.h

^ permalink raw reply	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 01/20] build: add block/snapshot.c
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-08 20:18   ` Eric Blake
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 02/20] build: add block/qapi.c Wenchao Xia
                   ` (19 subsequent siblings)
  20 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This file will have internal snapshot related functions.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/Makefile.objs      |    1 +
 block/snapshot.c         |   14 ++++++++++++++
 include/block/snapshot.h |    4 ++++
 3 files changed, 19 insertions(+), 0 deletions(-)
 create mode 100644 block/snapshot.c
 create mode 100644 include/block/snapshot.h

diff --git a/block/Makefile.objs b/block/Makefile.objs
index c067f38..60a4cd2 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -3,6 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-y += parallels.o blkdebug.o blkverify.o
+block-obj-y += snapshot.o
 block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
 block-obj-$(CONFIG_POSIX) += raw-posix.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
diff --git a/block/snapshot.c b/block/snapshot.c
new file mode 100644
index 0000000..c65519b
--- /dev/null
+++ b/block/snapshot.c
@@ -0,0 +1,14 @@
+/*
+ * Snapshot related functions.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Wenchao Xia   <xiawenc@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "block/snapshot.h"
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
new file mode 100644
index 0000000..278e064
--- /dev/null
+++ b/include/block/snapshot.h
@@ -0,0 +1,4 @@
+#ifndef SNAPSHOT_H
+#define SNAPSHOT_H
+
+#endif
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 02/20] build: add block/qapi.c
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 01/20] build: add block/snapshot.c Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-08 20:22   ` Eric Blake
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 03/20] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
                   ` (18 subsequent siblings)
  20 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This file will have qmp related functions for block. To avoid conflict and
tip better, macro in header file is BLOCK_QAPI_H instead of QAPI_H.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/Makefile.objs  |    2 +-
 block/qapi.c         |   14 ++++++++++++++
 include/block/qapi.h |    4 ++++
 3 files changed, 19 insertions(+), 1 deletions(-)
 create mode 100644 block/qapi.c
 create mode 100644 include/block/qapi.h

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 60a4cd2..fc27bed 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -3,7 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
 block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
 block-obj-y += qed-check.o
 block-obj-y += parallels.o blkdebug.o blkverify.o
-block-obj-y += snapshot.o
+block-obj-y += snapshot.o qapi.o
 block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
 block-obj-$(CONFIG_POSIX) += raw-posix.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
diff --git a/block/qapi.c b/block/qapi.c
new file mode 100644
index 0000000..3ac3590
--- /dev/null
+++ b/block/qapi.c
@@ -0,0 +1,14 @@
+/*
+ * Block layer qmp related functions
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ *  Wenchao Xia   <xiawenc@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "block/qapi.h"
diff --git a/include/block/qapi.h b/include/block/qapi.h
new file mode 100644
index 0000000..e1c0967
--- /dev/null
+++ b/include/block/qapi.h
@@ -0,0 +1,4 @@
+#ifndef BLOCK_QAPI_H
+#define BLOCK_QAPI_H
+
+#endif
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 03/20] block: move bdrv_snapshot_find() to block/snapshot.c
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 01/20] build: add block/snapshot.c Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 02/20] build: add block/qapi.c Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-08 20:27   ` Eric Blake
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 04/20] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
                   ` (17 subsequent siblings)
  20 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  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(-)

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.
  */
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 04/20] block: distinguish id and name in bdrv_find_snapshot()
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (2 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 03/20] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 05/20] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
                   ` (16 subsequent siblings)
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  To make it clear about id and name in searching, the API was changed
a bit to distinguish them. Caller can choose to search by id or name now.
Searching will be done with higher priority of id. This function also return
negative value from bdrv_snapshot_list() instead of -ENOENT on error.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/snapshot.c         |   46 ++++++++++++++++++++++++++++++++++++++--------
 include/block/snapshot.h |    2 +-
 savevm.c                 |   10 +++++-----
 3 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 8de73b4..1449b3d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -13,8 +13,20 @@
 
 #include "block/snapshot.h"
 
+/*
+ * Try find an internal snapshot with @id or @name, @id have higher priority
+ * in searching.
+ *
+ * @bs: block device to search on, must not be NULL.
+ * @sn_info: snapshot information to be filled in, must not be NULL.
+ * @id: snapshot id to search with, can be NULL.
+ * @name: snapshot name to search with, can be NULL.
+ *
+ * returns 0 and @sn_info is filled with related information if found,
+ * otherwise it returns negative value.
+ */
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                       const char *name)
+                       const char *id, const char *name)
 {
     QEMUSnapshotInfo *sn_tab, *sn;
     int nb_sns, i, ret;
@@ -22,16 +34,34 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
     ret = -ENOENT;
     nb_sns = bdrv_snapshot_list(bs, &sn_tab);
     if (nb_sns < 0) {
-        return ret;
+        return nb_sns;
     }
-    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;
+
+    /* search by id */
+    if (id) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->id_str, id)) {
+                *sn_info = *sn;
+                ret = 0;
+                goto out;
+            }
         }
     }
+
+    /* search by name */
+    if (name) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                ret = 0;
+                goto out;
+            }
+        }
+    }
+
+ out:
     g_free(sn_tab);
     return ret;
 }
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 369e047..71b8017 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -9,5 +9,5 @@
 #include "block.h"
 
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                       const char *name);
+                       const char *id, const char *name);
 #endif
diff --git a/savevm.c b/savevm.c
index 95f19ca..6557750 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2073,7 +2073,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
     bs = NULL;
     while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs) &&
-            bdrv_snapshot_find(bs, snapshot, name) >= 0)
+            bdrv_snapshot_find(bs, snapshot, name, name) >= 0)
         {
             ret = bdrv_snapshot_delete(bs, name);
             if (ret < 0) {
@@ -2133,7 +2133,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
 
     if (name) {
-        ret = bdrv_snapshot_find(bs, old_sn, name);
+        ret = bdrv_snapshot_find(bs, old_sn, name, name);
         if (ret >= 0) {
             pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
             pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
@@ -2224,7 +2224,7 @@ int load_vmstate(const char *name)
     }
 
     /* Don't even try to load empty VM states */
-    ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
+    ret = bdrv_snapshot_find(bs_vm_state, &sn, name, name);
     if (ret < 0) {
         return ret;
     } else if (sn.vm_state_size == 0) {
@@ -2248,7 +2248,7 @@ int load_vmstate(const char *name)
             return -ENOTSUP;
         }
 
-        ret = bdrv_snapshot_find(bs, &sn, name);
+        ret = bdrv_snapshot_find(bs, &sn, name, name);
         if (ret < 0) {
             error_report("Device '%s' does not have the requested snapshot '%s'",
                            bdrv_get_device_name(bs), name);
@@ -2354,7 +2354,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
 
         while ((bs1 = bdrv_next(bs1))) {
             if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
+                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
                 if (ret < 0) {
                     available = 0;
                     break;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 05/20] qemu-img: remove unused parameter in collect_image_info()
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (3 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 04/20] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-08 20:34   ` Eric Blake
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 06/20] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
                   ` (15 subsequent siblings)
  20 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  Parameter *fmt was not used, so remove it.

Reviewed-by: Eric Blake <eblake@redhat.com>

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qemu-img.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 471de7d..f4e5d90 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1640,8 +1640,7 @@ static void dump_json_image_info(ImageInfo *info)
 
 static void collect_image_info(BlockDriverState *bs,
                    ImageInfo *info,
-                   const char *filename,
-                   const char *fmt)
+                   const char *filename)
 {
     uint64_t total_sectors;
     char backing_filename[1024];
@@ -1815,7 +1814,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
         }
 
         info = g_new0(ImageInfo, 1);
-        collect_image_info(bs, info, filename, fmt);
+        collect_image_info(bs, info, filename);
         collect_snapshots(bs, info);
 
         elem = g_new0(ImageInfoList, 1);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 06/20] block: move collect_snapshots() and collect_image_info() to block/qapi.c
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (4 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 05/20] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-08 22:04   ` Eric Blake
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 07/20] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
                   ` (14 subsequent siblings)
  20 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  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(-)

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;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 07/20] block: add snapshot info query function bdrv_query_snapshot_info_list()
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (5 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 06/20] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-08 22:15   ` Eric Blake
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
                   ` (13 subsequent siblings)
  20 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  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         |   52 +++++++++++++++++++++++++++++++++++++------------
 include/block/qapi.h |    4 ++-
 qemu-img.c           |    4 ++-
 3 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 643839c..b503cfa 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -14,29 +14,53 @@
 #include "block/qapi.h"
 #include "block/block_int.h"
 
-void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info)
+/* return 0 on success, and @p_list will be set only 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(errp, "Can't list snapshots of device '%s': %s",
+                       dev, strerror(-sn_count));
+            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;
@@ -45,6 +69,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 a126768..030964b 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -4,7 +4,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 bbc8f5b..32a72f5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1735,7 +1735,9 @@ static ImageInfoList *collect_image_info_list(const char *filename,
 
         info = g_new0(ImageInfo, 1);
         bdrv_collect_image_info(bs, info, filename, fmt);
-        bdrv_collect_snapshots(bs, info);
+        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL)) {
+            info->has_snapshots = true;
+        }
 
         elem = g_new0(ImageInfoList, 1);
         elem->value = info;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list()
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (6 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 07/20] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-08 22:55   ` Eric Blake
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 09/20] block: add image info query function bdrv_query_image_info() Wenchao Xia
                   ` (12 subsequent siblings)
  20 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This patch adds a parameter to tell whether return valid snapshots
for whole VM only.

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

diff --git a/block/qapi.c b/block/qapi.c
index b503cfa..a90b4c7 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -12,11 +12,44 @@
  */
 
 #include "block/qapi.h"
+#include "block/snapshot.h"
 #include "block/block_int.h"
 
-/* return 0 on success, and @p_list will be set only on success. */
+/*
+ * check whether the snapshot is valid for whole vm.
+ *
+ * @sn: snapshot info need to be checked.
+ * @bs: where @sn was found.
+ *
+ * return 0 if valid, it means load_vmstate() for it could succeed.
+ */
+static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, BlockDriverState *bs)
+{
+    BlockDriverState *bs1 = NULL;
+    QEMUSnapshotInfo s, *sn_info = &s;
+    int ret = 0;
+
+    /* Check logic is connected with load_vmstate():
+       Only check the devices that can snapshot, other devices that can't
+       take snapshot, for example, readonly ones, will be ignored in
+       load_vmstate(). */
+    while ((bs1 = bdrv_next(bs1))) {
+        if (bdrv_can_snapshot(bs1) && bs1 != bs) {
+            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
+            if (ret < 0) {
+                ret = -1;
+                break;
+            }
+        }
+    }
+    return ret;
+}
+
+/* return 0 on success, and @p_list will be set only on success. If
+   @vm_snapshot is true, only the snapshot valid for whole vm will be got. */
 int bdrv_query_snapshot_info_list(BlockDriverState *bs,
                                   SnapshotInfoList **p_list,
+                                  bool vm_snapshot,
                                   Error **errp)
 {
     int i, sn_count;
@@ -45,7 +78,9 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
     }
 
     for (i = 0; i < sn_count; i++) {
-
+        if (vm_snapshot && snapshot_filter_vm(&sn_tab[i], bs)) {
+            continue;
+        }
         info = g_new0(SnapshotInfo, 1);
         info->id            = g_strdup(sn_tab[i].id_str);
         info->name          = g_strdup(sn_tab[i].name);
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 030964b..4842c12 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -6,6 +6,7 @@
 
 int bdrv_query_snapshot_info_list(BlockDriverState *bs,
                                   SnapshotInfoList **p_list,
+                                  bool vm_snapshot,
                                   Error **errp);
 void bdrv_collect_image_info(BlockDriverState *bs,
                              ImageInfo *info,
diff --git a/qemu-img.c b/qemu-img.c
index 32a72f5..7786953 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1735,7 +1735,8 @@ static ImageInfoList *collect_image_info_list(const char *filename,
 
         info = g_new0(ImageInfo, 1);
         bdrv_collect_image_info(bs, info, filename, fmt);
-        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL)) {
+        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
+                                           false, NULL)) {
             info->has_snapshots = true;
         }
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 09/20] block: add image info query function bdrv_query_image_info()
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (7 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-08 23:08   ` Eric Blake
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 10/20] qmp: add interface query-snapshots Wenchao Xia
                   ` (11 subsequent siblings)
  20 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This patch adds function bdrv_query_image_info(), which will
retrieve image info in qmp object format. The implementation are
based on the code moved from qemu-img.c, but use block layer
function to get snapshot info.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c         |   39 ++++++++++++++++++++++++++++++++-------
 include/block/qapi.h |    7 +++----
 qemu-img.c           |    7 ++-----
 3 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a90b4c7..0c3055f 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -108,18 +108,22 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
     return 0;
 }
 
-void bdrv_collect_image_info(BlockDriverState *bs,
-                             ImageInfo *info,
-                             const char *fmt)
+/* return 0 on success, and @p_info will be set only on success. */
+int bdrv_query_image_info(BlockDriverState *bs,
+                          ImageInfo **p_info,
+                          Error **errp)
 {
     uint64_t total_sectors;
-    char backing_filename[1024];
+    const char *backing_filename;
     char backing_filename2[1024];
     BlockDriverInfo bdi;
+    int ret;
+    Error *err = NULL;
+    ImageInfo *info = g_new0(ImageInfo, 1);
 
     bdrv_get_geometry(bs, &total_sectors);
 
-    info->filename        = g_strdup(filename);
+    info->filename        = g_strdup(bs->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);
@@ -136,8 +140,8 @@ void bdrv_collect_image_info(BlockDriverState *bs,
         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') {
+    backing_filename = bs->backing_file;
+    if (backing_filename && backing_filename[0] != '\0') {
         info->backing_filename = g_strdup(backing_filename);
         info->has_backing_filename = true;
         bdrv_get_full_backing_filename(bs, backing_filename2,
@@ -154,4 +158,25 @@ void bdrv_collect_image_info(BlockDriverState *bs,
             info->has_backing_filename_format = true;
         }
     }
+
+    ret = bdrv_query_snapshot_info_list(bs, &info->snapshots, false, &err);
+    switch (ret) {
+    case 0:
+        info->has_snapshots = true;
+        break;
+    /* recoverable error */
+    case -ENOMEDIUM:
+        error_free(err);
+        break;
+    case -ENOTSUP:
+        error_free(err);
+        break;
+    default:
+        error_propagate(errp, err);
+        qapi_free_ImageInfo(info);
+        return ret;
+    }
+
+    *p_info = info;
+    return 0;
 }
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 4842c12..675df0c 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -8,8 +8,7 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
                                   SnapshotInfoList **p_list,
                                   bool vm_snapshot,
                                   Error **errp);
-void bdrv_collect_image_info(BlockDriverState *bs,
-                             ImageInfo *info,
-                             const char *filename,
-                             const char *fmt);
+int bdrv_query_image_info(BlockDriverState *bs,
+                          ImageInfo **p_info,
+                          Error **errp);
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index 7786953..59d900a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1733,11 +1733,8 @@ static ImageInfoList *collect_image_info_list(const char *filename,
             goto err;
         }
 
-        info = g_new0(ImageInfo, 1);
-        bdrv_collect_image_info(bs, info, filename, fmt);
-        if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
-                                           false, NULL)) {
-            info->has_snapshots = true;
+        if (bdrv_query_image_info(bs, &info, NULL)) {
+            goto err;
         }
 
         elem = g_new0(ImageInfoList, 1);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 10/20] qmp: add interface query-snapshots
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (8 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 09/20] block: add image info query function bdrv_query_image_info() Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-08 23:30   ` Eric Blake
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 11/20] qmp: add interface query-images Wenchao Xia
                   ` (10 subsequent siblings)
  20 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This interface now return valid internal snapshots for whole vm.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c     |   22 +++++++++++++++++++++
 qapi-schema.json |   14 +++++++++++++
 qmp-commands.hx  |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 0 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 0c3055f..b903dd8 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -14,6 +14,7 @@
 #include "block/qapi.h"
 #include "block/snapshot.h"
 #include "block/block_int.h"
+#include "qmp-commands.h"
 
 /*
  * check whether the snapshot is valid for whole vm.
@@ -180,3 +181,24 @@ int bdrv_query_image_info(BlockDriverState *bs,
     *p_info = info;
     return 0;
 }
+
+SnapshotInfoList *qmp_query_snapshots(Error **errp)
+{
+    BlockDriverState *bs;
+    SnapshotInfoList *list = NULL;
+    int ret;
+
+    /* internal snapshot for whole vm */
+    bs = bdrv_snapshots();
+    if (!bs) {
+        error_setg(errp, "No available block device supports snapshots\n");
+        return NULL;
+    }
+
+    ret = bdrv_query_snapshot_info_list(bs, &list, true, errp);
+    if (ret < 0) {
+        qapi_free_SnapshotInfoList(list);
+        list = NULL;
+    }
+    return list;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 28b070f..3710495 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -839,6 +839,20 @@
 { 'command': 'query-block', 'returns': ['BlockInfo'] }
 
 ##
+# @query-snapshots:
+#
+# Get a list of internal snapshots for whole virtual machine, only valid
+# internal snapshot will be returned, inconsistent ones will be ignored
+#
+# Returns: a list of @SnapshotInfo describing all consistent virtual machine
+#          snapshots
+#
+# Since: 1.5
+##
+{ 'command': 'query-snapshots',
+  'returns': ['SnapshotInfo'] }
+
+##
 # @BlockDeviceStats:
 #
 # Statistics of a virtual block device or a block backing device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 95022e2..d505209 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1743,6 +1743,61 @@ EQMP
     },
 
 SQMP
+query-snapshots
+-----------
+
+Show the internal consistent snapshot information
+
+Each snapshot is represented by a json-object. The returned value
+is a json-array of all snapshots
+
+Each json-object contain the following:
+
+- "id": unique snapshot id (json-string)
+- "name": internal snapshot name (json-string)
+- "vm-state-size": size of the VM state in bytes (json-int)
+- "date-sec": UTC date of the snapshot in seconds (json-int)
+- "date-nsec": fractional part in nano seconds to be used with
+               date-sec(json-int)
+- "vm-clock-sec": VM clock relative to boot in seconds (json-int)
+- "vm-clock-nsec": fractional part in nano seconds to be used with
+                   vm-clock-sec (json-int)
+
+Example:
+
+-> { "execute": "query-snapshots" }
+<- {
+      "return":[
+         {
+            "id": "1",
+            "name": "snapshot1",
+            "vm-state-size": 0,
+            "date-sec": 10000200,
+            "date-nsec": 12,
+            "vm-clock-sec": 206,
+            "vm-clock-nsec": 30
+         },
+         {
+            "id": "2",
+            "name": "snapshot2",
+            "vm-state-size": 34000000,
+            "date-sec": 13000200,
+            "date-nsec": 32,
+            "vm-clock-sec": 406,
+            "vm-clock-nsec": 31
+         }
+      ]
+   }
+
+EQMP
+
+    {
+        .name       = "query-snapshots",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_snapshots,
+    },
+
+SQMP
 query-blockstats
 ----------------
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 11/20] qmp: add interface query-images
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (9 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 10/20] qmp: add interface query-snapshots Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 12/20] hmp: add function hmp_info_snapshots() Wenchao Xia
                   ` (9 subsequent siblings)
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This mirror function will return image info including snapshots,
if specified backing image's info will also be returned. Now Qemu
have both query-images and query-block interfaces.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c     |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   33 ++++++++++++++++++++
 qmp-commands.hx  |   86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 202 insertions(+), 0 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index b903dd8..a67d380 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -202,3 +202,86 @@ SnapshotInfoList *qmp_query_snapshots(Error **errp)
     }
     return list;
 }
+
+/* Collect all info for one bs, p_list point to the empty tail to be filled,
+   return the empty tail of the new list. @bs must be the top one. */
+static DeviceImageInfoList **
+collect_device_image_info_list(BlockDriverState *bs,
+                               bool show_backing,
+                               DeviceImageInfoList **p_list,
+                               Error **errp)
+{
+    DeviceImageInfoList **p_next = p_list;
+    char *device_name = bs->device_name;
+
+    while (bs) {
+        DeviceImageInfo *dii = g_malloc0(sizeof(*dii));
+        DeviceImageInfoList *diil = g_malloc0(sizeof(*diil));
+        diil->value = dii;
+        *p_next = diil;
+        p_next = &diil->next;
+
+        dii->device = g_strdup(device_name);
+        if (!bdrv_is_inserted(bs)) {
+            dii->has_image = false;
+            break;
+        }
+        dii->has_image = true;
+        if (bdrv_query_image_info(bs, &dii->image, errp)) {
+            break;
+        }
+
+        if (show_backing && bs->drv && bs->backing_hd) {
+            bs = bs->backing_hd;
+        } else {
+            bs = NULL;
+        }
+    }
+    return p_next;
+}
+
+DeviceImageInfoList *qmp_query_images(bool has_device,
+                                      const char *device,
+                                      bool has_backing,
+                                      bool backing,
+                                      Error **errp)
+{
+    DeviceImageInfoList *head = NULL, **p_next = &head;
+    BlockDriverState *bs = NULL;
+    Error *err = NULL;
+    const char *target_device = NULL;
+    bool show_backing = false;
+
+    if (has_device) {
+        target_device = device;
+    }
+    if (has_backing) {
+        show_backing = backing;
+    }
+
+    if (target_device) {
+        bs = bdrv_find(device);
+        if (!bs) {
+            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+            return NULL;
+        }
+        collect_device_image_info_list(bs, show_backing, p_next, errp);
+        if (error_is_set(&err)) {
+            goto err;
+        }
+    } else {
+        while ((bs = bdrv_next(bs))) {
+            p_next = collect_device_image_info_list(bs, show_backing,
+                                                    p_next, errp);
+            if (error_is_set(&err)) {
+                goto err;
+            }
+        }
+    }
+
+    return head;
+
+ err:
+    qapi_free_DeviceImageInfoList(head);
+    return NULL;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 3710495..71a9eba 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -294,6 +294,21 @@
            '*total-clusters': 'int', '*allocated-clusters': 'int',
            '*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
 
+# @DeviceImageInfo:
+#
+# Information about an image used by a QEMU block device
+#
+# @device: name of the block device
+#
+# @image: #optional info of the image used
+#
+# Since: 1.5
+#
+##
+
+{ 'type': 'DeviceImageInfo',
+  'data': {'device': 'str', '*image': 'ImageInfo' } }
+
 ##
 # @StatusInfo:
 #
@@ -853,6 +868,24 @@
   'returns': ['SnapshotInfo'] }
 
 ##
+# @query-images:
+#
+# Get block device image information
+#
+# @device: #optional the name of the device to get image info from. If not
+#          specified, all block devices will be queried
+# @backing: #optional true to show information on backing images, false or
+#          omitted to show just the top image of a block device
+#
+# Returns: a list of @DeviceImageInfo describing each virtual block device
+#
+# Since: 1.5
+##
+{ 'command': 'query-images',
+  'data': { '*device': 'str', '*backing': 'bool' },
+  'returns': ['DeviceImageInfo'] }
+
+##
 # @BlockDeviceStats:
 #
 # Statistics of a virtual block device or a block backing device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d505209..f0482c2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1798,6 +1798,92 @@ EQMP
     },
 
 SQMP
+query-images
+-----------
+
+Show block device image information
+
+Each device is represented by a json-object. The returned value is a json-array
+of all devices' images
+
+Each json-object contain the following:
+
+- "device": device name (json-string)
+- "image": image information(json-object, optional) containing:
+         - "filename": image file name (json-string)
+         - "format": image format (json-string)
+         - "virtual-size": image capacity in bytes (json-int)
+         - "dirty-flag": true if image is not cleanly closed, not present means
+                         clean (json-bool, optional)
+         - "actual-size": actual size on disk in bytes of the image, not
+                          present when image does not support thin provision
+                          (json-int, optional)
+         - "cluster-size": size of a cluster in bytes, only present when image
+                           format support cluster (json-int, optional)
+         - "encrypted": true if the image is encrypted, only present when image
+                        support encryption (json-bool, optional)
+         - "backing_file": backing file name, only present when image format
+                           support backing file chain (json-string, optional)
+         - "full-backing-filename": full path of the backing file
+                                    (json-string, optional)
+         - "backing-filename-format": the format of the backing file
+                                      (json-string, optional)
+         - "snapshots": the internal snapshot info, it is an optional list of
+            json-object containing the following:
+             - "id": unique snapshot id (json-string)
+             - "name": snapshot name (json-string)
+             - "vm-state-size": size of the VM state in bytes (json-int)
+             - "date-sec": UTC date of the snapshot in seconds (json-int)
+             - "date-nsec": fractional part in nano seconds to be used with
+                            date-sec(json-int)
+             - "vm-clock-sec": VM clock relative to boot in seconds (json-int)
+             - "vm-clock-nsec": fractional part in nano seconds to be used with
+                                vm-clock-sec (json-int)
+
+Example:
+
+-> { "execute": "query-images" }
+<- {
+      "return":[
+         {
+            "device":"ide0-hd0",
+            "image":{
+               "filename":"disks/test0.img",
+               "format":"qcow2",
+               "virtual-size":1024000
+            }
+         },
+         {
+            "device":"ide0-hd1",
+            "image":{
+               "filename":"disks/test1.img",
+               "format":"qcow2",
+               "virtual-size":2048000,
+               "snapshots":[
+                  {
+                     "id": "1",
+                     "name": "snapshot1",
+                     "vm-state-size": 0,
+                     "date-sec": 10000200,
+                     "date-nsec": 12,
+                     "vm-clock-sec": 206,
+                     "vm-clock-nsec": 30
+                  }
+               ]
+            }
+         }
+      ]
+   }
+
+EQMP
+
+    {
+        .name       = "query-images",
+        .args_type  = "device:B?, backing:-b",
+        .mhandler.cmd_new = qmp_marshal_input_query_images,
+    },
+
+SQMP
 query-blockstats
 ----------------
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 12/20] hmp: add function hmp_info_snapshots()
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (10 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 11/20] qmp: add interface query-images Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 13/20] hmp: switch snapshot info function to qmp based one Wenchao Xia
                   ` (8 subsequent siblings)
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This function will simply call qmp interface qmp_query_snapshots()
added in last commit and then dump information in monitor console.
  To get snapshot info, Now qemu and qemu-img both call block layer
function bdrv_query_snapshot_info_list() in their calling path, and
then they just translate the qmp object got to strings in stdout or
monitor console.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 hmp.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 hmp.h |    1 +
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/hmp.c b/hmp.c
index 2f47a8a..34f0691 100644
--- a/hmp.c
+++ b/hmp.c
@@ -607,6 +607,48 @@ void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
     }
 }
 
+/* assume list is valid */
+static void monitor_dump_snapshotinfolist(Monitor *mon, SnapshotInfoList *list)
+{
+    SnapshotInfoList *elem;
+    char buf[256];
+
+    monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+
+    for (elem = list; elem; elem = elem->next) {
+        QEMUSnapshotInfo sn = {
+            .vm_state_size = elem->value->vm_state_size,
+            .date_sec = elem->value->date_sec,
+            .date_nsec = elem->value->date_nsec,
+            .vm_clock_nsec = elem->value->vm_clock_sec * 1000000000ULL +
+                             elem->value->vm_clock_nsec,
+        };
+        pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
+        pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
+        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+    }
+}
+
+void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    SnapshotInfoList *list;
+
+    list = qmp_query_snapshots(&err);
+    if (error_is_set(&err)) {
+        hmp_handle_error(mon, &err);
+        return;
+    }
+
+    if (list == NULL) {
+        monitor_printf(mon, "There is no suitable snapshot available\n");
+        return;
+    }
+
+    monitor_dump_snapshotinfolist(mon, list);
+    qapi_free_SnapshotInfoList(list);
+}
+
 void hmp_quit(Monitor *mon, const QDict *qdict)
 {
     monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 30b3c20..0b0ae13 100644
--- a/hmp.h
+++ b/hmp.h
@@ -36,6 +36,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict);
 void hmp_info_balloon(Monitor *mon, const QDict *qdict);
 void hmp_info_pci(Monitor *mon, const QDict *qdict);
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
+void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 13/20] hmp: switch snapshot info function to qmp based one
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (11 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 12/20] hmp: add function hmp_info_snapshots() Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 14/20] block: move dump_human_image_info() to block/qapi.c Wenchao Xia
                   ` (7 subsequent siblings)
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This patch using new added function in last commit which retrieve
info from qmp for snapshot info.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 monitor.c |    2 +-
 savevm.c  |   64 -------------------------------------------------------------
 2 files changed, 1 insertions(+), 65 deletions(-)

diff --git a/monitor.c b/monitor.c
index 32a6e74..5112375 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2594,7 +2594,7 @@ static mon_cmd_t info_cmds[] = {
         .args_type  = "",
         .params     = "",
         .help       = "show the currently saved VM snapshots",
-        .mhandler.cmd = do_info_snapshots,
+        .mhandler.cmd = hmp_info_snapshots,
     },
     {
         .name       = "status",
diff --git a/savevm.c b/savevm.c
index 6557750..71e78dc 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2319,70 +2319,6 @@ void do_delvm(Monitor *mon, const QDict *qdict)
     }
 }
 
-void do_info_snapshots(Monitor *mon, const QDict *qdict)
-{
-    BlockDriverState *bs, *bs1;
-    QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
-    int nb_sns, i, ret, available;
-    int total;
-    int *available_snapshots;
-    char buf[256];
-
-    bs = bdrv_snapshots();
-    if (!bs) {
-        monitor_printf(mon, "No available block device supports snapshots\n");
-        return;
-    }
-
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0) {
-        monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
-        return;
-    }
-
-    if (nb_sns == 0) {
-        monitor_printf(mon, "There is no snapshot available.\n");
-        return;
-    }
-
-    available_snapshots = g_malloc0(sizeof(int) * nb_sns);
-    total = 0;
-    for (i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        available = 1;
-        bs1 = NULL;
-
-        while ((bs1 = bdrv_next(bs1))) {
-            if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
-                if (ret < 0) {
-                    available = 0;
-                    break;
-                }
-            }
-        }
-
-        if (available) {
-            available_snapshots[total] = i;
-            total++;
-        }
-    }
-
-    if (total > 0) {
-        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
-        for (i = 0; i < total; i++) {
-            sn = &sn_tab[available_snapshots[i]];
-            monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
-        }
-    } else {
-        monitor_printf(mon, "There is no suitable snapshot available\n");
-    }
-
-    g_free(sn_tab);
-    g_free(available_snapshots);
-
-}
-
 void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
 {
     qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK,
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 14/20] block: move dump_human_image_info() to block/qapi.c
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (12 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 13/20] hmp: switch snapshot info function to qmp based one Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 15/20] block: dump to buffer for bdrv_image_info_dump() Wenchao Xia
                   ` (6 subsequent siblings)
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This function is needed later in hmp command, it is also renamed to
for bdrv_image_info_dump().

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c         |   67 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/qapi.h |    1 +
 qemu-img.c           |   69 +-------------------------------------------------
 3 files changed, 69 insertions(+), 68 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a67d380..aef1397 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -285,3 +285,70 @@ DeviceImageInfoList *qmp_query_images(bool has_device,
     qapi_free_DeviceImageInfoList(head);
     return NULL;
 }
+
+void bdrv_image_info_dump(ImageInfo *info)
+{
+    char size_buf[128], dsize_buf[128];
+    if (!info->has_actual_size) {
+        snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
+    } else {
+        get_human_readable_size(dsize_buf, sizeof(dsize_buf),
+                                info->actual_size);
+    }
+    get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
+    printf("image: %s\n"
+           "file format: %s\n"
+           "virtual size: %s (%" PRId64 " bytes)\n"
+           "disk size: %s\n",
+           info->filename, info->format, size_buf,
+           info->virtual_size,
+           dsize_buf);
+
+    if (info->has_encrypted && info->encrypted) {
+        printf("encrypted: yes\n");
+    }
+
+    if (info->has_cluster_size) {
+        printf("cluster_size: %" PRId64 "\n", info->cluster_size);
+    }
+
+    if (info->has_dirty_flag && info->dirty_flag) {
+        printf("cleanly shut down: no\n");
+    }
+
+    if (info->has_backing_filename) {
+        printf("backing file: %s", info->backing_filename);
+        if (info->has_full_backing_filename) {
+            printf(" (actual path: %s)", info->full_backing_filename);
+        }
+        putchar('\n');
+        if (info->has_backing_filename_format) {
+            printf("backing file format: %s\n", info->backing_filename_format);
+        }
+    }
+
+    if (info->has_snapshots) {
+        SnapshotInfoList *elem;
+        char buf[256];
+
+        printf("Snapshot list:\n");
+        printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+
+        /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
+         * we convert to the block layer's native QEMUSnapshotInfo for now.
+         */
+        for (elem = info->snapshots; elem; elem = elem->next) {
+            QEMUSnapshotInfo sn = {
+                .vm_state_size = elem->value->vm_state_size,
+                .date_sec = elem->value->date_sec,
+                .date_nsec = elem->value->date_nsec,
+                .vm_clock_nsec = elem->value->vm_clock_sec * 1000000000ULL +
+                                 elem->value->vm_clock_nsec,
+            };
+
+            pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
+            pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
+            printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+        }
+    }
+}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 675df0c..76b9e3f 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -11,4 +11,5 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
 int bdrv_query_image_info(BlockDriverState *bs,
                           ImageInfo **p_info,
                           Error **errp);
+void bdrv_image_info_dump(ImageInfo *info);
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index 59d900a..e1d6bac 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1606,73 +1606,6 @@ static void dump_json_image_info(ImageInfo *info)
     QDECREF(str);
 }
 
-static void dump_human_image_info(ImageInfo *info)
-{
-    char size_buf[128], dsize_buf[128];
-    if (!info->has_actual_size) {
-        snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
-    } else {
-        get_human_readable_size(dsize_buf, sizeof(dsize_buf),
-                                info->actual_size);
-    }
-    get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
-    printf("image: %s\n"
-           "file format: %s\n"
-           "virtual size: %s (%" PRId64 " bytes)\n"
-           "disk size: %s\n",
-           info->filename, info->format, size_buf,
-           info->virtual_size,
-           dsize_buf);
-
-    if (info->has_encrypted && info->encrypted) {
-        printf("encrypted: yes\n");
-    }
-
-    if (info->has_cluster_size) {
-        printf("cluster_size: %" PRId64 "\n", info->cluster_size);
-    }
-
-    if (info->has_dirty_flag && info->dirty_flag) {
-        printf("cleanly shut down: no\n");
-    }
-
-    if (info->has_backing_filename) {
-        printf("backing file: %s", info->backing_filename);
-        if (info->has_full_backing_filename) {
-            printf(" (actual path: %s)", info->full_backing_filename);
-        }
-        putchar('\n');
-        if (info->has_backing_filename_format) {
-            printf("backing file format: %s\n", info->backing_filename_format);
-        }
-    }
-
-    if (info->has_snapshots) {
-        SnapshotInfoList *elem;
-        char buf[256];
-
-        printf("Snapshot list:\n");
-        printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
-
-        /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
-         * we convert to the block layer's native QEMUSnapshotInfo for now.
-         */
-        for (elem = info->snapshots; elem; elem = elem->next) {
-            QEMUSnapshotInfo sn = {
-                .vm_state_size = elem->value->vm_state_size,
-                .date_sec = elem->value->date_sec,
-                .date_nsec = elem->value->date_nsec,
-                .vm_clock_nsec = elem->value->vm_clock_sec * 1000000000ULL +
-                                 elem->value->vm_clock_nsec,
-            };
-
-            pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
-            pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
-            printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
-        }
-    }
-}
-
 static void dump_human_image_info_list(ImageInfoList *list)
 {
     ImageInfoList *elem;
@@ -1684,7 +1617,7 @@ static void dump_human_image_info_list(ImageInfoList *list)
         }
         delim = true;
 
-        dump_human_image_info(elem->value);
+        bdrv_image_info_dump(elem->value);
     }
 }
 
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 15/20] block: dump to buffer for bdrv_image_info_dump()
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (13 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 14/20] block: move dump_human_image_info() to block/qapi.c Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 16/20] hmp: add command info images Wenchao Xia
                   ` (5 subsequent siblings)
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This allow hmp use this function, just like qemu-img.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qapi.c         |   67 +++++++++++++++++++++++++++++++++++--------------
 include/block/qapi.h |    2 +-
 qemu-img.c           |    6 +++-
 3 files changed, 54 insertions(+), 21 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index aef1397..f890c1e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -286,9 +286,30 @@ DeviceImageInfoList *qmp_query_images(bool has_device,
     return NULL;
 }
 
-void bdrv_image_info_dump(ImageInfo *info)
+GCC_FMT_ATTR(3, 4)
+static void snprintf_tail(char **p_buf, int *p_size, const char *fmt, ...)
+{
+    int l;
+    if (*p_size < 1) {
+        return;
+    }
+
+    va_list ap;
+    va_start(ap, fmt);
+    l = vsnprintf(*p_buf, *p_size, fmt, ap);
+    va_end(ap);
+    if (l > 0) {
+        *p_buf += l;
+        *p_size -= l;
+    }
+}
+
+/* Use buf instead of asprintf to reduce memory fragility. */
+char *bdrv_image_info_dump(char *buf, int buf_size, ImageInfo *info)
 {
     char size_buf[128], dsize_buf[128];
+    char *buf1 = buf;
+
     if (!info->has_actual_size) {
         snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
     } else {
@@ -296,43 +317,49 @@ void bdrv_image_info_dump(ImageInfo *info)
                                 info->actual_size);
     }
     get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
-    printf("image: %s\n"
-           "file format: %s\n"
-           "virtual size: %s (%" PRId64 " bytes)\n"
-           "disk size: %s\n",
-           info->filename, info->format, size_buf,
-           info->virtual_size,
-           dsize_buf);
+    snprintf_tail(&buf1, &buf_size,
+                  "image: %s\n"
+                  "file format: %s\n"
+                  "virtual size: %s (%" PRId64 " bytes)\n"
+                  "disk size: %s\n",
+                  info->filename, info->format, size_buf,
+                  info->virtual_size,
+                  dsize_buf);
 
     if (info->has_encrypted && info->encrypted) {
-        printf("encrypted: yes\n");
+        snprintf_tail(&buf1, &buf_size, "encrypted: yes\n");
     }
 
     if (info->has_cluster_size) {
-        printf("cluster_size: %" PRId64 "\n", info->cluster_size);
+        snprintf_tail(&buf1, &buf_size, "cluster_size: %" PRId64 "\n",
+                 info->cluster_size);
     }
 
     if (info->has_dirty_flag && info->dirty_flag) {
-        printf("cleanly shut down: no\n");
+        snprintf_tail(&buf1, &buf_size, "cleanly shut down: no\n");
     }
 
     if (info->has_backing_filename) {
-        printf("backing file: %s", info->backing_filename);
+        snprintf_tail(&buf1, &buf_size, "backing file: %s",
+                      info->backing_filename);
         if (info->has_full_backing_filename) {
-            printf(" (actual path: %s)", info->full_backing_filename);
+            snprintf_tail(&buf1, &buf_size, " (actual path: %s)",
+                          info->full_backing_filename);
         }
-        putchar('\n');
+        snprintf_tail(&buf1, &buf_size, "\n");
         if (info->has_backing_filename_format) {
-            printf("backing file format: %s\n", info->backing_filename_format);
+            snprintf_tail(&buf1, &buf_size, "backing file format: %s\n",
+                          info->backing_filename_format);
         }
     }
 
     if (info->has_snapshots) {
         SnapshotInfoList *elem;
-        char buf[256];
+        char buf_sn[256];
 
-        printf("Snapshot list:\n");
-        printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+        snprintf_tail(&buf1, &buf_size, "Snapshot list:\n");
+        snprintf_tail(&buf1, &buf_size, "%s\n",
+                      bdrv_snapshot_dump(buf_sn, sizeof(buf_sn), NULL));
 
         /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
          * we convert to the block layer's native QEMUSnapshotInfo for now.
@@ -348,7 +375,9 @@ void bdrv_image_info_dump(ImageInfo *info)
 
             pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
             pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
-            printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+            snprintf_tail(&buf1, &buf_size, "%s\n",
+                          bdrv_snapshot_dump(buf_sn, sizeof(buf_sn), &sn));
         }
     }
+    return buf;
 }
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 76b9e3f..235e6f0 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -11,5 +11,5 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
 int bdrv_query_image_info(BlockDriverState *bs,
                           ImageInfo **p_info,
                           Error **errp);
-void bdrv_image_info_dump(ImageInfo *info);
+char *bdrv_image_info_dump(char *buf, int buf_size, ImageInfo *info);
 #endif
diff --git a/qemu-img.c b/qemu-img.c
index e1d6bac..6db2b38 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1606,10 +1606,12 @@ static void dump_json_image_info(ImageInfo *info)
     QDECREF(str);
 }
 
+#define IMAGE_INFO_BUF_SIZE (2048)
 static void dump_human_image_info_list(ImageInfoList *list)
 {
     ImageInfoList *elem;
     bool delim = false;
+    char *buf = g_malloc0(IMAGE_INFO_BUF_SIZE);
 
     for (elem = list; elem; elem = elem->next) {
         if (delim) {
@@ -1617,8 +1619,10 @@ static void dump_human_image_info_list(ImageInfoList *list)
         }
         delim = true;
 
-        bdrv_image_info_dump(elem->value);
+        printf("%s",
+               bdrv_image_info_dump(buf, IMAGE_INFO_BUF_SIZE, elem->value));
     }
+    g_free(buf);
 }
 
 static gboolean str_equal_func(gconstpointer a, gconstpointer b)
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 16/20] hmp: add command info images
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (14 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 15/20] block: dump to buffer for bdrv_image_info_dump() Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 17/20] block: return bool for bdrv_can_snapshot() Wenchao Xia
                   ` (4 subsequent siblings)
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This command will show block image's information, including
internal snapshots.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 hmp.c     |   38 ++++++++++++++++++++++++++++++++++++++
 hmp.h     |    1 +
 monitor.c |    7 +++++++
 3 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/hmp.c b/hmp.c
index 34f0691..05d9b3b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -22,6 +22,7 @@
 #include "qemu/sockets.h"
 #include "monitor/monitor.h"
 #include "ui/console.h"
+#include "block/qapi.h"
 
 static void hmp_handle_error(Monitor *mon, Error **errp)
 {
@@ -649,6 +650,43 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     qapi_free_SnapshotInfoList(list);
 }
 
+#define IMAGE_INFO_BUF_SIZE (2048)
+static void mon_dump_device_image_info_list(Monitor *mon,
+                                            DeviceImageInfoList *list)
+{
+    char *buf = g_malloc0(IMAGE_INFO_BUF_SIZE);
+    DeviceImageInfoList *p = list;
+    DeviceImageInfo *dii;
+    while (p) {
+        dii = p->value;
+        monitor_printf(mon, "Device : %s\n", dii->device);
+        if (dii->has_image) {
+            bdrv_image_info_dump(buf, IMAGE_INFO_BUF_SIZE, dii->image);
+            monitor_printf(mon, "%s", buf);
+        }
+        monitor_printf(mon, "\n");
+
+        p = p->next;
+    }
+    g_free(buf);
+}
+
+void hmp_info_images(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_try_str(qdict, "device");
+    int backing = qdict_get_try_bool(qdict, "backing", 0);
+    Error *err = NULL;
+    DeviceImageInfoList *list = NULL;
+
+    list = qmp_query_images(!!device, device, true, backing, &err);
+    if (!error_is_set(&err) && list) {
+        mon_dump_device_image_info_list(mon, list);
+    }
+
+    hmp_handle_error(mon, &err);
+    qapi_free_DeviceImageInfoList(list);
+}
+
 void hmp_quit(Monitor *mon, const QDict *qdict)
 {
     monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 0b0ae13..9d9ada3 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
 void hmp_info_pci(Monitor *mon, const QDict *qdict);
 void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
+void hmp_info_images(Monitor *mon, const QDict *qdict);
 void hmp_quit(Monitor *mon, const QDict *qdict);
 void hmp_stop(Monitor *mon, const QDict *qdict);
 void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index 5112375..b470963 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2597,6 +2597,13 @@ static mon_cmd_t info_cmds[] = {
         .mhandler.cmd = hmp_info_snapshots,
     },
     {
+        .name       = "images",
+        .args_type  = "backing:-b,device:B?",
+        .params     = "[-b] [device]",
+        .help       = "show the image info",
+        .mhandler.cmd = hmp_info_images,
+    },
+    {
         .name       = "status",
         .args_type  = "",
         .params     = "",
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 17/20] block: return bool for bdrv_can_snapshot()
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (15 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 16/20] hmp: add command info images Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 18/20] block: move snapshot related functions to block/snapshot.c Wenchao Xia
                   ` (3 subsequent siblings)
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  This function should return bool instead of int, just as
bdrv_can_read_snapshot().

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c               |    8 ++++----
 include/block/block.h |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 124a9eb..77e21f5 100644
--- a/block.c
+++ b/block.c
@@ -3123,21 +3123,21 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
 /**************************************************************/
 /* handling of snapshots */
 
-int bdrv_can_snapshot(BlockDriverState *bs)
+bool bdrv_can_snapshot(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
     if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
-        return 0;
+        return false;
     }
 
     if (!drv->bdrv_snapshot_create) {
         if (bs->file != NULL) {
             return bdrv_can_snapshot(bs->file);
         }
-        return 0;
+        return false;
     }
 
-    return 1;
+    return true;
 }
 
 int bdrv_is_snapshot(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index 0f750d7..c883857 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -327,7 +327,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
                                     char *dest, size_t sz);
 BlockInfo *bdrv_query_info(BlockDriverState *s);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
-int bdrv_can_snapshot(BlockDriverState *bs);
+bool bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);
 int bdrv_snapshot_create(BlockDriverState *bs,
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 18/20] block: move snapshot related functions to block/snapshot.c
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (16 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 17/20] block: return bool for bdrv_can_snapshot() Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 19/20] block: move bdrv_snapshot_dump() to block/qapi.c Wenchao Xia
                   ` (2 subsequent siblings)
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  bdrv_snapshots() ref to a static variable used by other functions in
block.c, and it returns *bs like a general block function, so this
function was not moved to avoid trouble. bdrv_snapshot_dump() will
goto block/api.c later, and block/snapshot.h is included to make
build pass for it, which will be erased after it is moved.
  This patch also fix code style errors reported by check script.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c                  |  105 ------------------------------------------
 block/snapshot.c         |  114 ++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h    |   24 +---------
 include/block/snapshot.h |   28 +++++++++--
 4 files changed, 139 insertions(+), 132 deletions(-)

diff --git a/block.c b/block.c
index 77e21f5..c2ca82a 100644
--- a/block.c
+++ b/block.c
@@ -3120,31 +3120,6 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
     return false;
 }
 
-/**************************************************************/
-/* handling of snapshots */
-
-bool bdrv_can_snapshot(BlockDriverState *bs)
-{
-    BlockDriver *drv = bs->drv;
-    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
-        return false;
-    }
-
-    if (!drv->bdrv_snapshot_create) {
-        if (bs->file != NULL) {
-            return bdrv_can_snapshot(bs->file);
-        }
-        return false;
-    }
-
-    return true;
-}
-
-int bdrv_is_snapshot(BlockDriverState *bs)
-{
-    return !!(bs->open_flags & BDRV_O_SNAPSHOT);
-}
-
 BlockDriverState *bdrv_snapshots(void)
 {
     BlockDriverState *bs;
@@ -3163,86 +3138,6 @@ BlockDriverState *bdrv_snapshots(void)
     return NULL;
 }
 
-int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
-{
-    BlockDriver *drv = bs->drv;
-    if (!drv)
-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_create)
-        return drv->bdrv_snapshot_create(bs, sn_info);
-    if (bs->file)
-        return bdrv_snapshot_create(bs->file, sn_info);
-    return -ENOTSUP;
-}
-
-int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id)
-{
-    BlockDriver *drv = bs->drv;
-    int ret, open_ret;
-
-    if (!drv)
-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_goto)
-        return drv->bdrv_snapshot_goto(bs, snapshot_id);
-
-    if (bs->file) {
-        drv->bdrv_close(bs);
-        ret = bdrv_snapshot_goto(bs->file, snapshot_id);
-        open_ret = drv->bdrv_open(bs, bs->open_flags);
-        if (open_ret < 0) {
-            bdrv_delete(bs->file);
-            bs->drv = NULL;
-            return open_ret;
-        }
-        return ret;
-    }
-
-    return -ENOTSUP;
-}
-
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
-{
-    BlockDriver *drv = bs->drv;
-    if (!drv)
-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_delete)
-        return drv->bdrv_snapshot_delete(bs, snapshot_id);
-    if (bs->file)
-        return bdrv_snapshot_delete(bs->file, snapshot_id);
-    return -ENOTSUP;
-}
-
-int bdrv_snapshot_list(BlockDriverState *bs,
-                       QEMUSnapshotInfo **psn_info)
-{
-    BlockDriver *drv = bs->drv;
-    if (!drv)
-        return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_list)
-        return drv->bdrv_snapshot_list(bs, psn_info);
-    if (bs->file)
-        return bdrv_snapshot_list(bs->file, psn_info);
-    return -ENOTSUP;
-}
-
-int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-        const char *snapshot_name)
-{
-    BlockDriver *drv = bs->drv;
-    if (!drv) {
-        return -ENOMEDIUM;
-    }
-    if (!bs->read_only) {
-        return -EINVAL;
-    }
-    if (drv->bdrv_snapshot_load_tmp) {
-        return drv->bdrv_snapshot_load_tmp(bs, snapshot_name);
-    }
-    return -ENOTSUP;
-}
-
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
  * relative, it must be relative to the chain.  So, passing in bs->filename
  * from a BDS as backing_file should not be done, as that may be relative to
diff --git a/block/snapshot.c b/block/snapshot.c
index 1449b3d..eb8d744 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -12,6 +12,7 @@
  */
 
 #include "block/snapshot.h"
+#include "block/block_int.h"
 
 /*
  * Try find an internal snapshot with @id or @name, @id have higher priority
@@ -65,3 +66,116 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
     g_free(sn_tab);
     return ret;
 }
+
+bool bdrv_can_snapshot(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+        return false;
+    }
+
+    if (!drv->bdrv_snapshot_create) {
+        if (bs->file != NULL) {
+            return bdrv_can_snapshot(bs->file);
+        }
+        return false;
+    }
+
+    return true;
+}
+
+int bdrv_is_snapshot(BlockDriverState *bs)
+{
+    return !!(bs->open_flags & BDRV_O_SNAPSHOT);
+}
+
+int bdrv_snapshot_create(BlockDriverState *bs,
+                         QEMUSnapshotInfo *sn_info)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (drv->bdrv_snapshot_create) {
+        return drv->bdrv_snapshot_create(bs, sn_info);
+    }
+    if (bs->file) {
+        return bdrv_snapshot_create(bs->file, sn_info);
+    }
+    return -ENOTSUP;
+}
+
+int bdrv_snapshot_goto(BlockDriverState *bs,
+                       const char *snapshot_id)
+{
+    BlockDriver *drv = bs->drv;
+    int ret, open_ret;
+
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (drv->bdrv_snapshot_goto) {
+        return drv->bdrv_snapshot_goto(bs, snapshot_id);
+    }
+
+    if (bs->file) {
+        drv->bdrv_close(bs);
+        ret = bdrv_snapshot_goto(bs->file, snapshot_id);
+        open_ret = drv->bdrv_open(bs, bs->open_flags);
+        if (open_ret < 0) {
+            bdrv_delete(bs->file);
+            bs->drv = NULL;
+            return open_ret;
+        }
+        return ret;
+    }
+
+    return -ENOTSUP;
+}
+
+int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (drv->bdrv_snapshot_delete) {
+        return drv->bdrv_snapshot_delete(bs, snapshot_id);
+    }
+    if (bs->file) {
+        return bdrv_snapshot_delete(bs->file, snapshot_id);
+    }
+    return -ENOTSUP;
+}
+
+int bdrv_snapshot_list(BlockDriverState *bs,
+                       QEMUSnapshotInfo **psn_info)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (drv->bdrv_snapshot_list) {
+        return drv->bdrv_snapshot_list(bs, psn_info);
+    }
+    if (bs->file) {
+        return bdrv_snapshot_list(bs->file, psn_info);
+    }
+    return -ENOTSUP;
+}
+
+int bdrv_snapshot_load_tmp(BlockDriverState *bs,
+        const char *snapshot_name)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (!bs->read_only) {
+        return -EINVAL;
+    }
+    if (drv->bdrv_snapshot_load_tmp) {
+        return drv->bdrv_snapshot_load_tmp(bs, snapshot_name);
+    }
+    return -ENOTSUP;
+}
diff --git a/include/block/block.h b/include/block/block.h
index c883857..1ca146a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -7,6 +7,7 @@
 #include "block/coroutine.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi-types.h"
+#include "block/snapshot.h"
 
 /* block.c */
 typedef struct BlockDriver BlockDriver;
@@ -27,17 +28,6 @@ typedef struct BlockFragInfo {
     uint64_t compressed_clusters;
 } BlockFragInfo;
 
-typedef struct QEMUSnapshotInfo {
-    char id_str[128]; /* unique snapshot id */
-    /* the following fields are informative. They are not needed for
-       the consistency of the snapshot */
-    char name[256]; /* user chosen name */
-    uint64_t vm_state_size; /* VM state info size */
-    uint32_t date_sec; /* UTC date of the snapshot */
-    uint32_t date_nsec;
-    uint64_t vm_clock_nsec; /* VM clock relative to boot */
-} QEMUSnapshotInfo;
-
 /* Callbacks for block device models */
 typedef struct BlockDevOps {
     /*
@@ -327,18 +317,8 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
                                     char *dest, size_t sz);
 BlockInfo *bdrv_query_info(BlockDriverState *s);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
-bool bdrv_can_snapshot(BlockDriverState *bs);
-int bdrv_is_snapshot(BlockDriverState *bs);
+
 BlockDriverState *bdrv_snapshots(void);
-int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info);
-int bdrv_snapshot_goto(BlockDriverState *bs,
-                       const char *snapshot_id);
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
-int bdrv_snapshot_list(BlockDriverState *bs,
-                       QEMUSnapshotInfo **psn_info);
-int bdrv_snapshot_load_tmp(BlockDriverState *bs,
-                           const char *snapshot_name);
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size);
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 71b8017..3e1e05b 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -2,12 +2,30 @@
 #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"
+
+typedef struct QEMUSnapshotInfo {
+    char id_str[128]; /* unique snapshot id */
+    /* the following fields are informative. They are not needed for
+       the consistency of the snapshot */
+    char name[256]; /* user chosen name */
+    uint64_t vm_state_size; /* VM state info size */
+    uint32_t date_sec; /* UTC date of the snapshot */
+    uint32_t date_nsec;
+    uint64_t vm_clock_nsec; /* VM clock relative to boot */
+} QEMUSnapshotInfo;
 
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *id, const char *name);
+bool bdrv_can_snapshot(BlockDriverState *bs);
+int bdrv_is_snapshot(BlockDriverState *bs);
+int bdrv_snapshot_create(BlockDriverState *bs,
+                         QEMUSnapshotInfo *sn_info);
+int bdrv_snapshot_goto(BlockDriverState *bs,
+                       const char *snapshot_id);
+int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int bdrv_snapshot_list(BlockDriverState *bs,
+                       QEMUSnapshotInfo **psn_info);
+int bdrv_snapshot_load_tmp(BlockDriverState *bs,
+                           const char *snapshot_name);
+
 #endif
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 19/20] block: move bdrv_snapshot_dump() to block/qapi.c
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (17 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 18/20] block: move snapshot related functions to block/snapshot.c Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 20/20] block: rename bdrv_query_info() to bdrv_query_block_info() Wenchao Xia
  2013-03-07  7:36 ` [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Markus Armbruster
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c                   |   33 ---------------------------------
 block/qapi.c              |   33 +++++++++++++++++++++++++++++++++
 include/block/block.h     |    2 --
 include/block/block_int.h |    1 +
 include/block/qapi.h      |    2 ++
 5 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/block.c b/block.c
index c2ca82a..4d87eec 100644
--- a/block.c
+++ b/block.c
@@ -3263,39 +3263,6 @@ char *get_human_readable_size(char *buf, int buf_size, int64_t size)
     return buf;
 }
 
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
-{
-    char buf1[128], date_buf[128], clock_buf[128];
-    struct tm tm;
-    time_t ti;
-    int64_t secs;
-
-    if (!sn) {
-        snprintf(buf, buf_size,
-                 "%-10s%-20s%7s%20s%15s",
-                 "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
-    } else {
-        ti = sn->date_sec;
-        localtime_r(&ti, &tm);
-        strftime(date_buf, sizeof(date_buf),
-                 "%Y-%m-%d %H:%M:%S", &tm);
-        secs = sn->vm_clock_nsec / 1000000000;
-        snprintf(clock_buf, sizeof(clock_buf),
-                 "%02d:%02d:%02d.%03d",
-                 (int)(secs / 3600),
-                 (int)((secs / 60) % 60),
-                 (int)(secs % 60),
-                 (int)((sn->vm_clock_nsec / 1000000) % 1000));
-        snprintf(buf, buf_size,
-                 "%-10s%-20s%7s%20s%15s",
-                 sn->id_str, sn->name,
-                 get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
-                 date_buf,
-                 clock_buf);
-    }
-    return buf;
-}
-
 /**************************************************************/
 /* async I/Os */
 
diff --git a/block/qapi.c b/block/qapi.c
index f890c1e..0b8f697 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -304,6 +304,39 @@ static void snprintf_tail(char **p_buf, int *p_size, const char *fmt, ...)
     }
 }
 
+char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
+{
+    char buf1[128], date_buf[128], clock_buf[128];
+    struct tm tm;
+    time_t ti;
+    int64_t secs;
+
+    if (!sn) {
+        snprintf(buf, buf_size,
+                 "%-10s%-20s%7s%20s%15s",
+                 "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
+    } else {
+        ti = sn->date_sec;
+        localtime_r(&ti, &tm);
+        strftime(date_buf, sizeof(date_buf),
+                 "%Y-%m-%d %H:%M:%S", &tm);
+        secs = sn->vm_clock_nsec / 1000000000;
+        snprintf(clock_buf, sizeof(clock_buf),
+                 "%02d:%02d:%02d.%03d",
+                 (int)(secs / 3600),
+                 (int)((secs / 60) % 60),
+                 (int)(secs % 60),
+                 (int)((sn->vm_clock_nsec / 1000000) % 1000));
+        snprintf(buf, buf_size,
+                 "%-10s%-20s%7s%20s%15s",
+                 sn->id_str, sn->name,
+                 get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
+                 date_buf,
+                 clock_buf);
+    }
+    return buf;
+}
+
 /* Use buf instead of asprintf to reduce memory fragility. */
 char *bdrv_image_info_dump(char *buf, int buf_size, ImageInfo *info)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 1ca146a..df61beb 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -7,7 +7,6 @@
 #include "block/coroutine.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi-types.h"
-#include "block/snapshot.h"
 
 /* block.c */
 typedef struct BlockDriver BlockDriver;
@@ -319,7 +318,6 @@ BlockInfo *bdrv_query_info(BlockDriverState *s);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
 
 BlockDriverState *bdrv_snapshots(void);
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size);
 int path_is_absolute(const char *path);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index eaad53e..6b19545 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -33,6 +33,7 @@
 #include "qapi/qmp/qerror.h"
 #include "monitor/monitor.h"
 #include "qemu/hbitmap.h"
+#include "block/snapshot.h"
 
 #define BLOCK_FLAG_ENCRYPT          1
 #define BLOCK_FLAG_COMPAT6          4
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 235e6f0..b76c6a5 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -3,6 +3,7 @@
 
 #include "qapi-types.h"
 #include "block/block.h"
+#include "block/snapshot.h"
 
 int bdrv_query_snapshot_info_list(BlockDriverState *bs,
                                   SnapshotInfoList **p_list,
@@ -11,5 +12,6 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
 int bdrv_query_image_info(BlockDriverState *bs,
                           ImageInfo **p_info,
                           Error **errp);
+char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
 char *bdrv_image_info_dump(char *buf, int buf_size, ImageInfo *info);
 #endif
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* [Qemu-devel] [PATCH V8 20/20] block: rename bdrv_query_info() to bdrv_query_block_info()
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (18 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 19/20] block: move bdrv_snapshot_dump() to block/qapi.c Wenchao Xia
@ 2013-03-07  6:07 ` Wenchao Xia
  2013-03-07  7:36 ` [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Markus Armbruster
  20 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-07  6:07 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, aliguori, capitulino, stefanha, armbru, pbonzini, Wenchao Xia

  Now that we have bdrv_query_image_info, rename this function to make it
more obvious what it is doing.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |    4 ++--
 include/block/block.h |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 4d87eec..0a06d1e 100644
--- a/block.c
+++ b/block.c
@@ -2878,7 +2878,7 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
     return data.ret;
 }
 
-BlockInfo *bdrv_query_info(BlockDriverState *bs)
+BlockInfo *bdrv_query_block_info(BlockDriverState *bs)
 {
     BlockInfo *info = g_malloc0(sizeof(*info));
     info->device = g_strdup(bs->device_name);
@@ -2945,7 +2945,7 @@ BlockInfoList *qmp_query_block(Error **errp)
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
         BlockInfoList *info = g_malloc0(sizeof(*info));
-        info->value = bdrv_query_info(bs);
+        info->value = bdrv_query_block_info(bs);
 
         *p_next = info;
         p_next = &info->next;
diff --git a/include/block/block.h b/include/block/block.h
index df61beb..5dbbaef 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -314,7 +314,7 @@ 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);
-BlockInfo *bdrv_query_info(BlockDriverState *s);
+BlockInfo *bdrv_query_block_info(BlockDriverState *s);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
 
 BlockDriverState *bdrv_snapshots(void);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info
  2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
                   ` (19 preceding siblings ...)
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 20/20] block: rename bdrv_query_info() to bdrv_query_block_info() Wenchao Xia
@ 2013-03-07  7:36 ` Markus Armbruster
  2013-03-11  1:24   ` Wenchao Xia
  20 siblings, 1 reply; 42+ messages in thread
From: Markus Armbruster @ 2013-03-07  7:36 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, capitulino, stefanha, qemu-devel, pbonzini

I intend to review at least the QMP interfaces.  Unfortunately, I'm
struggling with getting my review queue under control.  Please be
patient.

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 01/20] build: add block/snapshot.c
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 01/20] build: add block/snapshot.c Wenchao Xia
@ 2013-03-08 20:18   ` Eric Blake
  2013-03-09  4:00     ` Wenchao Xia
  0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2013-03-08 20:18 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This file will have internal snapshot related functions.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---

> --- /dev/null
> +++ b/include/block/snapshot.h
> @@ -0,0 +1,4 @@
> +#ifndef SNAPSHOT_H

Introducing a new file without a copyright notice is a no-no.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 02/20] build: add block/qapi.c
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 02/20] build: add block/qapi.c Wenchao Xia
@ 2013-03-08 20:22   ` Eric Blake
  2013-03-09  4:05     ` Wenchao Xia
  0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2013-03-08 20:22 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This file will have qmp related functions for block. To avoid conflict and
> tip better, macro in header file is BLOCK_QAPI_H instead of QAPI_H.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---

> --- /dev/null
> +++ b/include/block/qapi.h
> @@ -0,0 +1,4 @@
> +#ifndef BLOCK_QAPI_H

Again, introducing a new file without a copyright notice is a no-no.

Why are you adding stub files in two separate patches?  If you're going
to bother with stub files, then add all of them in one commit.  But my
preference would be to add one file at a time, but with contents instead
of being a stub (for example, merge patch 1 and 3, and merge patch 2 and
6).  It just feels like you have subdivided this series too far.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 03/20] block: move bdrv_snapshot_find() to block/snapshot.c
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 03/20] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
@ 2013-03-08 20:27   ` Eric Blake
  2013-03-09  4:17     ` Wenchao Xia
  0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2013-03-08 20:27 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 05/20] qemu-img: remove unused parameter in collect_image_info()
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 05/20] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
@ 2013-03-08 20:34   ` Eric Blake
  2013-03-09  4:18     ` Wenchao Xia
  0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2013-03-08 20:34 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   Parameter *fmt was not used, so remove it.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Typically, these annotations should be kept in chronological order.
That is, the first line should always be a Signed-off-by (you have to
write a patch before anyone else can add their Reviewed-by).  Also, we
tend to avoid blank lines in the attribution section.  So, the preferred
way to write this would be:

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

But that's cosmetic, and my review still stands.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 06/20] block: move collect_snapshots() and collect_image_info() to block/qapi.c
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 06/20] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
@ 2013-03-08 22:04   ` Eric Blake
  2013-03-09  4:20     ` Wenchao Xia
  0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2013-03-08 22:04 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 07/20] block: add snapshot info query function bdrv_query_snapshot_info_list()
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 07/20] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-03-08 22:15   ` Eric Blake
  0 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2013-03-08 22:15 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

On 03/06/2013 11:07 PM, 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         |   52 +++++++++++++++++++++++++++++++++++++------------
>  include/block/qapi.h |    4 ++-
>  qemu-img.c           |    4 ++-
>  3 files changed, 45 insertions(+), 15 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list()
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-03-08 22:55   ` Eric Blake
  2013-03-09  4:24     ` Wenchao Xia
  0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2013-03-08 22:55 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This patch adds a parameter to tell whether return valid snapshots
> for whole VM only.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qapi.c         |   39 +++++++++++++++++++++++++++++++++++++--
>  include/block/qapi.h |    1 +
>  qemu-img.c           |    3 ++-
>  3 files changed, 40 insertions(+), 3 deletions(-)
> 

> + * @sn: snapshot info need to be checked.

s/need //

> + * return 0 if valid, it means load_vmstate() for it could succeed.

Reads awkwardly; how about:

it means load_vmstate() could load that snapshot.

> + */
> +static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, BlockDriverState *bs)
> +{
> +    BlockDriverState *bs1 = NULL;
> +    QEMUSnapshotInfo s, *sn_info = &s;
> +    int ret = 0;
> +
> +    /* Check logic is connected with load_vmstate():
> +       Only check the devices that can snapshot, other devices that can't
> +       take snapshot, for example, readonly ones, will be ignored in
> +       load_vmstate(). */
> +    while ((bs1 = bdrv_next(bs1))) {
> +        if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> +            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
> +            if (ret < 0) {
> +                ret = -1;

This function only returns 0 or -1...

> +/* return 0 on success, and @p_list will be set only on success. If
> +   @vm_snapshot is true, only the snapshot valid for whole vm will be got. */

grammar

If @vm_snapshot is true, limit the results to snapshots valid for the
whole vm.

> -
> +        if (vm_snapshot && snapshot_filter_vm(&sn_tab[i], bs)) {

...yet you are only ever calling it in a boolean context.  Would it be
smarter to have the function return bool (true for valid vm snapshot,
false if the image snapshot is not usable as a vm snapshot)?

Also, it might be nicer to name it snapshot_valid_for_vm, as in:

if (vm_snapshot && !snapshot_valid_for_vm(...)) {
    continue;
}

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 09/20] block: add image info query function bdrv_query_image_info()
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 09/20] block: add image info query function bdrv_query_image_info() Wenchao Xia
@ 2013-03-08 23:08   ` Eric Blake
  2013-03-09  4:26     ` Wenchao Xia
  0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2013-03-08 23:08 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This patch adds function bdrv_query_image_info(), which will
> retrieve image info in qmp object format. The implementation are

s/are/is/

> based on the code moved from qemu-img.c, but use block layer

s/use/uses/

> function to get snapshot info.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qapi.c         |   39 ++++++++++++++++++++++++++++++++-------
>  include/block/qapi.h |    7 +++----
>  qemu-img.c           |    7 ++-----
>  3 files changed, 37 insertions(+), 16 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 10/20] qmp: add interface query-snapshots
  2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 10/20] qmp: add interface query-snapshots Wenchao Xia
@ 2013-03-08 23:30   ` Eric Blake
  2013-03-09  4:28     ` Wenchao Xia
  0 siblings, 1 reply; 42+ messages in thread
From: Eric Blake @ 2013-03-08 23:30 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 2593 bytes --]

On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>   This interface now return valid internal snapshots for whole vm.

s/now return/returns/

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block/qapi.c     |   22 +++++++++++++++++++++
>  qapi-schema.json |   14 +++++++++++++
>  qmp-commands.hx  |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+), 0 deletions(-)
> 

> +
> +SnapshotInfoList *qmp_query_snapshots(Error **errp)
> +{
> +    BlockDriverState *bs;
> +    SnapshotInfoList *list = NULL;
> +    int ret;
> +
> +    /* internal snapshot for whole vm */
> +    bs = bdrv_snapshots();
> +    if (!bs) {
> +        error_setg(errp, "No available block device supports snapshots\n");
> +        return NULL;
> +    }
> +

list is NULL here,

> +    ret = bdrv_query_snapshot_info_list(bs, &list, true, errp);
> +    if (ret < 0) {
> +        qapi_free_SnapshotInfoList(list);

and you documented that bdrv_query_snapshot_info_list leaves list
untouched on error, so why do you have to clean it up?

> +        list = NULL;
> +    }
> +    return list;

In fact, you could write this as:

    bdrv_query_snapshot_info_list(bs, &list, true, errp);
    return list;

without needing 'ret'.

>  ##
> +# @query-snapshots:
> +#
> +# Get a list of internal snapshots for whole virtual machine, only valid

s/for/for the/; s/machine, only/machine. Only/

> +# internal snapshot will be returned, inconsistent ones will be ignored

s/snapshot/snapshots/


>  SQMP
> +query-snapshots
> +-----------

Common practice is to make the divider line match the line length of the
line above (you were short by '----')

> +
> +Show the internal consistent snapshot information
> +
> +Each snapshot is represented by a json-object. The returned value
> +is a json-array of all snapshots
> +
> +Each json-object contain the following:
> +
> +- "id": unique snapshot id (json-string)
> +- "name": internal snapshot name (json-string)
> +- "vm-state-size": size of the VM state in bytes (json-int)
> +- "date-sec": UTC date of the snapshot in seconds (json-int)
> +- "date-nsec": fractional part in nano seconds to be used with

s/nano seconds/nanoseconds/

> +               date-sec(json-int)
> +- "vm-clock-sec": VM clock relative to boot in seconds (json-int)
> +- "vm-clock-nsec": fractional part in nano seconds to be used with

and again

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 01/20] build: add block/snapshot.c
  2013-03-08 20:18   ` Eric Blake
@ 2013-03-09  4:00     ` Wenchao Xia
  2013-03-09 13:10       ` Eric Blake
  0 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-09  4:00 UTC (permalink / raw)
  To: Eric Blake; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini


> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    This file will have internal snapshot related functions.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
> 
>> --- /dev/null
>> +++ b/include/block/snapshot.h
>> @@ -0,0 +1,4 @@
>> +#ifndef SNAPSHOT_H
> 
> Introducing a new file without a copyright notice is a no-no.
> 

  Copy right is notice in snapshot.c, other header files seems
no copy right declaration, what more should be added?

-- 
Best Regards

Wenchao Xia

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 02/20] build: add block/qapi.c
  2013-03-08 20:22   ` Eric Blake
@ 2013-03-09  4:05     ` Wenchao Xia
  0 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-09  4:05 UTC (permalink / raw)
  To: Eric Blake; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

于 2013-3-9 4:22, Eric Blake 写道:
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    This file will have qmp related functions for block. To avoid conflict and
>> tip better, macro in header file is BLOCK_QAPI_H instead of QAPI_H.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
> 
>> --- /dev/null
>> +++ b/include/block/qapi.h
>> @@ -0,0 +1,4 @@
>> +#ifndef BLOCK_QAPI_H
> 
> Again, introducing a new file without a copyright notice is a no-no.
> 
> Why are you adding stub files in two separate patches?  If you're going
> to bother with stub files, then add all of them in one commit.  But my
> preference would be to add one file at a time, but with contents instead
> of being a stub (for example, merge patch 1 and 3, and merge patch 2 and
> 6).  It just feels like you have subdivided this series too far.
> 
  Just to make it easy for review, one patch for one step. I am OK to
merge if you insist, but prefer not for that these stub brings no
trouble, to save effort.



-- 
Best Regards

Wenchao Xia

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 03/20] block: move bdrv_snapshot_find() to block/snapshot.c
  2013-03-08 20:27   ` Eric Blake
@ 2013-03-09  4:17     ` Wenchao Xia
  0 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-09  4:17 UTC (permalink / raw)
  To: Eric Blake; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

 > 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.
>


-- 
Best Regards

Wenchao Xia

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 05/20] qemu-img: remove unused parameter in collect_image_info()
  2013-03-08 20:34   ` Eric Blake
@ 2013-03-09  4:18     ` Wenchao Xia
  0 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-09  4:18 UTC (permalink / raw)
  To: Eric Blake; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

于 2013-3-9 4:34, Eric Blake 写道:
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    Parameter *fmt was not used, so remove it.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> Typically, these annotations should be kept in chronological order.
> That is, the first line should always be a Signed-off-by (you have to
> write a patch before anyone else can add their Reviewed-by).  Also, we
> tend to avoid blank lines in the attribution section.  So, the preferred
> way to write this would be:
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> But that's cosmetic, and my review still stands.
>

OK.

-- 
Best Regards

Wenchao Xia

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 06/20] block: move collect_snapshots() and collect_image_info() to block/qapi.c
  2013-03-08 22:04   ` Eric Blake
@ 2013-03-09  4:20     ` Wenchao Xia
  0 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-09  4:20 UTC (permalink / raw)
  To: Eric Blake; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

于 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.

-- 
Best Regards

Wenchao Xia

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list()
  2013-03-08 22:55   ` Eric Blake
@ 2013-03-09  4:24     ` Wenchao Xia
  0 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-09  4:24 UTC (permalink / raw)
  To: Eric Blake; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

于 2013-3-9 6:55, Eric Blake 写道:
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    This patch adds a parameter to tell whether return valid snapshots
>> for whole VM only.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qapi.c         |   39 +++++++++++++++++++++++++++++++++++++--
>>   include/block/qapi.h |    1 +
>>   qemu-img.c           |    3 ++-
>>   3 files changed, 40 insertions(+), 3 deletions(-)
>>
>
>> + * @sn: snapshot info need to be checked.
>
> s/need //
>
   OK.

>> + * return 0 if valid, it means load_vmstate() for it could succeed.
>
> Reads awkwardly; how about:
>
> it means load_vmstate() could load that snapshot.
>
   I forgot it may not have vmstate() but only only block snapshot,
It should be:
    return 0 if valid, it means the snapshot is consistent for the VM.

>> + */
>> +static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, BlockDriverState *bs)
>> +{
>> +    BlockDriverState *bs1 = NULL;
>> +    QEMUSnapshotInfo s, *sn_info = &s;
>> +    int ret = 0;
>> +
>> +    /* Check logic is connected with load_vmstate():
>> +       Only check the devices that can snapshot, other devices that can't
>> +       take snapshot, for example, readonly ones, will be ignored in
>> +       load_vmstate(). */
>> +    while ((bs1 = bdrv_next(bs1))) {
>> +        if (bdrv_can_snapshot(bs1) && bs1 != bs) {
>> +            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
>> +            if (ret < 0) {
>> +                ret = -1;
>
> This function only returns 0 or -1...
>
   OK, it should be bool.

>> +/* return 0 on success, and @p_list will be set only on success. If
>> +   @vm_snapshot is true, only the snapshot valid for whole vm will be got. */
>
> grammar
>
> If @vm_snapshot is true, limit the results to snapshots valid for the
> whole vm.
>
   Looks better, thanks.

>> -
>> +        if (vm_snapshot && snapshot_filter_vm(&sn_tab[i], bs)) {
>
> ...yet you are only ever calling it in a boolean context.  Would it be
> smarter to have the function return bool (true for valid vm snapshot,
> false if the image snapshot is not usable as a vm snapshot)?
>
> Also, it might be nicer to name it snapshot_valid_for_vm, as in:
>
> if (vm_snapshot && !snapshot_valid_for_vm(...)) {
>      continue;
> }
   OK.

>


-- 
Best Regards

Wenchao Xia

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 09/20] block: add image info query function bdrv_query_image_info()
  2013-03-08 23:08   ` Eric Blake
@ 2013-03-09  4:26     ` Wenchao Xia
  0 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-09  4:26 UTC (permalink / raw)
  To: Eric Blake; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

于 2013-3-9 7:08, Eric Blake 写道:
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    This patch adds function bdrv_query_image_info(), which will
>> retrieve image info in qmp object format. The implementation are
>
> s/are/is/
>
OK.

>> based on the code moved from qemu-img.c, but use block layer
>
> s/use/uses/
>
OK.

>> function to get snapshot info.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qapi.c         |   39 ++++++++++++++++++++++++++++++++-------
>>   include/block/qapi.h |    7 +++----
>>   qemu-img.c           |    7 ++-----
>>   3 files changed, 37 insertions(+), 16 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>


-- 
Best Regards

Wenchao Xia

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 10/20] qmp: add interface query-snapshots
  2013-03-08 23:30   ` Eric Blake
@ 2013-03-09  4:28     ` Wenchao Xia
  0 siblings, 0 replies; 42+ messages in thread
From: Wenchao Xia @ 2013-03-09  4:28 UTC (permalink / raw)
  To: Eric Blake; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

于 2013-3-9 7:30, Eric Blake 写道:
> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>    This interface now return valid internal snapshots for whole vm.
>
> s/now return/returns/
>
   OK.

>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qapi.c     |   22 +++++++++++++++++++++
>>   qapi-schema.json |   14 +++++++++++++
>>   qmp-commands.hx  |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 91 insertions(+), 0 deletions(-)
>>
>
>> +
>> +SnapshotInfoList *qmp_query_snapshots(Error **errp)
>> +{
>> +    BlockDriverState *bs;
>> +    SnapshotInfoList *list = NULL;
>> +    int ret;
>> +
>> +    /* internal snapshot for whole vm */
>> +    bs = bdrv_snapshots();
>> +    if (!bs) {
>> +        error_setg(errp, "No available block device supports snapshots\n");
>> +        return NULL;
>> +    }
>> +
>
> list is NULL here,
>
>> +    ret = bdrv_query_snapshot_info_list(bs, &list, true, errp);
>> +    if (ret < 0) {
>> +        qapi_free_SnapshotInfoList(list);
>
> and you documented that bdrv_query_snapshot_info_list leaves list
> untouched on error, so why do you have to clean it up?
>
>> +        list = NULL;
>> +    }
>> +    return list;
>
> In fact, you could write this as:
>
>      bdrv_query_snapshot_info_list(bs, &list, true, errp);
>      return list;
>
> without needing 'ret'.
>
   Yep, you are right.

>>   ##
>> +# @query-snapshots:
>> +#
>> +# Get a list of internal snapshots for whole virtual machine, only valid
>
> s/for/for the/; s/machine, only/machine. Only/
>
>> +# internal snapshot will be returned, inconsistent ones will be ignored
>
> s/snapshot/snapshots/
>
   OK.

>
>>   SQMP
>> +query-snapshots
>> +-----------
>
> Common practice is to make the divider line match the line length of the
> line above (you were short by '----')
>
>> +
>> +Show the internal consistent snapshot information
>> +
>> +Each snapshot is represented by a json-object. The returned value
>> +is a json-array of all snapshots
>> +
>> +Each json-object contain the following:
>> +
>> +- "id": unique snapshot id (json-string)
>> +- "name": internal snapshot name (json-string)
>> +- "vm-state-size": size of the VM state in bytes (json-int)
>> +- "date-sec": UTC date of the snapshot in seconds (json-int)
>> +- "date-nsec": fractional part in nano seconds to be used with
>
> s/nano seconds/nanoseconds/
>
>> +               date-sec(json-int)
>> +- "vm-clock-sec": VM clock relative to boot in seconds (json-int)
>> +- "vm-clock-nsec": fractional part in nano seconds to be used with
>
> and again
>
OK.


-- 
Best Regards

Wenchao Xia

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 01/20] build: add block/snapshot.c
  2013-03-09  4:00     ` Wenchao Xia
@ 2013-03-09 13:10       ` Eric Blake
  0 siblings, 0 replies; 42+ messages in thread
From: Eric Blake @ 2013-03-09 13:10 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, stefanha, qemu-devel, armbru, pbonzini

[-- Attachment #1: Type: text/plain, Size: 786 bytes --]

On 03/08/2013 09:00 PM, Wenchao Xia wrote:
> 
>> On 03/06/2013 11:07 PM, Wenchao Xia wrote:
>>>    This file will have internal snapshot related functions.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>
>>> --- /dev/null
>>> +++ b/include/block/snapshot.h
>>> @@ -0,0 +1,4 @@
>>> +#ifndef SNAPSHOT_H
>>
>> Introducing a new file without a copyright notice is a no-no.
>>
> 
>   Copy right is notice in snapshot.c, other header files seems
> no copy right declaration, what more should be added?

The copyright of the .c file should be copied to the .h file then.
EVERY file needs a copyright, not just one of a pair of files.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info
  2013-03-07  7:36 ` [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Markus Armbruster
@ 2013-03-11  1:24   ` Wenchao Xia
  2013-03-11  8:34     ` Markus Armbruster
  0 siblings, 1 reply; 42+ messages in thread
From: Wenchao Xia @ 2013-03-11  1:24 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: kwolf, aliguori, capitulino, stefanha, qemu-devel, pbonzini

于 2013-3-7 15:36, Markus Armbruster 写道:
> I intend to review at least the QMP interfaces.  Unfortunately, I'm
> struggling with getting my review queue under control.  Please be
> patient.
> 
  Thanks for your attention. I am coding for next version
to address Eric's comments, please ignore this version to save
time.

-- 
Best Regards

Wenchao Xia

^ permalink raw reply	[flat|nested] 42+ messages in thread

* Re: [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info
  2013-03-11  1:24   ` Wenchao Xia
@ 2013-03-11  8:34     ` Markus Armbruster
  0 siblings, 0 replies; 42+ messages in thread
From: Markus Armbruster @ 2013-03-11  8:34 UTC (permalink / raw)
  To: Wenchao Xia; +Cc: kwolf, aliguori, capitulino, stefanha, qemu-devel, pbonzini

Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:

> 于 2013-3-7 15:36, Markus Armbruster 写道:
>> I intend to review at least the QMP interfaces.  Unfortunately, I'm
>> struggling with getting my review queue under control.  Please be
>> patient.
>> 
>   Thanks for your attention. I am coding for next version
> to address Eric's comments, please ignore this version to save
> time.

Thanks for the heads up!

^ permalink raw reply	[flat|nested] 42+ messages in thread

end of thread, other threads:[~2013-03-11  9:21 UTC | newest]

Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-07  6:07 [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 01/20] build: add block/snapshot.c Wenchao Xia
2013-03-08 20:18   ` Eric Blake
2013-03-09  4:00     ` Wenchao Xia
2013-03-09 13:10       ` Eric Blake
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 02/20] build: add block/qapi.c Wenchao Xia
2013-03-08 20:22   ` Eric Blake
2013-03-09  4:05     ` Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 03/20] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
2013-03-08 20:27   ` Eric Blake
2013-03-09  4:17     ` Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 04/20] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 05/20] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
2013-03-08 20:34   ` Eric Blake
2013-03-09  4:18     ` Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 06/20] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
2013-03-08 22:04   ` Eric Blake
2013-03-09  4:20     ` Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 07/20] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
2013-03-08 22:15   ` Eric Blake
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 08/20] block: add filter for vm snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
2013-03-08 22:55   ` Eric Blake
2013-03-09  4:24     ` Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 09/20] block: add image info query function bdrv_query_image_info() Wenchao Xia
2013-03-08 23:08   ` Eric Blake
2013-03-09  4:26     ` Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 10/20] qmp: add interface query-snapshots Wenchao Xia
2013-03-08 23:30   ` Eric Blake
2013-03-09  4:28     ` Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 11/20] qmp: add interface query-images Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 12/20] hmp: add function hmp_info_snapshots() Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 13/20] hmp: switch snapshot info function to qmp based one Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 14/20] block: move dump_human_image_info() to block/qapi.c Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 15/20] block: dump to buffer for bdrv_image_info_dump() Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 16/20] hmp: add command info images Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 17/20] block: return bool for bdrv_can_snapshot() Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 18/20] block: move snapshot related functions to block/snapshot.c Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 19/20] block: move bdrv_snapshot_dump() to block/qapi.c Wenchao Xia
2013-03-07  6:07 ` [Qemu-devel] [PATCH V8 20/20] block: rename bdrv_query_info() to bdrv_query_block_info() Wenchao Xia
2013-03-07  7:36 ` [Qemu-devel] [PATCH V8 00/20] qmp/hmp interfaces for internal snapshot info Markus Armbruster
2013-03-11  1:24   ` Wenchao Xia
2013-03-11  8:34     ` Markus Armbruster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.