* [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info
@ 2013-04-02 11:47 Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 01/17] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
` (20 more replies)
0 siblings, 21 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, 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
or enchance following interfaces for qemu:
1) qmp: query-block, show info for a block device include image info
Example:
-> { "execute": "query-block" }
<- {
"return":[
{
"io-status": "ok",
"device":"ide0-hd0",
"locked":false,
"removable":false,
"inserted":{
"ro":false,
"drv":"qcow2",
"encrypted":false,
"file":"disks/test.img",
"backing_file_depth":1,
"bps":1000000,
"bps_rd":0,
"bps_wr":0,
"iops":1000000,
"iops_rd":0,
"iops_wr":0,
"image":{
"filename":"disks/test.img",
"format":"qcow2",
"virtual-size":2048000,
"backing_file":"base.img",
"full-backing-filename":"disks/base.img",
"backing-filename-format:"qcow2",
"snapshots":[
{
"id": "1",
"name": "snapshot1",
"vm-state-size": 0,
"date-sec": 10000200,
"date-nsec": 12,
"vm-clock-sec": 206,
"vm-clock-nsec": 30
}
],
"backing-image":{
"filename":"disks/base.img",
"format":"qcow2",
"virtual-size":2048000
}
}
},
"type":"unknown"
},
{
"io-status": "ok",
"device":"ide1-cd0",
"locked":false,
"removable":true,
"type":"unknown"
},
{
"device":"floppy0",
"locked":false,
"removable":true,
"type":"unknown"
},
{
"device":"sd0",
"locked":false,
"removable":true,
"type":"unknown"
}
]
}
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 block, show what got in qmp query-block in monitor
These patches follows the rule that use qmp to retieve information,
hmp layer just does 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.
V9:
General change:
Drop 4 cleaning patches in the tail in laster version, to make the serial
shorter, which can be submitted later.
Address Eric's comments:
1/14: Merge the patch with file adding patch, add copyright note in the
header file, fix the argument number issue in last version.
3/14: Better commit message.
4/14: Merge the patch with file adding patch, add copyright note in the
header file.
6/14: Better document and comments, rename snapshot_filter_vm() to
snapshot_valid_for_vm() and return bool now.
7/14: Better commit message.
8/14: Remove the free of *list on fail calling
bdrv_query_snapshot_info_list(), better commit message and documents.
9/14: Better documents.
V10:
General change:
use original license in new files. Use query-block to show the info
instead of new API query-images, related hmp change in patches 16/17, 17/17.
Address Eric's comments:
6/17: check 'bs1 != bs' first as optimization.
8/17, 10/17, 11/17: new patches recusively show image info in the backing
chain in command query-block.
v11:
General change:
5/17: check if snapshot > 0 on success in caller, add comments on the
function says that caller need to check it on success.
7/17: check if snapshot > 0 on success before set info->has_snapshots.
Address Eric's comments:
5/17: use error_setg_errno instead of strerror().
12/17: also move bdrv_snapshot_dump() since it need to be changed using
GString later.
13/17: using GString as buffer.
14/17: using GString to dump snapshot info.
16/17: using GString to dump image info.
Address Kevin's comments:
11/17: return void for bdrv_query_info().
17/17: spell fix in help message, using parameter "-v" to show
verbose info.
Wenchao Xia (17):
1 block: move bdrv_snapshot_find() to block/snapshot.c
2 block: distinguish id and name in bdrv_find_snapshot()
3 qemu-img: remove unused parameter in collect_image_info()
4 block: move collect_snapshots() and collect_image_info() to block/qapi.c
5 block: add snapshot info query function bdrv_query_snapshot_info_list()
6 block: add check for VM snapshot in bdrv_query_snapshot_info_list()
7 block: add image info query function bdrv_query_image_info()
8 block: move qmp_query_block() and bdrv_query_info() to block/qapi.c
9 qmp: add interface query-snapshots
10 qmp: add recursive member in ImageInfo
11 qmp: add ImageInfo in BlockDeviceInfo used by query-block
12 block: move bdrv_snapshot_dump() and dump_human_image_info() to block/qapi.c
13 block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()
14 hmp: add function hmp_info_snapshots()
15 hmp: switch snapshot info function to qmp based one
16 hmp: show ImageInfo in 'info block'
17 hmp: add parameters device and -v for info block
block.c | 109 ------------
block/Makefile.objs | 1 +
block/qapi.c | 433 ++++++++++++++++++++++++++++++++++++++++++++++
block/snapshot.c | 78 +++++++++
hmp.c | 76 ++++++++
hmp.h | 1 +
include/block/block.h | 2 -
include/block/qapi.h | 43 +++++
include/block/snapshot.h | 37 ++++
monitor.c | 9 +-
qapi-schema.json | 24 +++-
qemu-img.c | 178 ++-----------------
qmp-commands.hx | 122 +++++++++++++-
savevm.c | 96 +----------
14 files changed, 843 insertions(+), 366 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] 67+ messages in thread
* [Qemu-devel] [PATCH V11 01/17] block: move bdrv_snapshot_find() to block/snapshot.c
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
` (19 subsequent siblings)
20 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This patch adds block/snapshot.c and then moves the function
there. It also fixes small code style errors reported by check script.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/Makefile.objs | 1 +
block/snapshot.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
include/block/snapshot.h | 37 +++++++++++++++++++++++++++++++++++
savevm.c | 23 +---------------------
4 files changed, 87 insertions(+), 22 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..c47a899
--- /dev/null
+++ b/block/snapshot.c
@@ -0,0 +1,48 @@
+/*
+ * Block layer snapshot related functions
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#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
new file mode 100644
index 0000000..4ad070c
--- /dev/null
+++ b/include/block/snapshot.h
@@ -0,0 +1,37 @@
+/*
+ * Block layer snapshot related functions
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#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 406caa9..88acc38 100644
--- a/savevm.c
+++ b/savevm.c
@@ -40,6 +40,7 @@
#include "trace.h"
#include "qemu/bitops.h"
#include "qemu/iov.h"
+#include "block/snapshot.h"
#define SELF_ANNOUNCE_ROUNDS 5
@@ -2199,28 +2200,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] 67+ messages in thread
* [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot()
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 01/17] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-10 15:21 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 03/17] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
` (18 subsequent siblings)
20 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
To make it clear about id and name in searching, the API is changed
a bit to distinguish them, and caller can choose to search by id or name.
Searching will be done with higher priority of id. This function also
returns negative value from bdrv_snapshot_list() instead of -ENOENT on
error now.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@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 c47a899..7b2026c 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -24,8 +24,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;
@@ -33,16 +45,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 4ad070c..a047a8e 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -33,5 +33,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 88acc38..1d2da99 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2212,7 +2212,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) {
@@ -2272,7 +2272,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);
@@ -2363,7 +2363,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) {
@@ -2387,7 +2387,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);
@@ -2493,7 +2493,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] 67+ messages in thread
* [Qemu-devel] [PATCH V11 03/17] qemu-img: remove unused parameter in collect_image_info()
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 01/17] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 04/17] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
` (17 subsequent siblings)
20 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
Parameter *fmt was not used, so remove it.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-img.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 31627b0..937ec01 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] 67+ messages in thread
* [Qemu-devel] [PATCH V11 04/17] block: move collect_snapshots() and collect_image_info() to block/qapi.c
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (2 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 03/17] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
` (16 subsequent siblings)
20 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This patch adds block/qapi.c and moves the functions there. To avoid
conflict and tip better, macro in header file is BLOCK_QAPI_H instead
of QAPI_H. The moving is for making review easier, those functions
will be modified and renamed later.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/Makefile.objs | 2 +-
block/qapi.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/block/qapi.h | 35 ++++++++++++++++
qemu-img.c | 86 +--------------------------------------
4 files changed, 146 insertions(+), 84 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..e2b1b6b
--- /dev/null
+++ b/block/qapi.c
@@ -0,0 +1,107 @@
+/*
+ * Block layer qmp related functions
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#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 *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;
+ }
+ }
+}
diff --git a/include/block/qapi.h b/include/block/qapi.h
new file mode 100644
index 0000000..4586578
--- /dev/null
+++ b/include/block/qapi.h
@@ -0,0 +1,35 @@
+/*
+ * Block layer qmp related functions
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#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);
+#endif
diff --git a/qemu-img.c b/qemu-img.c
index 937ec01..a020ccc 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);
+ bdrv_collect_snapshots(bs, info);
elem = g_new0(ImageInfoList, 1);
elem->value = info;
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list()
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (3 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 04/17] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-03 1:17 ` Eric Blake
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
` (15 subsequent siblings)
20 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, 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 | 55 ++++++++++++++++++++++++++++++++++++++-----------
include/block/qapi.h | 4 ++-
qemu-img.c | 5 +++-
3 files changed, 49 insertions(+), 15 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index e2b1b6b..03369a5 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -25,29 +25,56 @@
#include "block/qapi.h"
#include "block/block_int.h"
-void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info)
+/*
+ * return 0 on success, @p_list will be set only on success, and caller need to
+ * check *p_list 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_errno(errp, -sn_count,
+ "Can't list snapshots of device '%s'", dev);
+ 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;
@@ -56,6 +83,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 4586578..91dc41b 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -28,7 +28,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 a020ccc..51043ef 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1735,7 +1735,10 @@ static ImageInfoList *collect_image_info_list(const char *filename,
info = g_new0(ImageInfo, 1);
bdrv_collect_image_info(bs, info, filename);
- bdrv_collect_snapshots(bs, info);
+ if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
+ info->snapshots) {
+ info->has_snapshots = true;
+ }
elem = g_new0(ImageInfoList, 1);
elem->value = info;
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH V11 06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list()
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (4 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-10 15:19 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 07/17] block: add image info query function bdrv_query_image_info() Wenchao Xia
` (14 subsequent siblings)
20 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.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 03369a5..19d4d93 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -23,14 +23,47 @@
*/
#include "block/qapi.h"
+#include "block/snapshot.h"
#include "block/block_int.h"
/*
+ * check whether the snapshot is valid for whole vm.
+ *
+ * @sn: snapshot info to be checked.
+ * @bs: where @sn was found.
+ *
+ * return true if the snapshot is consistent for the VM.
+ */
+static bool snapshot_valid_for_vm(const QEMUSnapshotInfo *sn,
+ BlockDriverState *bs)
+{
+ BlockDriverState *bs1 = NULL;
+ QEMUSnapshotInfo s, *sn_info = &s;
+ int ret;
+
+ /* 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 (bs1 != bs && bdrv_can_snapshot(bs1)) {
+ ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
+ if (ret < 0) {
+ return false;
+ }
+ }
+ }
+ return true;
+}
+
+/*
* return 0 on success, @p_list will be set only on success, and caller need to
- * check *p_list on success.
+ * check *p_list on success. If @vm_snapshot is true, limit the results to
+ * snapshots valid for the whole VM.
*/
int bdrv_query_snapshot_info_list(BlockDriverState *bs,
SnapshotInfoList **p_list,
+ bool vm_snapshot,
Error **errp)
{
int i, sn_count;
@@ -59,7 +92,9 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
}
for (i = 0; i < sn_count; i++) {
-
+ if (vm_snapshot && !snapshot_valid_for_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 91dc41b..fe66053 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -30,6 +30,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 51043ef..261c277 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);
- if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
+ if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
+ false, NULL) &&
info->snapshots) {
info->has_snapshots = true;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH V11 07/17] block: add image info query function bdrv_query_image_info()
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (5 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-10 15:28 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 08/17] block: move qmp_query_block() and bdrv_query_info() to block/qapi.c Wenchao Xia
` (13 subsequent siblings)
20 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This patch adds function bdrv_query_image_info(), which will
retrieve image info in qmp object format. The implementation is
based on the code moved from qemu-img.c, but uses block layer
function to get snapshot info.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/qapi.c | 41 ++++++++++++++++++++++++++++++++++-------
include/block/qapi.h | 6 +++---
qemu-img.c | 8 ++------
3 files changed, 39 insertions(+), 16 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 19d4d93..176a479 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -122,18 +122,22 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
return 0;
}
-void bdrv_collect_image_info(BlockDriverState *bs,
- ImageInfo *info,
- const char *filename)
+/* 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);
@@ -150,8 +154,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,
@@ -168,4 +172,27 @@ 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:
+ if (info->snapshots) {
+ 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 fe66053..2c62fdf 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -32,7 +32,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);
+int bdrv_query_image_info(BlockDriverState *bs,
+ ImageInfo **p_info,
+ Error **errp);
#endif
diff --git a/qemu-img.c b/qemu-img.c
index 261c277..1dd0a60 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1733,12 +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);
- if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
- false, NULL) &&
- info->snapshots) {
- 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] 67+ messages in thread
* [Qemu-devel] [PATCH V11 08/17] block: move qmp_query_block() and bdrv_query_info() to block/qapi.c
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (6 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 07/17] block: add image info query function bdrv_query_image_info() Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots Wenchao Xia
` (12 subsequent siblings)
20 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This is a code move patch, except in qmp_query_block bdrv_next(bs)
is used instead of direct traverse of global array 'bdrv_states'.
This patch also fix code style error reported by check script.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block.c | 76 ------------------------------------------------
block/qapi.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 1 -
include/block/qapi.h | 1 +
4 files changed, 78 insertions(+), 77 deletions(-)
diff --git a/block.c b/block.c
index 0ae2e93..288cacd 100644
--- a/block.c
+++ b/block.c
@@ -3019,82 +3019,6 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
return data.ret;
}
-BlockInfo *bdrv_query_info(BlockDriverState *bs)
-{
- BlockInfo *info = g_malloc0(sizeof(*info));
- info->device = g_strdup(bs->device_name);
- info->type = g_strdup("unknown");
- info->locked = bdrv_dev_is_medium_locked(bs);
- info->removable = bdrv_dev_has_removable_media(bs);
-
- if (bdrv_dev_has_removable_media(bs)) {
- info->has_tray_open = true;
- info->tray_open = bdrv_dev_is_tray_open(bs);
- }
-
- if (bdrv_iostatus_is_enabled(bs)) {
- info->has_io_status = true;
- info->io_status = bs->iostatus;
- }
-
- if (bs->dirty_bitmap) {
- info->has_dirty = true;
- info->dirty = g_malloc0(sizeof(*info->dirty));
- info->dirty->count = bdrv_get_dirty_count(bs) * BDRV_SECTOR_SIZE;
- info->dirty->granularity =
- ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bs->dirty_bitmap));
- }
-
- if (bs->drv) {
- info->has_inserted = true;
- info->inserted = g_malloc0(sizeof(*info->inserted));
- info->inserted->file = g_strdup(bs->filename);
- info->inserted->ro = bs->read_only;
- info->inserted->drv = g_strdup(bs->drv->format_name);
- info->inserted->encrypted = bs->encrypted;
- info->inserted->encryption_key_missing = bdrv_key_required(bs);
-
- if (bs->backing_file[0]) {
- info->inserted->has_backing_file = true;
- info->inserted->backing_file = g_strdup(bs->backing_file);
- }
-
- info->inserted->backing_file_depth = bdrv_get_backing_file_depth(bs);
-
- if (bs->io_limits_enabled) {
- info->inserted->bps =
- bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL];
- info->inserted->bps_rd =
- bs->io_limits.bps[BLOCK_IO_LIMIT_READ];
- info->inserted->bps_wr =
- bs->io_limits.bps[BLOCK_IO_LIMIT_WRITE];
- info->inserted->iops =
- bs->io_limits.iops[BLOCK_IO_LIMIT_TOTAL];
- info->inserted->iops_rd =
- bs->io_limits.iops[BLOCK_IO_LIMIT_READ];
- info->inserted->iops_wr =
- bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
- }
- }
- return info;
-}
-
-BlockInfoList *qmp_query_block(Error **errp)
-{
- BlockInfoList *head = NULL, **p_next = &head;
- BlockDriverState *bs;
-
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
- BlockInfoList *info = g_malloc0(sizeof(*info));
- info->value = bdrv_query_info(bs);
-
- *p_next = info;
- p_next = &info->next;
- }
-
- return head;
-}
-
BlockStats *bdrv_query_stats(const BlockDriverState *bs)
{
BlockStats *s;
diff --git a/block/qapi.c b/block/qapi.c
index 176a479..6e0c7c3 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -25,6 +25,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.
@@ -196,3 +197,79 @@ int bdrv_query_image_info(BlockDriverState *bs,
*p_info = info;
return 0;
}
+
+BlockInfo *bdrv_query_info(BlockDriverState *bs)
+{
+ BlockInfo *info = g_malloc0(sizeof(*info));
+ info->device = g_strdup(bs->device_name);
+ info->type = g_strdup("unknown");
+ info->locked = bdrv_dev_is_medium_locked(bs);
+ info->removable = bdrv_dev_has_removable_media(bs);
+
+ if (bdrv_dev_has_removable_media(bs)) {
+ info->has_tray_open = true;
+ info->tray_open = bdrv_dev_is_tray_open(bs);
+ }
+
+ if (bdrv_iostatus_is_enabled(bs)) {
+ info->has_io_status = true;
+ info->io_status = bs->iostatus;
+ }
+
+ if (bs->dirty_bitmap) {
+ info->has_dirty = true;
+ info->dirty = g_malloc0(sizeof(*info->dirty));
+ info->dirty->count = bdrv_get_dirty_count(bs) * BDRV_SECTOR_SIZE;
+ info->dirty->granularity =
+ ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bs->dirty_bitmap));
+ }
+
+ if (bs->drv) {
+ info->has_inserted = true;
+ info->inserted = g_malloc0(sizeof(*info->inserted));
+ info->inserted->file = g_strdup(bs->filename);
+ info->inserted->ro = bs->read_only;
+ info->inserted->drv = g_strdup(bs->drv->format_name);
+ info->inserted->encrypted = bs->encrypted;
+ info->inserted->encryption_key_missing = bdrv_key_required(bs);
+
+ if (bs->backing_file[0]) {
+ info->inserted->has_backing_file = true;
+ info->inserted->backing_file = g_strdup(bs->backing_file);
+ }
+
+ info->inserted->backing_file_depth = bdrv_get_backing_file_depth(bs);
+
+ if (bs->io_limits_enabled) {
+ info->inserted->bps =
+ bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL];
+ info->inserted->bps_rd =
+ bs->io_limits.bps[BLOCK_IO_LIMIT_READ];
+ info->inserted->bps_wr =
+ bs->io_limits.bps[BLOCK_IO_LIMIT_WRITE];
+ info->inserted->iops =
+ bs->io_limits.iops[BLOCK_IO_LIMIT_TOTAL];
+ info->inserted->iops_rd =
+ bs->io_limits.iops[BLOCK_IO_LIMIT_READ];
+ info->inserted->iops_wr =
+ bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
+ }
+ }
+ return info;
+}
+
+BlockInfoList *qmp_query_block(Error **errp)
+{
+ BlockInfoList *head = NULL, **p_next = &head;
+ BlockDriverState *bs = NULL;
+
+ while ((bs = bdrv_next(bs))) {
+ BlockInfoList *info = g_malloc0(sizeof(*info));
+ info->value = bdrv_query_info(bs);
+
+ *p_next = info;
+ p_next = &info->next;
+ }
+
+ return head;
+}
diff --git a/include/block/block.h b/include/block/block.h
index 9dc6aad..a319f0a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -326,7 +326,6 @@ 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);
BlockStats *bdrv_query_stats(const BlockDriverState *bs);
int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_is_snapshot(BlockDriverState *bs);
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 2c62fdf..0039a70 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -35,4 +35,5 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
int bdrv_query_image_info(BlockDriverState *bs,
ImageInfo **p_info,
Error **errp);
+BlockInfo *bdrv_query_info(BlockDriverState *bs);
#endif
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (7 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 08/17] block: move qmp_query_block() and bdrv_query_info() to block/qapi.c Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-08 13:28 ` Stefan Hajnoczi
2013-04-10 15:51 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 10/17] qmp: add recursive member in ImageInfo Wenchao Xia
` (11 subsequent siblings)
20 siblings, 2 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This interface returns info of valid internal snapshots for whole vm.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/qapi.c | 17 ++++++++++++++++
qapi-schema.json | 14 +++++++++++++
qmp-commands.hx | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 6e0c7c3..5e91ab8 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -258,6 +258,23 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs)
return info;
}
+SnapshotInfoList *qmp_query_snapshots(Error **errp)
+{
+ BlockDriverState *bs;
+ SnapshotInfoList *list = NULL;
+
+ /* internal snapshot for whole vm */
+ bs = bdrv_snapshots();
+ if (!bs) {
+ error_setg(errp, "No available block device supports snapshots\n");
+ return NULL;
+ }
+
+ bdrv_query_snapshot_info_list(bs, &list, true, errp);
+
+ return list;
+}
+
BlockInfoList *qmp_query_block(Error **errp)
{
BlockInfoList *head = NULL, **p_next = &head;
diff --git a/qapi-schema.json b/qapi-schema.json
index f629a24..225afef 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -841,6 +841,20 @@
{ 'command': 'query-block', 'returns': ['BlockInfo'] }
##
+# @query-snapshots:
+#
+# Get a list of internal snapshots for the whole virtual machine. Only valid
+# internal snapshots 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 1e0e11e..6b20684 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1765,6 +1765,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 nanoseconds 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 nanoseconds 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] 67+ messages in thread
* [Qemu-devel] [PATCH V11 10/17] qmp: add recursive member in ImageInfo
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (8 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-10 16:06 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block Wenchao Xia
` (10 subsequent siblings)
20 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
New member *backing-image is added to reflect the backing chain
status.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qapi.c | 6 +++++-
qapi-schema.json | 5 ++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 5e91ab8..fa61c85 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -123,7 +123,11 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
return 0;
}
-/* return 0 on success, and @p_info will be set only on success. */
+/*
+ * return 0 on success, and @p_info will be set only on success,
+ * (*pinfo)->has_backing_image will be false and (*pinfo)->backing_image will
+ * be NULL.
+ */
int bdrv_query_image_info(BlockDriverState *bs,
ImageInfo **p_info,
Error **errp)
diff --git a/qapi-schema.json b/qapi-schema.json
index 225afef..ad9dd82 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -233,6 +233,8 @@
#
# @snapshots: #optional list of VM snapshots
#
+# @backing-image: #optional info of the backing image (since 1.5)
+#
# Since: 1.3
#
##
@@ -242,7 +244,8 @@
'*actual-size': 'int', 'virtual-size': 'int',
'*cluster-size': 'int', '*encrypted': 'bool',
'*backing-filename': 'str', '*full-backing-filename': 'str',
- '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'] } }
+ '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
+ '*backing-image': 'ImageInfo' } }
##
# @ImageCheck:
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (9 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 10/17] qmp: add recursive member in ImageInfo Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-08 10:04 ` Kevin Wolf
` (2 more replies)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 12/17] block: move bdrv_snapshot_dump() and dump_human_image_info() to block/qapi.c Wenchao Xia
` (9 subsequent siblings)
20 siblings, 3 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
Now image info will be retrieved as an embbed json object inside
BlockDeviceInfo, backing chain info and all related internal snapshot
info can be got in the enhanced recursive structure of ImageInfo.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/qapi.c | 38 ++++++++++++++++++++++++++--
include/block/qapi.h | 4 ++-
qapi-schema.json | 5 +++-
qmp-commands.hx | 67 +++++++++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 108 insertions(+), 6 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index fa61c85..e98a028 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -202,9 +202,14 @@ int bdrv_query_image_info(BlockDriverState *bs,
return 0;
}
-BlockInfo *bdrv_query_info(BlockDriverState *bs)
+/* @p_info will be set only on success. */
+void bdrv_query_info(BlockDriverState *bs,
+ BlockInfo **p_info,
+ Error **errp)
{
BlockInfo *info = g_malloc0(sizeof(*info));
+ BlockDriverState *bs0;
+ ImageInfo **p_image_info;
info->device = g_strdup(bs->device_name);
info->type = g_strdup("unknown");
info->locked = bdrv_dev_is_medium_locked(bs);
@@ -258,8 +263,28 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs)
info->inserted->iops_wr =
bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
}
+
+ bs0 = bs;
+ p_image_info = &info->inserted->image;
+ while (1) {
+ if (bdrv_query_image_info(bs0, p_image_info, errp)) {
+ goto err;
+ }
+ if (bs0->drv && bs0->backing_hd) {
+ bs0 = bs0->backing_hd;
+ (*p_image_info)->has_backing_image = true;
+ p_image_info = &((*p_image_info)->backing_image);
+ } else {
+ break;
+ }
+ }
}
- return info;
+
+ *p_info = info;
+ return;
+
+ err:
+ qapi_free_BlockInfo(info);
}
SnapshotInfoList *qmp_query_snapshots(Error **errp)
@@ -286,11 +311,18 @@ BlockInfoList *qmp_query_block(Error **errp)
while ((bs = bdrv_next(bs))) {
BlockInfoList *info = g_malloc0(sizeof(*info));
- info->value = bdrv_query_info(bs);
+ bdrv_query_info(bs, &info->value, errp);
+ if (error_is_set(errp)) {
+ goto err;
+ }
*p_next = info;
p_next = &info->next;
}
return head;
+
+ err:
+ qapi_free_BlockInfoList(head);
+ return NULL;
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 0039a70..e0fd0a5 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -35,5 +35,7 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
int bdrv_query_image_info(BlockDriverState *bs,
ImageInfo **p_info,
Error **errp);
-BlockInfo *bdrv_query_info(BlockDriverState *bs);
+void bdrv_query_info(BlockDriverState *bs,
+ BlockInfo **p_info,
+ Error **errp);
#endif
diff --git a/qapi-schema.json b/qapi-schema.json
index ad9dd82..02dabc3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -756,6 +756,8 @@
#
# @iops_wr: write I/O operations per second is specified
#
+# @image: the info of image used (since: 1.5)
+#
# Since: 0.14.0
#
# Notes: This interface is only found in @BlockInfo.
@@ -765,7 +767,8 @@
'*backing_file': 'str', 'backing_file_depth': 'int',
'encrypted': 'bool', 'encryption_key_missing': 'bool',
'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
- 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
+ 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
+ 'image': 'ImageInfo' } }
##
# @BlockDeviceIoStatus:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6b20684..b856be7 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1703,6 +1703,47 @@ Each json-object contain the following:
- "iops": limit total I/O operations per second (json-int)
- "iops_rd": limit read operations per second (json-int)
- "iops_wr": limit write operations per second (json-int)
+ - "image": the detail of the image, it is a json-object containing
+ the following:
+ - "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, not present if image
+ format does not support it (json-int, optional)
+ - "encrypted": true if the image is encrypted, not present means
+ false or the image format does not support
+ encryption (json-bool, optional)
+ - "backing_file": backing file name, not present means no backing
+ file is used or the image format does not
+ support backing file chain
+ (json-string, optional)
+ - "full-backing-filename": full path of the backing file, not
+ present if it equals backing_file or no
+ backing file is used
+ (json-string, optional)
+ - "backing-filename-format": the format of the backing file, not
+ present means unknown or no 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 nanoseconds 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 nanoseconds to be used
+ with vm-clock-sec (json-int)
+ - "backing-image": the detail of the backing image, it is an
+ optional json-object only present when a
+ backing image present for this image
- "io-status": I/O operation status, only present if the device supports it
and the VM is configured to stop on errors. It's always reset
@@ -1724,13 +1765,37 @@ Example:
"drv":"qcow2",
"encrypted":false,
"file":"disks/test.img",
- "backing_file_depth":0,
+ "backing_file_depth":1,
"bps":1000000,
"bps_rd":0,
"bps_wr":0,
"iops":1000000,
"iops_rd":0,
"iops_wr":0,
+ "image":{
+ "filename":"disks/test.img",
+ "format":"qcow2",
+ "virtual-size":2048000,
+ "backing_file":"base.img",
+ "full-backing-filename":"disks/base.img",
+ "backing-filename-format:"qcow2",
+ "snapshots":[
+ {
+ "id": "1",
+ "name": "snapshot1",
+ "vm-state-size": 0,
+ "date-sec": 10000200,
+ "date-nsec": 12,
+ "vm-clock-sec": 206,
+ "vm-clock-nsec": 30
+ }
+ ],
+ "backing-image":{
+ "filename":"disks/base.img",
+ "format":"qcow2",
+ "virtual-size":2048000
+ }
+ }
},
"type":"unknown"
},
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH V11 12/17] block: move bdrv_snapshot_dump() and dump_human_image_info() to block/qapi.c
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (10 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-10 16:49 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
` (8 subsequent siblings)
20 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
They are needed later in hmp command, dump_human_image_info()
is renamed to bdrv_image_info_dump().
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block.c | 33 ----------------
block/qapi.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 1 -
include/block/qapi.h | 2 +
qemu-img.c | 69 +---------------------------------
savevm.c | 1 +
6 files changed, 104 insertions(+), 102 deletions(-)
diff --git a/block.c b/block.c
index 288cacd..cd4bc7e 100644
--- a/block.c
+++ b/block.c
@@ -3433,39 +3433,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 e98a028..df63c97 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -326,3 +326,103 @@ BlockInfoList *qmp_query_block(Error **errp)
qapi_free_BlockInfoList(head);
return NULL;
}
+
+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;
+}
+
+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/block.h b/include/block/block.h
index a319f0a..ce28241 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -339,7 +339,6 @@ 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);
int path_is_absolute(const char *path);
diff --git a/include/block/qapi.h b/include/block/qapi.h
index e0fd0a5..237660f 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -38,4 +38,6 @@ int bdrv_query_image_info(BlockDriverState *bs,
void bdrv_query_info(BlockDriverState *bs,
BlockInfo **p_info,
Error **errp);
+char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
+void bdrv_image_info_dump(ImageInfo *info);
#endif
diff --git a/qemu-img.c b/qemu-img.c
index 1dd0a60..5b229a9 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);
}
}
diff --git a/savevm.c b/savevm.c
index 1d2da99..e4e0008 100644
--- a/savevm.c
+++ b/savevm.c
@@ -41,6 +41,7 @@
#include "qemu/bitops.h"
#include "qemu/iov.h"
#include "block/snapshot.h"
+#include "block/qapi.h"
#define SELF_ANNOUNCE_ROUNDS 5
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (11 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 12/17] block: move bdrv_snapshot_dump() and dump_human_image_info() to block/qapi.c Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-08 13:36 ` Stefan Hajnoczi
2013-04-10 17:05 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 14/17] hmp: add function hmp_info_snapshots() Wenchao Xia
` (7 subsequent siblings)
20 siblings, 2 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This patch would allow hmp use them just like qemu-img, and avoid
string truncation.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/qapi.c | 65 +++++++++++++++++++++++++++-----------------------
include/block/qapi.h | 4 +-
qemu-img.c | 22 ++++++++++++----
savevm.c | 11 ++++++--
4 files changed, 61 insertions(+), 41 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index df63c97..e27519e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -327,7 +327,7 @@ BlockInfoList *qmp_query_block(Error **errp)
return NULL;
}
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
+void bdrv_snapshot_dump(GString *buf, QEMUSnapshotInfo *sn)
{
char buf1[128], date_buf[128], clock_buf[128];
struct tm tm;
@@ -335,9 +335,8 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
int64_t secs;
if (!sn) {
- snprintf(buf, buf_size,
- "%-10s%-20s%7s%20s%15s",
- "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
+ g_string_append_printf(buf, "%-10s%-20s%7s%20s%15s",
+ "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
} else {
ti = sn->date_sec;
localtime_r(&ti, &tm);
@@ -350,17 +349,17 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
(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);
+ g_string_append_printf(buf,
+ "%-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;
}
-void bdrv_image_info_dump(ImageInfo *info)
+void bdrv_image_info_dump(GString *buf, ImageInfo *info)
{
char size_buf[128], dsize_buf[128];
if (!info->has_actual_size) {
@@ -370,43 +369,48 @@ 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);
+ g_string_append_printf(buf,
+ "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");
+ g_string_append_printf(buf, "encrypted: yes\n");
}
if (info->has_cluster_size) {
- printf("cluster_size: %" PRId64 "\n", info->cluster_size);
+ g_string_append_printf(buf, "cluster_size: %" PRId64 "\n",
+ info->cluster_size);
}
if (info->has_dirty_flag && info->dirty_flag) {
- printf("cleanly shut down: no\n");
+ g_string_append_printf(buf, "cleanly shut down: no\n");
}
if (info->has_backing_filename) {
- printf("backing file: %s", info->backing_filename);
+ g_string_append_printf(buf, "backing file: %s",
+ info->backing_filename);
if (info->has_full_backing_filename) {
- printf(" (actual path: %s)", info->full_backing_filename);
+ g_string_append_printf(buf, " (actual path: %s)",
+ info->full_backing_filename);
}
- putchar('\n');
+ g_string_append_printf(buf, "\n");
if (info->has_backing_filename_format) {
- printf("backing file format: %s\n", info->backing_filename_format);
+ g_string_append_printf(buf, "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));
+ g_string_append_printf(buf, "Snapshot list:\n");
+ bdrv_snapshot_dump(buf, NULL);
+ g_string_append_printf(buf, "\n");
/* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
* we convert to the block layer's native QEMUSnapshotInfo for now.
@@ -422,7 +426,8 @@ 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));
+ bdrv_snapshot_dump(buf, &sn);
+ g_string_append_printf(buf, "\n");
}
}
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 237660f..dc1e090 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -38,6 +38,6 @@ int bdrv_query_image_info(BlockDriverState *bs,
void bdrv_query_info(BlockDriverState *bs,
BlockInfo **p_info,
Error **errp);
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
-void bdrv_image_info_dump(ImageInfo *info);
+void bdrv_snapshot_dump(GString *buf, QEMUSnapshotInfo *sn);
+void bdrv_image_info_dump(GString *buf, ImageInfo *info);
#endif
diff --git a/qemu-img.c b/qemu-img.c
index 5b229a9..032f68c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1558,18 +1558,24 @@ static void dump_snapshots(BlockDriverState *bs)
{
QEMUSnapshotInfo *sn_tab, *sn;
int nb_sns, i;
- char buf[256];
+ GString *buf = g_string_new(NULL);
nb_sns = bdrv_snapshot_list(bs, &sn_tab);
if (nb_sns <= 0)
return;
- printf("Snapshot list:\n");
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ g_string_append_printf(buf, "Snapshot list:\n");
+ bdrv_snapshot_dump(buf, NULL);
+ g_string_append_printf(buf, "\n");
for(i = 0; i < nb_sns; i++) {
sn = &sn_tab[i];
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+ bdrv_snapshot_dump(buf, sn);
+ g_string_append_printf(buf, "\n");
}
+
+ printf("%s", buf->str);
+ g_string_free(buf, true);
g_free(sn_tab);
+
}
static void dump_json_image_info_list(ImageInfoList *list)
@@ -1610,15 +1616,19 @@ static void dump_human_image_info_list(ImageInfoList *list)
{
ImageInfoList *elem;
bool delim = false;
+ GString *buf = g_string_new(NULL);
for (elem = list; elem; elem = elem->next) {
if (delim) {
- printf("\n");
+ g_string_append_printf(buf, "\n");
}
delim = true;
- bdrv_image_info_dump(elem->value);
+ bdrv_image_info_dump(buf, elem->value);
}
+
+ printf("%s", buf->str);
+ g_string_free(buf, true);
}
static gboolean str_equal_func(gconstpointer a, gconstpointer b)
diff --git a/savevm.c b/savevm.c
index e4e0008..ce0bbe1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2466,7 +2466,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
int nb_sns, i, ret, available;
int total;
int *available_snapshots;
- char buf[256];
+ GString *buf = NULL;
bs = bdrv_snapshots();
if (!bs) {
@@ -2509,11 +2509,16 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
}
if (total > 0) {
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ buf = g_string_new(NULL);
+ bdrv_snapshot_dump(buf, NULL);
+ g_string_append_printf(buf, "\n");
for (i = 0; i < total; i++) {
sn = &sn_tab[available_snapshots[i]];
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+ bdrv_snapshot_dump(buf, sn);
+ g_string_append_printf(buf, "\n");
}
+ monitor_printf(mon, "%s", buf->str);
+ g_string_free(buf, true);
} else {
monitor_printf(mon, "There is no suitable snapshot available\n");
}
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH V11 14/17] hmp: add function hmp_info_snapshots()
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (12 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 15/17] hmp: switch snapshot info function to qmp based one Wenchao Xia
` (6 subsequent siblings)
20 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
hmp.h | 1 +
2 files changed, 49 insertions(+), 0 deletions(-)
diff --git a/hmp.c b/hmp.c
index dbe9b90..89e1aaf 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)
{
@@ -653,6 +654,53 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
qapi_free_TPMInfoList(info_list);
}
+/* assume list is valid */
+static void monitor_dump_snapshotinfolist(Monitor *mon, SnapshotInfoList *list)
+{
+ SnapshotInfoList *elem;
+ GString *buf = g_string_sized_new(1024);
+
+ bdrv_snapshot_dump(buf, NULL);
+ g_string_append_printf(buf, "\n");
+
+ 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);
+ bdrv_snapshot_dump(buf, &sn);
+ g_string_append_printf(buf, "\n");
+ }
+
+ monitor_printf(mon, "%s", buf->str);
+ g_string_free(buf, true);
+}
+
+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 80e8b41..1172c47 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_tpm(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] 67+ messages in thread
* [Qemu-devel] [PATCH V11 15/17] hmp: switch snapshot info function to qmp based one
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (13 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 14/17] hmp: add function hmp_info_snapshots() Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 16/17] hmp: show ImageInfo in 'info block' Wenchao Xia
` (5 subsequent siblings)
20 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
monitor.c | 2 +-
savevm.c | 69 -------------------------------------------------------------
2 files changed, 1 insertions(+), 70 deletions(-)
diff --git a/monitor.c b/monitor.c
index 4ec1db9..4720207 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2621,7 +2621,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 ce0bbe1..ca1e393 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2459,75 +2459,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;
- GString *buf = NULL;
-
- 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) {
- buf = g_string_new(NULL);
- bdrv_snapshot_dump(buf, NULL);
- g_string_append_printf(buf, "\n");
- for (i = 0; i < total; i++) {
- sn = &sn_tab[available_snapshots[i]];
- bdrv_snapshot_dump(buf, sn);
- g_string_append_printf(buf, "\n");
- }
- monitor_printf(mon, "%s", buf->str);
- g_string_free(buf, true);
- } 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] 67+ messages in thread
* [Qemu-devel] [PATCH V11 16/17] hmp: show ImageInfo in 'info block'
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (14 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 15/17] hmp: switch snapshot info function to qmp based one Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 17/17] hmp: add parameters device and -v for info block Wenchao Xia
` (4 subsequent siblings)
20 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
Now human monitor can show image details include internal
snapshot info for every block device.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
hmp.c | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/hmp.c b/hmp.c
index 89e1aaf..6f93fcd 100644
--- a/hmp.c
+++ b/hmp.c
@@ -278,6 +278,8 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
void hmp_info_block(Monitor *mon, const QDict *qdict)
{
BlockInfoList *block_list, *info;
+ ImageInfo *image_info;
+ GString *buf = NULL;
block_list = qmp_query_block(NULL);
@@ -319,6 +321,23 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
info->value->inserted->iops,
info->value->inserted->iops_rd,
info->value->inserted->iops_wr);
+
+ if (!buf) {
+ buf = g_string_sized_new(2048);
+ }
+ monitor_printf(mon, " images:\n");
+ image_info = info->value->inserted->image;
+ while (1) {
+ bdrv_image_info_dump(buf, image_info);
+ monitor_printf(mon, "%s", buf->str);
+ g_string_set_size(buf, 0);
+ if (image_info->has_backing_image) {
+ image_info = image_info->backing_image;
+ } else {
+ break;
+ }
+ }
+
} else {
monitor_printf(mon, " [not inserted]");
}
@@ -326,6 +345,9 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "\n");
}
+ if (buf) {
+ g_string_free(buf, true);
+ }
qapi_free_BlockInfoList(block_list);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* [Qemu-devel] [PATCH V11 17/17] hmp: add parameters device and -v for info block
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (15 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 16/17] hmp: show ImageInfo in 'info block' Wenchao Xia
@ 2013-04-02 11:47 ` Wenchao Xia
2013-04-07 5:54 ` [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (3 subsequent siblings)
20 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-02 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
With these parameters, user can choose the information to be showed,
to avoid message flood in the montior.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
hmp.c | 34 ++++++++++++++++++++--------------
monitor.c | 7 ++++---
2 files changed, 24 insertions(+), 17 deletions(-)
diff --git a/hmp.c b/hmp.c
index 6f93fcd..677bc7f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -280,10 +280,15 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
BlockInfoList *block_list, *info;
ImageInfo *image_info;
GString *buf = NULL;
+ const char *device = qdict_get_try_str(qdict, "device");
+ int verbose = qdict_get_try_bool(qdict, "verbose", 0);
block_list = qmp_query_block(NULL);
for (info = block_list; info; info = info->next) {
+ if (device && strcmp(device, info->value->device)) {
+ continue;
+ }
monitor_printf(mon, "%s: removable=%d",
info->value->device, info->value->removable);
@@ -322,22 +327,23 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
info->value->inserted->iops_rd,
info->value->inserted->iops_wr);
- if (!buf) {
- buf = g_string_sized_new(2048);
- }
- monitor_printf(mon, " images:\n");
- image_info = info->value->inserted->image;
- while (1) {
- bdrv_image_info_dump(buf, image_info);
- monitor_printf(mon, "%s", buf->str);
- g_string_set_size(buf, 0);
- if (image_info->has_backing_image) {
- image_info = image_info->backing_image;
- } else {
- break;
+ if (verbose) {
+ if (!buf) {
+ buf = g_string_sized_new(2048);
+ }
+ monitor_printf(mon, " images:\n");
+ image_info = info->value->inserted->image;
+ while (1) {
+ bdrv_image_info_dump(buf, image_info);
+ monitor_printf(mon, "%s", buf->str);
+ g_string_set_size(buf, 0);
+ if (image_info->has_backing_image) {
+ image_info = image_info->backing_image;
+ } else {
+ break;
+ }
}
}
-
} else {
monitor_printf(mon, " [not inserted]");
}
diff --git a/monitor.c b/monitor.c
index 4720207..ae5a61e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2465,9 +2465,10 @@ static mon_cmd_t info_cmds[] = {
},
{
.name = "block",
- .args_type = "",
- .params = "",
- .help = "show the block devices",
+ .args_type = "verbose:-v,device:B?",
+ .params = "[-v] [device]",
+ .help = "show info of one block device or all block devices "
+ "(and details of images with -v option)",
.mhandler.cmd = hmp_info_block,
},
{
--
1.7.1
^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list()
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-04-03 1:17 ` Eric Blake
2013-04-03 5:24 ` Wenchao Xia
2013-04-10 15:11 ` Markus Armbruster
0 siblings, 2 replies; 67+ messages in thread
From: Eric Blake @ 2013-04-03 1:17 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, armbru, qemu-devel, lcapitulino, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 2555 bytes --]
On 04/02/2013 05:47 AM, 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 | 55 ++++++++++++++++++++++++++++++++++++++-----------
> include/block/qapi.h | 4 ++-
> qemu-img.c | 5 +++-
> 3 files changed, 49 insertions(+), 15 deletions(-)
> +/*
> + * return 0 on success, @p_list will be set only on success, and caller need to
s/need/needs/
> + * check *p_list on success.
I wonder if this wording would be any better:
Returns 0 on success, with *p_list either set to describe snapshot
information, or NULL because there are no snapshots. Returns -1 on
error, with *p_list untouched.
> + */
> +int bdrv_query_snapshot_info_list(BlockDriverState *bs,
> + SnapshotInfoList **p_list,
> + Error **errp)
> {
At any rate, my only commentary was on grammar and a possible wording
for a comment, while the code itself is fine from my viewpoint; so feel
free to add:
Reviewed-by: Eric Blake <eblake@redhat.com>
> +++ b/qemu-img.c
> @@ -1735,7 +1735,10 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>
> info = g_new0(ImageInfo, 1);
> bdrv_collect_image_info(bs, info, filename);
> - bdrv_collect_snapshots(bs, info);
> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
> + info->snapshots) {
> + info->has_snapshots = true;
> + }
Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is
untouched on error. Since you are ignoring any errors, you technically
could write:
bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL);
if (info->snapshots) {
info->has_snapshots = true;
}
for the same semantics. That means that as of this commit, no caller
cares about the return value of bdrv_query_snapshot_info_list (they only
care about whether info->snapshots was changed to non-null), so it could
return void for a slightly simpler implementation.
But I don't know if any later patches in the series start to care about
which error was returned.
--
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] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list()
2013-04-03 1:17 ` Eric Blake
@ 2013-04-03 5:24 ` Wenchao Xia
2013-04-10 15:11 ` Markus Armbruster
1 sibling, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-03 5:24 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, qemu-devel, stefanha, armbru, lcapitulino, pbonzini
于 2013-4-3 9:17, Eric Blake 写道:
> On 04/02/2013 05:47 AM, 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 | 55 ++++++++++++++++++++++++++++++++++++++-----------
>> include/block/qapi.h | 4 ++-
>> qemu-img.c | 5 +++-
>> 3 files changed, 49 insertions(+), 15 deletions(-)
>
>> +/*
>> + * return 0 on success, @p_list will be set only on success, and caller need to
>
> s/need/needs/
>
>> + * check *p_list on success.
>
> I wonder if this wording would be any better:
>
> Returns 0 on success, with *p_list either set to describe snapshot
> information, or NULL because there are no snapshots. Returns -1 on
> error, with *p_list untouched.
>
>> + */
>> +int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>> + SnapshotInfoList **p_list,
>> + Error **errp)
>> {
>
> At any rate, my only commentary was on grammar and a possible wording
> for a comment, while the code itself is fine from my viewpoint; so feel
> free to add:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +++ b/qemu-img.c
>> @@ -1735,7 +1735,10 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>>
>> info = g_new0(ImageInfo, 1);
>> bdrv_collect_image_info(bs, info, filename);
>> - bdrv_collect_snapshots(bs, info);
>> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
>> + info->snapshots) {
>> + info->has_snapshots = true;
>> + }
>
> Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is
> untouched on error. Since you are ignoring any errors, you technically
> could write:
>
> bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL);
> if (info->snapshots) {
> info->has_snapshots = true;
> }
>
> for the same semantics. That means that as of this commit, no caller
> cares about the return value of bdrv_query_snapshot_info_list (they only
> care about whether info->snapshots was changed to non-null), so it could
> return void for a slightly simpler implementation.
>
> But I don't know if any later patches in the series start to care about
> which error was returned.
>
Yes it would be cared in ImageInfo retrieving later. This section in
qemu-img.c would be replaced later so did not check strictly the
returned value.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (16 preceding siblings ...)
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 17/17] hmp: add parameters device and -v for info block Wenchao Xia
@ 2013-04-07 5:54 ` Wenchao Xia
2013-04-08 10:18 ` Kevin Wolf
` (2 subsequent siblings)
20 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-07 5:54 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, armbru, pbonzini, lcapitulino
Hi, Kevin
Except the comments from Eric for better code comments, do you think
other things need improved? Modified patches is addressed as following,
I hope fix all in next version, to avoid using more time of reviewer.
>
> v11:
> General change:
> 5/17: check if snapshot > 0 on success in caller, add comments on the
> function says that caller need to check it on success.
> 7/17: check if snapshot > 0 on success before set info->has_snapshots.
>
> Address Eric's comments:
> 5/17: use error_setg_errno instead of strerror().
> 12/17: also move bdrv_snapshot_dump() since it need to be changed using
> GString later.
> 13/17: using GString as buffer.
> 14/17: using GString to dump snapshot info.
> 16/17: using GString to dump image info.
>
> Address Kevin's comments:
> 11/17: return void for bdrv_query_info().
> 17/17: spell fix in help message, using parameter "-v" to show
> verbose info.
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block Wenchao Xia
@ 2013-04-08 10:04 ` Kevin Wolf
2013-04-08 13:33 ` Stefan Hajnoczi
2013-04-10 16:27 ` Markus Armbruster
2 siblings, 0 replies; 67+ messages in thread
From: Kevin Wolf @ 2013-04-08 10:04 UTC (permalink / raw)
To: Wenchao Xia; +Cc: armbru, qemu-devel, lcapitulino, stefanha, pbonzini
Am 02.04.2013 um 13:47 hat Wenchao Xia geschrieben:
> Now image info will be retrieved as an embbed json object inside
> BlockDeviceInfo, backing chain info and all related internal snapshot
> info can be got in the enhanced recursive structure of ImageInfo.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qapi.c | 38 ++++++++++++++++++++++++++--
> include/block/qapi.h | 4 ++-
> qapi-schema.json | 5 +++-
> qmp-commands.hx | 67 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 108 insertions(+), 6 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index fa61c85..e98a028 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -202,9 +202,14 @@ int bdrv_query_image_info(BlockDriverState *bs,
> return 0;
> }
>
> -BlockInfo *bdrv_query_info(BlockDriverState *bs)
> +/* @p_info will be set only on success. */
> +void bdrv_query_info(BlockDriverState *bs,
> + BlockInfo **p_info,
> + Error **errp)
> {
> BlockInfo *info = g_malloc0(sizeof(*info));
> + BlockDriverState *bs0;
> + ImageInfo **p_image_info;
> info->device = g_strdup(bs->device_name);
> info->type = g_strdup("unknown");
> info->locked = bdrv_dev_is_medium_locked(bs);
> @@ -258,8 +263,28 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs)
> info->inserted->iops_wr =
> bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
> }
> +
> + bs0 = bs;
> + p_image_info = &info->inserted->image;
> + while (1) {
> + if (bdrv_query_image_info(bs0, p_image_info, errp)) {
> + goto err;
> + }
> + if (bs0->drv && bs0->backing_hd) {
> + bs0 = bs0->backing_hd;
> + (*p_image_info)->has_backing_image = true;
> + p_image_info = &((*p_image_info)->backing_image);
> + } else {
> + break;
> + }
> + }
> }
> - return info;
> +
> + *p_info = info;
> + return;
> +
> + err:
> + qapi_free_BlockInfo(info);
> }
>
> SnapshotInfoList *qmp_query_snapshots(Error **errp)
> @@ -286,11 +311,18 @@ BlockInfoList *qmp_query_block(Error **errp)
>
> while ((bs = bdrv_next(bs))) {
> BlockInfoList *info = g_malloc0(sizeof(*info));
> - info->value = bdrv_query_info(bs);
> + bdrv_query_info(bs, &info->value, errp);
> + if (error_is_set(errp)) {
> + goto err;
> + }
This doesn't do what you think it does: The caller can pass NULL as
errp, and then this becomes never true. I think you need to use a
local_err and propagate it to errp in error cases.
>
> *p_next = info;
> p_next = &info->next;
> }
>
> return head;
> +
> + err:
> + qapi_free_BlockInfoList(head);
> + return NULL;
> }
Kevin
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (17 preceding siblings ...)
2013-04-07 5:54 ` [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
@ 2013-04-08 10:18 ` Kevin Wolf
2013-04-08 13:43 ` Stefan Hajnoczi
2013-04-11 12:17 ` Markus Armbruster
20 siblings, 0 replies; 67+ messages in thread
From: Kevin Wolf @ 2013-04-08 10:18 UTC (permalink / raw)
To: Wenchao Xia; +Cc: armbru, qemu-devel, lcapitulino, stefanha, pbonzini
Am 02.04.2013 um 13:47 hat Wenchao Xia geschrieben:
> v11:
> General change:
> 5/17: check if snapshot > 0 on success in caller, add comments on the
> function says that caller need to check it on success.
> 7/17: check if snapshot > 0 on success before set info->has_snapshots.
>
> Address Eric's comments:
> 5/17: use error_setg_errno instead of strerror().
> 12/17: also move bdrv_snapshot_dump() since it need to be changed using
> GString later.
> 13/17: using GString as buffer.
> 14/17: using GString to dump snapshot info.
> 16/17: using GString to dump image info.
>
> Address Kevin's comments:
> 11/17: return void for bdrv_query_info().
> 17/17: spell fix in help message, using parameter "-v" to show
> verbose info.
In patch 11 I commented on what I think is an error handling bug. The
rest of the series is:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots Wenchao Xia
@ 2013-04-08 13:28 ` Stefan Hajnoczi
2013-04-10 15:51 ` Markus Armbruster
1 sibling, 0 replies; 67+ messages in thread
From: Stefan Hajnoczi @ 2013-04-08 13:28 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, armbru, qemu-devel, lcapitulino, pbonzini
On Tue, Apr 02, 2013 at 07:47:22PM +0800, Wenchao Xia wrote:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1e0e11e..6b20684 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1765,6 +1765,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
I think the "consistent snapshot" term is not defined. Please add
something like:
"Consistent snapshots are snapshots that exist in all writeable block
devices. When 'savevm' takes a snapshot it uses the same ID across all
writeable block devices. If a snapshot ID only exists in one block
device then it is not a consistent snapshot."
Stefan
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block Wenchao Xia
2013-04-08 10:04 ` Kevin Wolf
@ 2013-04-08 13:33 ` Stefan Hajnoczi
2013-04-10 7:30 ` Wenchao Xia
2013-04-10 16:17 ` Markus Armbruster
2013-04-10 16:27 ` Markus Armbruster
2 siblings, 2 replies; 67+ messages in thread
From: Stefan Hajnoczi @ 2013-04-08 13:33 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, armbru, qemu-devel, lcapitulino, pbonzini
On Tue, Apr 02, 2013 at 07:47:24PM +0800, Wenchao Xia wrote:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6b20684..b856be7 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1703,6 +1703,47 @@ Each json-object contain the following:
> - "iops": limit total I/O operations per second (json-int)
> - "iops_rd": limit read operations per second (json-int)
> - "iops_wr": limit write operations per second (json-int)
> + - "image": the detail of the image, it is a json-object containing
> + the following:
> + - "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, not present if image
> + format does not support it (json-int, optional)
> + - "encrypted": true if the image is encrypted, not present means
> + false or the image format does not support
> + encryption (json-bool, optional)
> + - "backing_file": backing file name, not present means no backing
> + file is used or the image format does not
> + support backing file chain
> + (json-string, optional)
> + - "full-backing-filename": full path of the backing file, not
> + present if it equals backing_file or no
> + backing file is used
> + (json-string, optional)
> + - "backing-filename-format": the format of the backing file, not
> + present means unknown or no 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 nanoseconds 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 nanoseconds to be used
> + with vm-clock-sec (json-int)
> + - "backing-image": the detail of the backing image, it is an
> + optional json-object only present when a
> + backing image present for this image
Please don't duplicate the ImageInfo documentation from
qapi-schema.json.
I'm not sure how the documentation gets generated but we need to avoid
this, otherwise it will quickly get out of sync.
Stefan
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
@ 2013-04-08 13:36 ` Stefan Hajnoczi
2013-04-10 16:56 ` Markus Armbruster
2013-04-10 17:05 ` Markus Armbruster
1 sibling, 1 reply; 67+ messages in thread
From: Stefan Hajnoczi @ 2013-04-08 13:36 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, armbru, qemu-devel, lcapitulino, pbonzini
On Tue, Apr 02, 2013 at 07:47:26PM +0800, Wenchao Xia wrote:
> diff --git a/qemu-img.c b/qemu-img.c
> index 5b229a9..032f68c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1558,18 +1558,24 @@ static void dump_snapshots(BlockDriverState *bs)
> {
> QEMUSnapshotInfo *sn_tab, *sn;
> int nb_sns, i;
> - char buf[256];
> + GString *buf = g_string_new(NULL);
>
> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> if (nb_sns <= 0)
> return;
Leaks buf.
> diff --git a/savevm.c b/savevm.c
> index e4e0008..ce0bbe1 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2466,7 +2466,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> int nb_sns, i, ret, available;
> int total;
> int *available_snapshots;
> - char buf[256];
> + GString *buf = NULL;
>
> bs = bdrv_snapshots();
> if (!bs) {
> @@ -2509,11 +2509,16 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> }
>
> if (total > 0) {
Please declare buf here since it is only used in this scope.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (18 preceding siblings ...)
2013-04-08 10:18 ` Kevin Wolf
@ 2013-04-08 13:43 ` Stefan Hajnoczi
2013-04-11 12:17 ` Markus Armbruster
20 siblings, 0 replies; 67+ messages in thread
From: Stefan Hajnoczi @ 2013-04-08 13:43 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, armbru, qemu-devel, lcapitulino, pbonzini
On Tue, Apr 02, 2013 at 07:47:13PM +0800, Wenchao Xia wrote:
> 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
> or enchance following interfaces for qemu:
Looks pretty good, I commented on a few small things.
Stefan
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-08 13:33 ` Stefan Hajnoczi
@ 2013-04-10 7:30 ` Wenchao Xia
2013-04-11 8:54 ` Stefan Hajnoczi
2013-04-10 16:17 ` Markus Armbruster
1 sibling, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-10 7:30 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, lcapitulino, armbru, qemu-devel
于 2013-4-8 21:33, Stefan Hajnoczi 写道:
> On Tue, Apr 02, 2013 at 07:47:24PM +0800, Wenchao Xia wrote:
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 6b20684..b856be7 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -1703,6 +1703,47 @@ Each json-object contain the following:
>> - "iops": limit total I/O operations per second (json-int)
>> - "iops_rd": limit read operations per second (json-int)
>> - "iops_wr": limit write operations per second (json-int)
>> + - "image": the detail of the image, it is a json-object containing
>> + the following:
>> + - "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, not present if image
>> + format does not support it (json-int, optional)
>> + - "encrypted": true if the image is encrypted, not present means
>> + false or the image format does not support
>> + encryption (json-bool, optional)
>> + - "backing_file": backing file name, not present means no backing
>> + file is used or the image format does not
>> + support backing file chain
>> + (json-string, optional)
>> + - "full-backing-filename": full path of the backing file, not
>> + present if it equals backing_file or no
>> + backing file is used
>> + (json-string, optional)
>> + - "backing-filename-format": the format of the backing file, not
>> + present means unknown or no 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 nanoseconds 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 nanoseconds to be used
>> + with vm-clock-sec (json-int)
>> + - "backing-image": the detail of the backing image, it is an
>> + optional json-object only present when a
>> + backing image present for this image
>
> Please don't duplicate the ImageInfo documentation from
> qapi-schema.json.
>
> I'm not sure how the documentation gets generated but we need to avoid
> this, otherwise it will quickly get out of sync.
>
> Stefan
>
So remove this section? It seems no other way to sync them except
manually change both of them.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list()
2013-04-03 1:17 ` Eric Blake
2013-04-03 5:24 ` Wenchao Xia
@ 2013-04-10 15:11 ` Markus Armbruster
2013-04-11 3:19 ` Wenchao Xia
1 sibling, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 15:11 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, Wenchao Xia
Eric Blake <eblake@redhat.com> writes:
> On 04/02/2013 05:47 AM, 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 | 55 ++++++++++++++++++++++++++++++++++++++-----------
>> include/block/qapi.h | 4 ++-
>> qemu-img.c | 5 +++-
>> 3 files changed, 49 insertions(+), 15 deletions(-)
>
>> +/*
>> + * return 0 on success, @p_list will be set only on success, and caller need to
>
> s/need/needs/
>
>> + * check *p_list on success.
>
> I wonder if this wording would be any better:
>
> Returns 0 on success, with *p_list either set to describe snapshot
> information, or NULL because there are no snapshots. Returns -1 on
> error, with *p_list untouched.
It actually returns -errno then, doesn't it?
>
>> + */
>> +int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>> + SnapshotInfoList **p_list,
>> + Error **errp)
>> {
>
> At any rate, my only commentary was on grammar and a possible wording
> for a comment, while the code itself is fine from my viewpoint; so feel
> free to add:
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
>> +++ b/qemu-img.c
>> @@ -1735,7 +1735,10 @@ static ImageInfoList
>> *collect_image_info_list(const char *filename,
>>
>> info = g_new0(ImageInfo, 1);
>> bdrv_collect_image_info(bs, info, filename);
>> - bdrv_collect_snapshots(bs, info);
>> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
>> + info->snapshots) {
>> + info->has_snapshots = true;
>> + }
>
> Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is
> untouched on error. Since you are ignoring any errors, you technically
> could write:
>
> bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL);
> if (info->snapshots) {
> info->has_snapshots = true;
> }
>
> for the same semantics. That means that as of this commit, no caller
> cares about the return value of bdrv_query_snapshot_info_list (they only
> care about whether info->snapshots was changed to non-null), so it could
> return void for a slightly simpler implementation.
Return the list, or NULL.
> But I don't know if any later patches in the series start to care about
> which error was returned.
Me neither :)
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list()
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-04-10 15:19 ` Markus Armbruster
2013-04-11 5:56 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 15:19 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 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>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.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 03369a5..19d4d93 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -23,14 +23,47 @@
> */
>
> #include "block/qapi.h"
> +#include "block/snapshot.h"
> #include "block/block_int.h"
>
> /*
> + * check whether the snapshot is valid for whole vm.
> + *
> + * @sn: snapshot info to be checked.
> + * @bs: where @sn was found.
> + *
> + * return true if the snapshot is consistent for the VM.
> + */
> +static bool snapshot_valid_for_vm(const QEMUSnapshotInfo *sn,
> + BlockDriverState *bs)
> +{
> + BlockDriverState *bs1 = NULL;
> + QEMUSnapshotInfo s, *sn_info = &s;
> + int ret;
> +
> + /* 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))) {
Unlike load_vmstate(), you don't do
if (!bdrv_is_inserted(bs1) || bdrv_is_read_only(bs1)) {
continue;
}
Why?
> + if (bs1 != bs && bdrv_can_snapshot(bs1)) {
> + ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
> + if (ret < 0) {
> + return false;
> + }
> + }
> + }
> + return true;
> +}
> +
> +/*
> * return 0 on success, @p_list will be set only on success, and caller need to
> - * check *p_list on success.
> + * check *p_list on success. If @vm_snapshot is true, limit the results to
> + * snapshots valid for the whole VM.
> */
> int bdrv_query_snapshot_info_list(BlockDriverState *bs,
> SnapshotInfoList **p_list,
> + bool vm_snapshot,
> Error **errp)
> {
> int i, sn_count;
> @@ -59,7 +92,9 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
> }
>
> for (i = 0; i < sn_count; i++) {
> -
> + if (vm_snapshot && !snapshot_valid_for_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 91dc41b..fe66053 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -30,6 +30,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 51043ef..261c277 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);
> - if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
> + false, NULL) &&
> info->snapshots) {
> info->has_snapshots = true;
> }
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot()
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
@ 2013-04-10 15:21 ` Markus Armbruster
2013-04-11 3:16 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 15:21 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> To make it clear about id and name in searching, the API is changed
> a bit to distinguish them, and caller can choose to search by id or name.
> Searching will be done with higher priority of id. This function also
> returns negative value from bdrv_snapshot_list() instead of -ENOENT on
> error now.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@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 c47a899..7b2026c 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -24,8 +24,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.
> + */
Let me try to improve:
/**
* Look up an internal snapshot by @id, or else by @name.
* @bs: block device to search
* @sn_info: location to store information on the snapshot found
* @id: unique snapshot ID, or NULL
* @name: snapshot name, or NULL
*
* If the snapshot with unique ID @id exists, find it.
* Else, if snapshots with name @name exists, find one of them.
*
* Returns: 0 when a snapshot is found, else -errno.
*/
I'd do two separate functions, one to loop up a snapshot by ID, and one
to look it up by name, because I find them simpler. Matter of taste.
> 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;
> @@ -33,16 +45,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 4ad070c..a047a8e 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -33,5 +33,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 88acc38..1d2da99 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2212,7 +2212,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) {
Before your patch: deletes first snapshot sn whose sn->id_str or
sn->name match name.
After your patch: deletes first snapshot sn whose sn->id_str matches
name, or else first snapshot whose sn->name matches name.
Running example: say we have the following two snapshots with rather
unwisely chosen names:
id_str name
1 2
2 1
Which one will del_existing_snapshots(mon, "2") delete?
Before your patch: first one.
After: second one.
See below for impact.
> @@ -2272,7 +2272,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);
Similar change.
This function is the only user of del_existing_snapshots().
Same example.
Before your patch: "savevm 2" overwrites the first snapshot.
After: it overwrites the second snapshot.
Thus, your patch changes del_existing_snapshots()
I'm fine with the change, but I want it noted *prominently* in the
commit message that it can change savevm's interpretation of ambiguous
name arguments.
> @@ -2363,7 +2363,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) {
> @@ -2387,7 +2387,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);
Same example.
Before your patch: "loadvm 2" loads the first snapshot.
After: it loads the second snapshot.
Document in commit message, just like the savevm change.
> @@ -2493,7 +2493,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;
Different from the other calls: name argument is NULL rather than same
as id argument. Let's see what difference it makes.
The loop iterates over all snapshots on the "primary" snapshot device
(the one that gets the VM state on savevm). For each one, it checks
whether all other snapshot devices have that snapshot.
Before your patch: a device has the snapshot when it has one whose
sn->id_str or sn->name match the primary snapshot's id_str. Nuts :)
After your patch: a device has the snapshot when it has one whose
sn->id_str matches the primary snapshot's id_str.
I'm fine with the change, but it needs to be documented in the commit
message.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 07/17] block: add image info query function bdrv_query_image_info()
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 07/17] block: add image info query function bdrv_query_image_info() Wenchao Xia
@ 2013-04-10 15:28 ` Markus Armbruster
2013-04-11 5:59 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 15:28 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> This patch adds function bdrv_query_image_info(), which will
> retrieve image info in qmp object format. The implementation is
> based on the code moved from qemu-img.c, but uses block layer
> function to get snapshot info.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qapi.c | 41 ++++++++++++++++++++++++++++++++++-------
> include/block/qapi.h | 6 +++---
> qemu-img.c | 8 ++------
> 3 files changed, 39 insertions(+), 16 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 19d4d93..176a479 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -122,18 +122,22 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
> return 0;
> }
>
> -void bdrv_collect_image_info(BlockDriverState *bs,
> - ImageInfo *info,
> - const char *filename)
> +/* 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);
> @@ -150,8 +154,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') {
backing_filename can't possibly be null here, because bs->backing_file
is an array.
> info->backing_filename = g_strdup(backing_filename);
> info->has_backing_filename = true;
> bdrv_get_full_backing_filename(bs, backing_filename2,
> @@ -168,4 +172,27 @@ 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:
> + if (info->snapshots) {
> + info->has_snapshots = true;
> + }
> + break;
> + /* recoverable error */
> + case -ENOMEDIUM:
> + error_free(err);
> + break;
> + case -ENOTSUP:
> + error_free(err);
> + break;
Suggest
case -ENOMEDIUM:
case -ENOTSUP:
/* no snapshots on this device */
error_free(err);
break;
for clarity.
> + 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 fe66053..2c62fdf 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -32,7 +32,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);
> +int bdrv_query_image_info(BlockDriverState *bs,
> + ImageInfo **p_info,
> + Error **errp);
> #endif
> diff --git a/qemu-img.c b/qemu-img.c
> index 261c277..1dd0a60 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1733,12 +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);
> - if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
> - false, NULL) &&
> - info->snapshots) {
> - info->has_snapshots = true;
> + if (bdrv_query_image_info(bs, &info, NULL)) {
> + goto err;
> }
>
> elem = g_new0(ImageInfoList, 1);
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots Wenchao Xia
2013-04-08 13:28 ` Stefan Hajnoczi
@ 2013-04-10 15:51 ` Markus Armbruster
2013-04-11 6:03 ` Wenchao Xia
1 sibling, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 15:51 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> This interface returns info of valid internal snapshots for whole vm.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/qapi.c | 17 ++++++++++++++++
> qapi-schema.json | 14 +++++++++++++
> qmp-commands.hx | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 86 insertions(+), 0 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 6e0c7c3..5e91ab8 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -258,6 +258,23 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs)
> return info;
> }
>
> +SnapshotInfoList *qmp_query_snapshots(Error **errp)
> +{
> + BlockDriverState *bs;
> + SnapshotInfoList *list = NULL;
> +
> + /* internal snapshot for whole vm */
> + bs = bdrv_snapshots();
> + if (!bs) {
> + error_setg(errp, "No available block device supports snapshots\n");
> + return NULL;
> + }
> +
> + bdrv_query_snapshot_info_list(bs, &list, true, errp);
> +
> + return list;
> +}
> +
> BlockInfoList *qmp_query_block(Error **errp)
> {
> BlockInfoList *head = NULL, **p_next = &head;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index f629a24..225afef 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -841,6 +841,20 @@
> { 'command': 'query-block', 'returns': ['BlockInfo'] }
>
> ##
> +# @query-snapshots:
> +#
> +# Get a list of internal snapshots for the whole virtual machine. Only valid
> +# internal snapshots 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 1e0e11e..6b20684 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1765,6 +1765,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 nanoseconds 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 nanoseconds 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
Not your patch's fault, but here goes anyway: I dislike this
representation of time.
QMP has time in seconds, milliseconds, nanoseconds, (seconds,
milliseconds) and (seconds, nanoseconds). There has been no adult
supervision, obviously (I may say that, because it's as much my fault as
it's anybody else's).
The sanest one by far is nanoseconds. Good for 2^63 of them. Since pi
seconds is a nanocentury, good for 2^63 / (pi * 1e9) centuries, which
should be safely beyond your retirement age.
> + },
> + {
> + "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
> ----------------
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 10/17] qmp: add recursive member in ImageInfo
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 10/17] qmp: add recursive member in ImageInfo Wenchao Xia
@ 2013-04-10 16:06 ` Markus Armbruster
2013-04-11 6:06 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 16:06 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> New member *backing-image is added to reflect the backing chain
> status.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> block/qapi.c | 6 +++++-
> qapi-schema.json | 5 ++++-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 5e91ab8..fa61c85 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -123,7 +123,11 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
> return 0;
> }
>
> -/* return 0 on success, and @p_info will be set only on success. */
> +/*
> + * return 0 on success, and @p_info will be set only on success,
> + * (*pinfo)->has_backing_image will be false and (*pinfo)->backing_image will
> + * be NULL.
> + */
Sounds like this function computes incomplete ImageInfo. Correct? If
yes, why?
> int bdrv_query_image_info(BlockDriverState *bs,
> ImageInfo **p_info,
> Error **errp)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 225afef..ad9dd82 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -233,6 +233,8 @@
> #
> # @snapshots: #optional list of VM snapshots
> #
> +# @backing-image: #optional info of the backing image (since 1.5)
> +#
> # Since: 1.3
> #
> ##
> @@ -242,7 +244,8 @@
> '*actual-size': 'int', 'virtual-size': 'int',
> '*cluster-size': 'int', '*encrypted': 'bool',
> '*backing-filename': 'str', '*full-backing-filename': 'str',
> - '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'] } }
> + '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
> + '*backing-image': 'ImageInfo' } }
>
> ##
> # @ImageCheck:
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-08 13:33 ` Stefan Hajnoczi
2013-04-10 7:30 ` Wenchao Xia
@ 2013-04-10 16:17 ` Markus Armbruster
2013-04-10 18:57 ` Luiz Capitulino
1 sibling, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 16:17 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, lcapitulino, Wenchao Xia, qemu-devel
Stefan Hajnoczi <stefanha@gmail.com> writes:
> On Tue, Apr 02, 2013 at 07:47:24PM +0800, Wenchao Xia wrote:
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 6b20684..b856be7 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -1703,6 +1703,47 @@ Each json-object contain the following:
>> - "iops": limit total I/O operations per second (json-int)
>> - "iops_rd": limit read operations per second (json-int)
>> - "iops_wr": limit write operations per second (json-int)
>> + - "image": the detail of the image, it is a json-object containing
>> + the following:
>> + - "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, not present if image
>> + format does not support it (json-int, optional)
>> + - "encrypted": true if the image is encrypted, not present means
>> + false or the image format does not support
>> + encryption (json-bool, optional)
>> + - "backing_file": backing file name, not present means no backing
>> + file is used or the image format does not
>> + support backing file chain
>> + (json-string, optional)
>> + - "full-backing-filename": full path of the backing file, not
>> + present if it equals backing_file or no
>> + backing file is used
>> + (json-string, optional)
>> + - "backing-filename-format": the format of the backing file, not
>> + present means unknown or no 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 nanoseconds 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 nanoseconds to be used
>> + with vm-clock-sec (json-int)
>> + - "backing-image": the detail of the backing image, it is an
>> + optional json-object only present when a
>> + backing image present for this image
>
> Please don't duplicate the ImageInfo documentation from
> qapi-schema.json.
No, this is actually how it still needs to be done.
qmp-commands.hx predates qapi-schema.json. We still generate
QMP/qmp-commands.txt from qmp-commands.hx, not from qapi-schema.json.
> I'm not sure how the documentation gets generated but we need to avoid
> this, otherwise it will quickly get out of sync.
If I remember correctly, Luiz wants to get rid of qmp-commands.hx. But
we're not there, yet.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block Wenchao Xia
2013-04-08 10:04 ` Kevin Wolf
2013-04-08 13:33 ` Stefan Hajnoczi
@ 2013-04-10 16:27 ` Markus Armbruster
2013-04-11 6:20 ` Wenchao Xia
2 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 16:27 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> Now image info will be retrieved as an embbed json object inside
> BlockDeviceInfo, backing chain info and all related internal snapshot
> info can be got in the enhanced recursive structure of ImageInfo.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qapi.c | 38 ++++++++++++++++++++++++++--
> include/block/qapi.h | 4 ++-
> qapi-schema.json | 5 +++-
> qmp-commands.hx | 67 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 108 insertions(+), 6 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index fa61c85..e98a028 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -202,9 +202,14 @@ int bdrv_query_image_info(BlockDriverState *bs,
> return 0;
> }
>
> -BlockInfo *bdrv_query_info(BlockDriverState *bs)
> +/* @p_info will be set only on success. */
> +void bdrv_query_info(BlockDriverState *bs,
> + BlockInfo **p_info,
> + Error **errp)
> {
> BlockInfo *info = g_malloc0(sizeof(*info));
> + BlockDriverState *bs0;
> + ImageInfo **p_image_info;
> info->device = g_strdup(bs->device_name);
> info->type = g_strdup("unknown");
> info->locked = bdrv_dev_is_medium_locked(bs);
> @@ -258,8 +263,28 @@ BlockInfo *bdrv_query_info(BlockDriverState *bs)
> info->inserted->iops_wr =
> bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
> }
> +
> + bs0 = bs;
> + p_image_info = &info->inserted->image;
> + while (1) {
> + if (bdrv_query_image_info(bs0, p_image_info, errp)) {
> + goto err;
> + }
> + if (bs0->drv && bs0->backing_hd) {
> + bs0 = bs0->backing_hd;
> + (*p_image_info)->has_backing_image = true;
> + p_image_info = &((*p_image_info)->backing_image);
> + } else {
> + break;
> + }
> + }
> }
> - return info;
> +
> + *p_info = info;
> + return;
> +
> + err:
> + qapi_free_BlockInfo(info);
> }
>
> SnapshotInfoList *qmp_query_snapshots(Error **errp)
> @@ -286,11 +311,18 @@ BlockInfoList *qmp_query_block(Error **errp)
>
> while ((bs = bdrv_next(bs))) {
> BlockInfoList *info = g_malloc0(sizeof(*info));
> - info->value = bdrv_query_info(bs);
> + bdrv_query_info(bs, &info->value, errp);
> + if (error_is_set(errp)) {
> + goto err;
> + }
>
> *p_next = info;
> p_next = &info->next;
> }
>
> return head;
> +
> + err:
> + qapi_free_BlockInfoList(head);
> + return NULL;
> }
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index 0039a70..e0fd0a5 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -35,5 +35,7 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
> int bdrv_query_image_info(BlockDriverState *bs,
> ImageInfo **p_info,
> Error **errp);
> -BlockInfo *bdrv_query_info(BlockDriverState *bs);
> +void bdrv_query_info(BlockDriverState *bs,
> + BlockInfo **p_info,
> + Error **errp);
> #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index ad9dd82..02dabc3 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -756,6 +756,8 @@
> #
> # @iops_wr: write I/O operations per second is specified
> #
> +# @image: the info of image used (since: 1.5)
> +#
> # Since: 0.14.0
> #
> # Notes: This interface is only found in @BlockInfo.
> @@ -765,7 +767,8 @@
> '*backing_file': 'str', 'backing_file_depth': 'int',
> 'encrypted': 'bool', 'encryption_key_missing': 'bool',
> 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> - 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
> + 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
> + 'image': 'ImageInfo' } }
>
> ##
> # @BlockDeviceIoStatus:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6b20684..b856be7 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1703,6 +1703,47 @@ Each json-object contain the following:
> - "iops": limit total I/O operations per second (json-int)
> - "iops_rd": limit read operations per second (json-int)
> - "iops_wr": limit write operations per second (json-int)
> + - "image": the detail of the image, it is a json-object containing
> + the following:
> + - "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, not present if image
> + format does not support it (json-int, optional)
> + - "encrypted": true if the image is encrypted, not present means
> + false or the image format does not support
> + encryption (json-bool, optional)
> + - "backing_file": backing file name, not present means no backing
> + file is used or the image format does not
> + support backing file chain
> + (json-string, optional)
> + - "full-backing-filename": full path of the backing file, not
> + present if it equals backing_file or no
> + backing file is used
> + (json-string, optional)
> + - "backing-filename-format": the format of the backing file, not
> + present means unknown or no 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 nanoseconds 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 nanoseconds to be used
> + with vm-clock-sec (json-int)
> + - "backing-image": the detail of the backing image, it is an
> + optional json-object only present when a
> + backing image present for this image
>
> - "io-status": I/O operation status, only present if the device supports it
> and the VM is configured to stop on errors. It's always reset
> @@ -1724,13 +1765,37 @@ Example:
> "drv":"qcow2",
> "encrypted":false,
> "file":"disks/test.img",
> - "backing_file_depth":0,
> + "backing_file_depth":1,
> "bps":1000000,
> "bps_rd":0,
> "bps_wr":0,
> "iops":1000000,
> "iops_rd":0,
> "iops_wr":0,
> + "image":{
> + "filename":"disks/test.img",
Please change to "disks/test.qcow2". I don't want people to get the
idea they should name their QCOW2 images .img.
> + "format":"qcow2",
I wonder how inserted.file and inserted.drv are related to
inserted.image.filename and inserted.image.format. Redundant or not?
Kevin?
> + "virtual-size":2048000,
> + "backing_file":"base.img",
> + "full-backing-filename":"disks/base.img",
> + "backing-filename-format:"qcow2",
> + "snapshots":[
> + {
> + "id": "1",
> + "name": "snapshot1",
> + "vm-state-size": 0,
> + "date-sec": 10000200,
> + "date-nsec": 12,
> + "vm-clock-sec": 206,
> + "vm-clock-nsec": 30
> + }
> + ],
> + "backing-image":{
> + "filename":"disks/base.img",
> + "format":"qcow2",
And here I wonder how inserted.image.full-backing-filename and
.backing-filename-format are related to
inserted.image.backing-image.filename and .backing-image.format. Kevin?
> + "virtual-size":2048000
> + }
> + }
> },
> "type":"unknown"
> },
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 12/17] block: move bdrv_snapshot_dump() and dump_human_image_info() to block/qapi.c
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 12/17] block: move bdrv_snapshot_dump() and dump_human_image_info() to block/qapi.c Wenchao Xia
@ 2013-04-10 16:49 ` Markus Armbruster
2013-04-11 6:30 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 16:49 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> They are needed later in hmp command, dump_human_image_info()
> is renamed to bdrv_image_info_dump().
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
I'd be tempted move everything that needs to move to block/qapi.c in one
go.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-08 13:36 ` Stefan Hajnoczi
@ 2013-04-10 16:56 ` Markus Armbruster
0 siblings, 0 replies; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 16:56 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, pbonzini, lcapitulino, Wenchao Xia, qemu-devel
Stefan Hajnoczi <stefanha@gmail.com> writes:
> On Tue, Apr 02, 2013 at 07:47:26PM +0800, Wenchao Xia wrote:
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 5b229a9..032f68c 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1558,18 +1558,24 @@ static void dump_snapshots(BlockDriverState *bs)
>> {
>> QEMUSnapshotInfo *sn_tab, *sn;
>> int nb_sns, i;
>> - char buf[256];
>> + GString *buf = g_string_new(NULL);
>>
>> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> if (nb_sns <= 0)
>> return;
>
> Leaks buf.
Trivially avoided my moving the initialization behind the return.
>> diff --git a/savevm.c b/savevm.c
>> index e4e0008..ce0bbe1 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2466,7 +2466,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>> int nb_sns, i, ret, available;
>> int total;
>> int *available_snapshots;
>> - char buf[256];
>> + GString *buf = NULL;
>>
>> bs = bdrv_snapshots();
>> if (!bs) {
>> @@ -2509,11 +2509,16 @@ void do_info_snapshots(Monitor *mon, const
>> QDict *qdict)
>> }
>>
>> if (total > 0) {
>
> Please declare buf here since it is only used in this scope.
Matter of taste. I'd keep it where it is :)
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
2013-04-08 13:36 ` Stefan Hajnoczi
@ 2013-04-10 17:05 ` Markus Armbruster
2013-04-11 6:35 ` Wenchao Xia
1 sibling, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-10 17:05 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> This patch would allow hmp use them just like qemu-img, and avoid
> string truncation.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qapi.c | 65 +++++++++++++++++++++++++++-----------------------
> include/block/qapi.h | 4 +-
> qemu-img.c | 22 ++++++++++++----
> savevm.c | 11 ++++++--
> 4 files changed, 61 insertions(+), 41 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index df63c97..e27519e 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -327,7 +327,7 @@ BlockInfoList *qmp_query_block(Error **errp)
> return NULL;
> }
>
> -char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
> +void bdrv_snapshot_dump(GString *buf, QEMUSnapshotInfo *sn)
> {
> char buf1[128], date_buf[128], clock_buf[128];
> struct tm tm;
I like this change, because it replaces an arbitrarily sized buffer by a
flexible one.
> @@ -335,9 +335,8 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
> int64_t secs;
>
> if (!sn) {
> - snprintf(buf, buf_size,
> - "%-10s%-20s%7s%20s%15s",
> - "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
> + g_string_append_printf(buf, "%-10s%-20s%7s%20s%15s",
> + "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
> } else {
> ti = sn->date_sec;
> localtime_r(&ti, &tm);
> @@ -350,17 +349,17 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
> (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);
> + g_string_append_printf(buf,
> + "%-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;
> }
>
> -void bdrv_image_info_dump(ImageInfo *info)
> +void bdrv_image_info_dump(GString *buf, ImageInfo *info)
> {
> char size_buf[128], dsize_buf[128];
> if (!info->has_actual_size) {
> @@ -370,43 +369,48 @@ void bdrv_image_info_dump(ImageInfo *info)
I don't like this change, because it introduces buffering for no
discernible reason. Unless you can show me one, I'd like you to keep
printing directly.
> 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);
> + g_string_append_printf(buf,
> + "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");
> + g_string_append_printf(buf, "encrypted: yes\n");
> }
>
> if (info->has_cluster_size) {
> - printf("cluster_size: %" PRId64 "\n", info->cluster_size);
> + g_string_append_printf(buf, "cluster_size: %" PRId64 "\n",
> + info->cluster_size);
> }
>
> if (info->has_dirty_flag && info->dirty_flag) {
> - printf("cleanly shut down: no\n");
> + g_string_append_printf(buf, "cleanly shut down: no\n");
> }
>
> if (info->has_backing_filename) {
> - printf("backing file: %s", info->backing_filename);
> + g_string_append_printf(buf, "backing file: %s",
> + info->backing_filename);
> if (info->has_full_backing_filename) {
> - printf(" (actual path: %s)", info->full_backing_filename);
> + g_string_append_printf(buf, " (actual path: %s)",
> + info->full_backing_filename);
> }
> - putchar('\n');
> + g_string_append_printf(buf, "\n");
> if (info->has_backing_filename_format) {
> - printf("backing file format: %s\n", info->backing_filename_format);
> + g_string_append_printf(buf, "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));
> + g_string_append_printf(buf, "Snapshot list:\n");
> + bdrv_snapshot_dump(buf, NULL);
> + g_string_append_printf(buf, "\n");
>
> /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
> * we convert to the block layer's native QEMUSnapshotInfo for now.
> @@ -422,7 +426,8 @@ 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));
> + bdrv_snapshot_dump(buf, &sn);
> + g_string_append_printf(buf, "\n");
> }
> }
> }
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index 237660f..dc1e090 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -38,6 +38,6 @@ int bdrv_query_image_info(BlockDriverState *bs,
> void bdrv_query_info(BlockDriverState *bs,
> BlockInfo **p_info,
> Error **errp);
> -char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
> -void bdrv_image_info_dump(ImageInfo *info);
> +void bdrv_snapshot_dump(GString *buf, QEMUSnapshotInfo *sn);
> +void bdrv_image_info_dump(GString *buf, ImageInfo *info);
> #endif
> diff --git a/qemu-img.c b/qemu-img.c
> index 5b229a9..032f68c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1558,18 +1558,24 @@ static void dump_snapshots(BlockDriverState *bs)
> {
> QEMUSnapshotInfo *sn_tab, *sn;
> int nb_sns, i;
> - char buf[256];
> + GString *buf = g_string_new(NULL);
>
> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> if (nb_sns <= 0)
> return;
> - printf("Snapshot list:\n");
> - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> + g_string_append_printf(buf, "Snapshot list:\n");
> + bdrv_snapshot_dump(buf, NULL);
> + g_string_append_printf(buf, "\n");
> for(i = 0; i < nb_sns; i++) {
> sn = &sn_tab[i];
> - printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
> + bdrv_snapshot_dump(buf, sn);
> + g_string_append_printf(buf, "\n");
> }
> +
> + printf("%s", buf->str);
> + g_string_free(buf, true);
> g_free(sn_tab);
> +
> }
>
> static void dump_json_image_info_list(ImageInfoList *list)
> @@ -1610,15 +1616,19 @@ static void dump_human_image_info_list(ImageInfoList *list)
> {
> ImageInfoList *elem;
> bool delim = false;
> + GString *buf = g_string_new(NULL);
>
> for (elem = list; elem; elem = elem->next) {
> if (delim) {
> - printf("\n");
> + g_string_append_printf(buf, "\n");
> }
> delim = true;
>
> - bdrv_image_info_dump(elem->value);
> + bdrv_image_info_dump(buf, elem->value);
> }
> +
> + printf("%s", buf->str);
> + g_string_free(buf, true);
> }
>
> static gboolean str_equal_func(gconstpointer a, gconstpointer b)
> diff --git a/savevm.c b/savevm.c
> index e4e0008..ce0bbe1 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2466,7 +2466,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> int nb_sns, i, ret, available;
> int total;
> int *available_snapshots;
> - char buf[256];
> + GString *buf = NULL;
Useless initialization. But if you keep bdrv_snapshot_dump() printing,
it all goes away.
>
> bs = bdrv_snapshots();
> if (!bs) {
> @@ -2509,11 +2509,16 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> }
>
> if (total > 0) {
> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> + buf = g_string_new(NULL);
> + bdrv_snapshot_dump(buf, NULL);
> + g_string_append_printf(buf, "\n");
> for (i = 0; i < total; i++) {
> sn = &sn_tab[available_snapshots[i]];
> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
> + bdrv_snapshot_dump(buf, sn);
> + g_string_append_printf(buf, "\n");
> }
> + monitor_printf(mon, "%s", buf->str);
> + g_string_free(buf, true);
> } else {
> monitor_printf(mon, "There is no suitable snapshot available\n");
> }
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-10 16:17 ` Markus Armbruster
@ 2013-04-10 18:57 ` Luiz Capitulino
2013-04-11 6:25 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Luiz Capitulino @ 2013-04-10 18:57 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, Stefan Hajnoczi, pbonzini, Wenchao Xia, qemu-devel
On Wed, 10 Apr 2013 18:17:04 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
> > On Tue, Apr 02, 2013 at 07:47:24PM +0800, Wenchao Xia wrote:
> >> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> index 6b20684..b856be7 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -1703,6 +1703,47 @@ Each json-object contain the following:
> >> - "iops": limit total I/O operations per second (json-int)
> >> - "iops_rd": limit read operations per second (json-int)
> >> - "iops_wr": limit write operations per second (json-int)
> >> + - "image": the detail of the image, it is a json-object containing
> >> + the following:
> >> + - "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, not present if image
> >> + format does not support it (json-int, optional)
> >> + - "encrypted": true if the image is encrypted, not present means
> >> + false or the image format does not support
> >> + encryption (json-bool, optional)
> >> + - "backing_file": backing file name, not present means no backing
> >> + file is used or the image format does not
> >> + support backing file chain
> >> + (json-string, optional)
> >> + - "full-backing-filename": full path of the backing file, not
> >> + present if it equals backing_file or no
> >> + backing file is used
> >> + (json-string, optional)
> >> + - "backing-filename-format": the format of the backing file, not
> >> + present means unknown or no 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 nanoseconds 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 nanoseconds to be used
> >> + with vm-clock-sec (json-int)
> >> + - "backing-image": the detail of the backing image, it is an
> >> + optional json-object only present when a
> >> + backing image present for this image
> >
> > Please don't duplicate the ImageInfo documentation from
> > qapi-schema.json.
>
> No, this is actually how it still needs to be done.
>
> qmp-commands.hx predates qapi-schema.json. We still generate
> QMP/qmp-commands.txt from qmp-commands.hx, not from qapi-schema.json.
Unfortunately, yes. However, as there are just a few commands missing
to be converted to the qapi, we could start generating the docs from
qapi-schema.json and avoid the duplication.
There's a problem though. qapi-schema.json is not tied to the QMP protocol
as qmp-commands.hx is, so I'm not sure it's a good place for the examples,
as the examples _are_ QMP specific. On the other hand, I don't know where
should the examples go.
Ideas?
> > I'm not sure how the documentation gets generated but we need to avoid
> > this, otherwise it will quickly get out of sync.
>
> If I remember correctly, Luiz wants to get rid of qmp-commands.hx. But
> we're not there, yet.
Right. We'll be able to do that when we finish to convert all commands
to the qapi. It's unfortunate that we (or maybe me) lost all speed at
the end of the race.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot()
2013-04-10 15:21 ` Markus Armbruster
@ 2013-04-11 3:16 ` Wenchao Xia
0 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 3:16 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
于 2013-4-10 23:21, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> To make it clear about id and name in searching, the API is changed
>> a bit to distinguish them, and caller can choose to search by id or name.
>> Searching will be done with higher priority of id. This function also
>> returns negative value from bdrv_snapshot_list() instead of -ENOENT on
>> error now.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@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 c47a899..7b2026c 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -24,8 +24,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.
>> + */
>
> Let me try to improve:
>
> /**
> * Look up an internal snapshot by @id, or else by @name.
> * @bs: block device to search
> * @sn_info: location to store information on the snapshot found
> * @id: unique snapshot ID, or NULL
> * @name: snapshot name, or NULL
> *
> * If the snapshot with unique ID @id exists, find it.
> * Else, if snapshots with name @name exists, find one of them.
> *
> * Returns: 0 when a snapshot is found, else -errno.
> */
>
I will use it, thanks.
> I'd do two separate functions, one to loop up a snapshot by ID, and one
> to look it up by name, because I find them simpler. Matter of taste.
>
>> 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;
>> @@ -33,16 +45,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 4ad070c..a047a8e 100644
>> --- a/include/block/snapshot.h
>> +++ b/include/block/snapshot.h
>> @@ -33,5 +33,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 88acc38..1d2da99 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2212,7 +2212,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) {
>
> Before your patch: deletes first snapshot sn whose sn->id_str or
> sn->name match name.
>
> After your patch: deletes first snapshot sn whose sn->id_str matches
> name, or else first snapshot whose sn->name matches name.
>
> Running example: say we have the following two snapshots with rather
> unwisely chosen names:
>
> id_str name
> 1 2
> 2 1
>
> Which one will del_existing_snapshots(mon, "2") delete?
>
> Before your patch: first one.
>
> After: second one.
>
> See below for impact.
>
>> @@ -2272,7 +2272,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);
>
> Similar change.
>
> This function is the only user of del_existing_snapshots().
>
> Same example.
>
> Before your patch: "savevm 2" overwrites the first snapshot.
>
> After: it overwrites the second snapshot.
>
> Thus, your patch changes del_existing_snapshots()
>
> I'm fine with the change, but I want it noted *prominently* in the
> commit message that it can change savevm's interpretation of ambiguous
> name arguments.
>
OK.
>> @@ -2363,7 +2363,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) {
>> @@ -2387,7 +2387,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);
>
> Same example.
>
> Before your patch: "loadvm 2" loads the first snapshot.
>
> After: it loads the second snapshot.
>
> Document in commit message, just like the savevm change.
>
>> @@ -2493,7 +2493,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;
>
> Different from the other calls: name argument is NULL rather than same
> as id argument. Let's see what difference it makes.
>
> The loop iterates over all snapshots on the "primary" snapshot device
> (the one that gets the VM state on savevm). For each one, it checks
> whether all other snapshot devices have that snapshot.
>
> Before your patch: a device has the snapshot when it has one whose
> sn->id_str or sn->name match the primary snapshot's id_str. Nuts :)
>
> After your patch: a device has the snapshot when it has one whose
> sn->id_str matches the primary snapshot's id_str.
>
> I'm fine with the change, but it needs to be documented in the commit
> message.
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list()
2013-04-10 15:11 ` Markus Armbruster
@ 2013-04-11 3:19 ` Wenchao Xia
0 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 3:19 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini
于 2013-4-10 23:11, Markus Armbruster 写道:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 04/02/2013 05:47 AM, 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 | 55 ++++++++++++++++++++++++++++++++++++++-----------
>>> include/block/qapi.h | 4 ++-
>>> qemu-img.c | 5 +++-
>>> 3 files changed, 49 insertions(+), 15 deletions(-)
>>
>>> +/*
>>> + * return 0 on success, @p_list will be set only on success, and caller need to
>>
>> s/need/needs/
>>
>>> + * check *p_list on success.
>>
>> I wonder if this wording would be any better:
>>
>> Returns 0 on success, with *p_list either set to describe snapshot
>> information, or NULL because there are no snapshots. Returns -1 on
>> error, with *p_list untouched.
>
> It actually returns -errno then, doesn't it?
>
Yes, I forgot change the comments in the version changing, thank you
for the carefully review.
>>
>>> + */
>>> +int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>>> + SnapshotInfoList **p_list,
>>> + Error **errp)
>>> {
>>
>> At any rate, my only commentary was on grammar and a possible wording
>> for a comment, while the code itself is fine from my viewpoint; so feel
>> free to add:
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>>> +++ b/qemu-img.c
>>> @@ -1735,7 +1735,10 @@ static ImageInfoList
>>> *collect_image_info_list(const char *filename,
>>>
>>> info = g_new0(ImageInfo, 1);
>>> bdrv_collect_image_info(bs, info, filename);
>>> - bdrv_collect_snapshots(bs, info);
>>> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
>>> + info->snapshots) {
>>> + info->has_snapshots = true;
>>> + }
>>
>> Hmm. info->snapshots starts life as NULL (thanks to g_new0), and is
>> untouched on error. Since you are ignoring any errors, you technically
>> could write:
>>
>> bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL);
>> if (info->snapshots) {
>> info->has_snapshots = true;
>> }
>>
>> for the same semantics. That means that as of this commit, no caller
>> cares about the return value of bdrv_query_snapshot_info_list (they only
>> care about whether info->snapshots was changed to non-null), so it could
>> return void for a slightly simpler implementation.
>
> Return the list, or NULL.
>
>> But I don't know if any later patches in the series start to care about
>> which error was returned.
>
> Me neither :)
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list()
2013-04-10 15:19 ` Markus Armbruster
@ 2013-04-11 5:56 ` Wenchao Xia
0 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 5:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
于 2013-4-10 23:19, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> 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>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.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 03369a5..19d4d93 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -23,14 +23,47 @@
>> */
>>
>> #include "block/qapi.h"
>> +#include "block/snapshot.h"
>> #include "block/block_int.h"
>>
>> /*
>> + * check whether the snapshot is valid for whole vm.
>> + *
>> + * @sn: snapshot info to be checked.
>> + * @bs: where @sn was found.
>> + *
>> + * return true if the snapshot is consistent for the VM.
>> + */
>> +static bool snapshot_valid_for_vm(const QEMUSnapshotInfo *sn,
>> + BlockDriverState *bs)
>> +{
>> + BlockDriverState *bs1 = NULL;
>> + QEMUSnapshotInfo s, *sn_info = &s;
>> + int ret;
>> +
>> + /* 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))) {
>
> Unlike load_vmstate(), you don't do
>
> if (!bdrv_is_inserted(bs1) || bdrv_is_read_only(bs1)) {
> continue;
> }
>
> Why?
>
It is copied from do_info_snapshots() and forgot to check it in
load_vmstate(), I think that is not correct, will use the exactly same
logic in load_vmstate() in next version.
>> + if (bs1 != bs && bdrv_can_snapshot(bs1)) {
>> + ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
>> + if (ret < 0) {
>> + return false;
>> + }
>> + }
>> + }
>> + return true;
>> +}
>> +
>> +/*
>> * return 0 on success, @p_list will be set only on success, and caller need to
>> - * check *p_list on success.
>> + * check *p_list on success. If @vm_snapshot is true, limit the results to
>> + * snapshots valid for the whole VM.
>> */
>> int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>> SnapshotInfoList **p_list,
>> + bool vm_snapshot,
>> Error **errp)
>> {
>> int i, sn_count;
>> @@ -59,7 +92,9 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>> }
>>
>> for (i = 0; i < sn_count; i++) {
>> -
>> + if (vm_snapshot && !snapshot_valid_for_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 91dc41b..fe66053 100644
>> --- a/include/block/qapi.h
>> +++ b/include/block/qapi.h
>> @@ -30,6 +30,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 51043ef..261c277 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);
>> - if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL) &&
>> + if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
>> + false, NULL) &&
>> info->snapshots) {
>> info->has_snapshots = true;
>> }
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 07/17] block: add image info query function bdrv_query_image_info()
2013-04-10 15:28 ` Markus Armbruster
@ 2013-04-11 5:59 ` Wenchao Xia
0 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 5:59 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
于 2013-4-10 23:28, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> This patch adds function bdrv_query_image_info(), which will
>> retrieve image info in qmp object format. The implementation is
>> based on the code moved from qemu-img.c, but uses block layer
>> function to get snapshot info.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>> block/qapi.c | 41 ++++++++++++++++++++++++++++++++++-------
>> include/block/qapi.h | 6 +++---
>> qemu-img.c | 8 ++------
>> 3 files changed, 39 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index 19d4d93..176a479 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -122,18 +122,22 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>> return 0;
>> }
>>
>> -void bdrv_collect_image_info(BlockDriverState *bs,
>> - ImageInfo *info,
>> - const char *filename)
>> +/* 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);
>> @@ -150,8 +154,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') {
>
> backing_filename can't possibly be null here, because bs->backing_file
> is an array.
>
OK, will remove it.
>> info->backing_filename = g_strdup(backing_filename);
>> info->has_backing_filename = true;
>> bdrv_get_full_backing_filename(bs, backing_filename2,
>> @@ -168,4 +172,27 @@ 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:
>> + if (info->snapshots) {
>> + info->has_snapshots = true;
>> + }
>> + break;
>> + /* recoverable error */
>> + case -ENOMEDIUM:
>> + error_free(err);
>> + break;
>> + case -ENOTSUP:
>> + error_free(err);
>> + break;
>
> Suggest
>
> case -ENOMEDIUM:
> case -ENOTSUP:
> /* no snapshots on this device */
> error_free(err);
> break;
>
> for clarity.
>
OK.
>> + 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 fe66053..2c62fdf 100644
>> --- a/include/block/qapi.h
>> +++ b/include/block/qapi.h
>> @@ -32,7 +32,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);
>> +int bdrv_query_image_info(BlockDriverState *bs,
>> + ImageInfo **p_info,
>> + Error **errp);
>> #endif
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 261c277..1dd0a60 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1733,12 +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);
>> - if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
>> - false, NULL) &&
>> - info->snapshots) {
>> - info->has_snapshots = true;
>> + if (bdrv_query_image_info(bs, &info, NULL)) {
>> + goto err;
>> }
>>
>> elem = g_new0(ImageInfoList, 1);
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-10 15:51 ` Markus Armbruster
@ 2013-04-11 6:03 ` Wenchao Xia
2013-04-11 12:11 ` Markus Armbruster
0 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 6:03 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
>> # Statistics of a virtual block device or a block backing device.
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 1e0e11e..6b20684 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -1765,6 +1765,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 nanoseconds 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 nanoseconds 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
>
> Not your patch's fault, but here goes anyway: I dislike this
> representation of time.
>
> QMP has time in seconds, milliseconds, nanoseconds, (seconds,
> milliseconds) and (seconds, nanoseconds). There has been no adult
> supervision, obviously (I may say that, because it's as much my fault as
> it's anybody else's).
>
> The sanest one by far is nanoseconds. Good for 2^63 of them. Since pi
> seconds is a nanocentury, good for 2^63 / (pi * 1e9) centuries, which
> should be safely beyond your retirement age.
>
OK, will insert a patch before removing "vm-clock-sec". Have one
question: how to declare uint64_t in qmp-schema.json?
>> + },
>> + {
>> + "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
>> ----------------
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 10/17] qmp: add recursive member in ImageInfo
2013-04-10 16:06 ` Markus Armbruster
@ 2013-04-11 6:06 ` Wenchao Xia
2013-04-11 9:28 ` Markus Armbruster
0 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 6:06 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
于 2013-4-11 0:06, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> New member *backing-image is added to reflect the backing chain
>> status.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>> block/qapi.c | 6 +++++-
>> qapi-schema.json | 5 ++++-
>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index 5e91ab8..fa61c85 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -123,7 +123,11 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>> return 0;
>> }
>>
>> -/* return 0 on success, and @p_info will be set only on success. */
>> +/*
>> + * return 0 on success, and @p_info will be set only on success,
>> + * (*pinfo)->has_backing_image will be false and (*pinfo)->backing_image will
>> + * be NULL.
>> + */
>
> Sounds like this function computes incomplete ImageInfo. Correct? If
> yes, why?
>
yes, qemu-img will use it to get info of an image that may be broken
in backing file chain(can't get backing file's info).
>> int bdrv_query_image_info(BlockDriverState *bs,
>> ImageInfo **p_info,
>> Error **errp)
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 225afef..ad9dd82 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -233,6 +233,8 @@
>> #
>> # @snapshots: #optional list of VM snapshots
>> #
>> +# @backing-image: #optional info of the backing image (since 1.5)
>> +#
>> # Since: 1.3
>> #
>> ##
>> @@ -242,7 +244,8 @@
>> '*actual-size': 'int', 'virtual-size': 'int',
>> '*cluster-size': 'int', '*encrypted': 'bool',
>> '*backing-filename': 'str', '*full-backing-filename': 'str',
>> - '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'] } }
>> + '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
>> + '*backing-image': 'ImageInfo' } }
>>
>> ##
>> # @ImageCheck:
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-10 16:27 ` Markus Armbruster
@ 2013-04-11 6:20 ` Wenchao Xia
0 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 6:20 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
>> "bps_wr":0,
>> "iops":1000000,
>> "iops_rd":0,
>> "iops_wr":0,
>> + "image":{
>> + "filename":"disks/test.img",
>
> Please change to "disks/test.qcow2". I don't want people to get the
> idea they should name their QCOW2 images .img.
>
>> + "format":"qcow2",
>
OK.
> I wonder how inserted.file and inserted.drv are related to
> inserted.image.filename and inserted.image.format. Redundant or not?
> Kevin?
>
From code, I think it is Redundant.
>> + "virtual-size":2048000,
>> + "backing_file":"base.img",
>> + "full-backing-filename":"disks/base.img",
>> + "backing-filename-format:"qcow2",
>> + "snapshots":[
>> + {
>> + "id": "1",
>> + "name": "snapshot1",
>> + "vm-state-size": 0,
>> + "date-sec": 10000200,
>> + "date-nsec": 12,
>> + "vm-clock-sec": 206,
>> + "vm-clock-nsec": 30
>> + }
>> + ],
>> + "backing-image":{
>> + "filename":"disks/base.img",
>> + "format":"qcow2",
>
> And here I wonder how inserted.image.full-backing-filename and
> .backing-filename-format are related to
> inserted.image.backing-image.filename and .backing-image.format. Kevin?
>
Different in source, inserted.image.* is what inserted.image tell,
inserted.image.backing-image.* is what inserted.image.backing-image
tell. Same for user, image.filename should be recommended to use.
>> + "virtual-size":2048000
>> + }
>> + }
>> },
>> "type":"unknown"
>> },
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-10 18:57 ` Luiz Capitulino
@ 2013-04-11 6:25 ` Wenchao Xia
2013-04-11 9:31 ` Markus Armbruster
0 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 6:25 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, Stefan Hajnoczi, pbonzini, Markus Armbruster, qemu-devel
于 2013-4-11 2:57, Luiz Capitulino 写道:
> On Wed, 10 Apr 2013 18:17:04 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>
>>> On Tue, Apr 02, 2013 at 07:47:24PM +0800, Wenchao Xia wrote:
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index 6b20684..b856be7 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -1703,6 +1703,47 @@ Each json-object contain the following:
>>>> - "iops": limit total I/O operations per second (json-int)
>>>> - "iops_rd": limit read operations per second (json-int)
>>>> - "iops_wr": limit write operations per second (json-int)
>>>> + - "image": the detail of the image, it is a json-object containing
>>>> + the following:
>>>> + - "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, not present if image
>>>> + format does not support it (json-int, optional)
>>>> + - "encrypted": true if the image is encrypted, not present means
>>>> + false or the image format does not support
>>>> + encryption (json-bool, optional)
>>>> + - "backing_file": backing file name, not present means no backing
>>>> + file is used or the image format does not
>>>> + support backing file chain
>>>> + (json-string, optional)
>>>> + - "full-backing-filename": full path of the backing file, not
>>>> + present if it equals backing_file or no
>>>> + backing file is used
>>>> + (json-string, optional)
>>>> + - "backing-filename-format": the format of the backing file, not
>>>> + present means unknown or no 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 nanoseconds 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 nanoseconds to be used
>>>> + with vm-clock-sec (json-int)
>>>> + - "backing-image": the detail of the backing image, it is an
>>>> + optional json-object only present when a
>>>> + backing image present for this image
>>>
>>> Please don't duplicate the ImageInfo documentation from
>>> qapi-schema.json.
>>
>> No, this is actually how it still needs to be done.
>>
>> qmp-commands.hx predates qapi-schema.json. We still generate
>> QMP/qmp-commands.txt from qmp-commands.hx, not from qapi-schema.json.
>
> Unfortunately, yes. However, as there are just a few commands missing
> to be converted to the qapi, we could start generating the docs from
> qapi-schema.json and avoid the duplication.
>
> There's a problem though. qapi-schema.json is not tied to the QMP protocol
> as qmp-commands.hx is, so I'm not sure it's a good place for the examples,
I agree that example should not get mixed with qapi-schema.json, which
faces developer instead of user. Personally I think a standalone example
file is better(only call in and out, no text for the structure defines).
> as the examples _are_ QMP specific. On the other hand, I don't know where
> should the examples go.
>
> Ideas?
>
>>> I'm not sure how the documentation gets generated but we need to avoid
>>> this, otherwise it will quickly get out of sync.
>>
>> If I remember correctly, Luiz wants to get rid of qmp-commands.hx. But
>> we're not there, yet.
>
> Right. We'll be able to do that when we finish to convert all commands
> to the qapi. It's unfortunate that we (or maybe me) lost all speed at
> the end of the race.
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 12/17] block: move bdrv_snapshot_dump() and dump_human_image_info() to block/qapi.c
2013-04-10 16:49 ` Markus Armbruster
@ 2013-04-11 6:30 ` Wenchao Xia
2013-04-11 9:50 ` Markus Armbruster
0 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 6:30 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
于 2013-4-11 0:49, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> They are needed later in hmp command, dump_human_image_info()
>> is renamed to bdrv_image_info_dump().
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> I'd be tempted move everything that needs to move to block/qapi.c in one
> go.
>
That is quite a lot of rebasing, and this patch shows tight relation
with following patch(for hmp only), could it be kept as what it is?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-10 17:05 ` Markus Armbruster
@ 2013-04-11 6:35 ` Wenchao Xia
2013-04-11 12:05 ` Markus Armbruster
0 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-11 6:35 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
>>
>> -void bdrv_image_info_dump(ImageInfo *info)
>> +void bdrv_image_info_dump(GString *buf, ImageInfo *info)
>> {
>> char size_buf[128], dsize_buf[128];
>> if (!info->has_actual_size) {
>> @@ -370,43 +369,48 @@ void bdrv_image_info_dump(ImageInfo *info)
>
> I don't like this change, because it introduces buffering for no
> discernible reason. Unless you can show me one, I'd like you to keep
> printing directly.
>
HMP code later need to call this function, and then print buf to
monitor console, which is the goal of this patch.
>> info->actual_size);
>> }
>>
>> static gboolean str_equal_func(gconstpointer a, gconstpointer b)
>> diff --git a/savevm.c b/savevm.c
>> index e4e0008..ce0bbe1 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2466,7 +2466,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>> int nb_sns, i, ret, available;
>> int total;
>> int *available_snapshots;
>> - char buf[256];
>> + GString *buf = NULL;
>
> Useless initialization. But if you keep bdrv_snapshot_dump() printing,
> it all goes away.
>
will remove.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-10 7:30 ` Wenchao Xia
@ 2013-04-11 8:54 ` Stefan Hajnoczi
0 siblings, 0 replies; 67+ messages in thread
From: Stefan Hajnoczi @ 2013-04-11 8:54 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, pbonzini, lcapitulino, armbru, qemu-devel
On Wed, Apr 10, 2013 at 03:30:10PM +0800, Wenchao Xia wrote:
> 于 2013-4-8 21:33, Stefan Hajnoczi 写道:
> >On Tue, Apr 02, 2013 at 07:47:24PM +0800, Wenchao Xia wrote:
> >>diff --git a/qmp-commands.hx b/qmp-commands.hx
> >>index 6b20684..b856be7 100644
> >>--- a/qmp-commands.hx
> >>+++ b/qmp-commands.hx
> >>@@ -1703,6 +1703,47 @@ Each json-object contain the following:
> >> - "iops": limit total I/O operations per second (json-int)
> >> - "iops_rd": limit read operations per second (json-int)
> >> - "iops_wr": limit write operations per second (json-int)
> >>+ - "image": the detail of the image, it is a json-object containing
> >>+ the following:
> >>+ - "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, not present if image
> >>+ format does not support it (json-int, optional)
> >>+ - "encrypted": true if the image is encrypted, not present means
> >>+ false or the image format does not support
> >>+ encryption (json-bool, optional)
> >>+ - "backing_file": backing file name, not present means no backing
> >>+ file is used or the image format does not
> >>+ support backing file chain
> >>+ (json-string, optional)
> >>+ - "full-backing-filename": full path of the backing file, not
> >>+ present if it equals backing_file or no
> >>+ backing file is used
> >>+ (json-string, optional)
> >>+ - "backing-filename-format": the format of the backing file, not
> >>+ present means unknown or no 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 nanoseconds 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 nanoseconds to be used
> >>+ with vm-clock-sec (json-int)
> >>+ - "backing-image": the detail of the backing image, it is an
> >>+ optional json-object only present when a
> >>+ backing image present for this image
> >
> >Please don't duplicate the ImageInfo documentation from
> >qapi-schema.json.
> >
> >I'm not sure how the documentation gets generated but we need to avoid
> >this, otherwise it will quickly get out of sync.
> >
> >Stefan
> >
> So remove this section? It seems no other way to sync them except
> manually change both of them.
Given the discussion about duplication, this can stay for now.
Stefan
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 10/17] qmp: add recursive member in ImageInfo
2013-04-11 6:06 ` Wenchao Xia
@ 2013-04-11 9:28 ` Markus Armbruster
0 siblings, 0 replies; 67+ messages in thread
From: Markus Armbruster @ 2013-04-11 9:28 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, lcapitulino, qemu-devel, pbonzini
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 于 2013-4-11 0:06, Markus Armbruster 写道:
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>
>>> New member *backing-image is added to reflect the backing chain
>>> status.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>> block/qapi.c | 6 +++++-
>>> qapi-schema.json | 5 ++++-
>>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/qapi.c b/block/qapi.c
>>> index 5e91ab8..fa61c85 100644
>>> --- a/block/qapi.c
>>> +++ b/block/qapi.c
>>> @@ -123,7 +123,11 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
>>> return 0;
>>> }
>>>
>>> -/* return 0 on success, and @p_info will be set only on success. */
>>> +/*
>>> + * return 0 on success, and @p_info will be set only on success,
>>> + * (*pinfo)->has_backing_image will be false and (*pinfo)->backing_image will
>>> + * be NULL.
>>> + */
>>
>> Sounds like this function computes incomplete ImageInfo. Correct? If
>> yes, why?
>>
> yes, qemu-img will use it to get info of an image that may be broken
> in backing file chain(can't get backing file's info).
Ah, that makes sense.
Suggest:
/**
* bdrv_query_image_info:
* @bs: block device to examine
* @p_info: location to store image information
* @errp: location to store error information
*
* Store "flat" image inforation in @p_info.
*
* "Flat" means it does *not* query backing image information,
* i.e. (*pinfo)->has_backing_image will be set to false and
* (*pinfo)->backing_image to NULL even when the image does in fact have
* a backing image.
*
* On error, store error in @errp.
*
* Returns: 0 on success, -errno on error.
*/
>>> int bdrv_query_image_info(BlockDriverState *bs,
>>> ImageInfo **p_info,
>>> Error **errp)
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 225afef..ad9dd82 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -233,6 +233,8 @@
>>> #
>>> # @snapshots: #optional list of VM snapshots
>>> #
>>> +# @backing-image: #optional info of the backing image (since 1.5)
>>> +#
>>> # Since: 1.3
>>> #
>>> ##
>>> @@ -242,7 +244,8 @@
>>> '*actual-size': 'int', 'virtual-size': 'int',
>>> '*cluster-size': 'int', '*encrypted': 'bool',
>>> '*backing-filename': 'str', '*full-backing-filename': 'str',
>>> - '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'] } }
>>> + '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
>>> + '*backing-image': 'ImageInfo' } }
>>>
>>> ##
>>> # @ImageCheck:
>>
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-11 6:25 ` Wenchao Xia
@ 2013-04-11 9:31 ` Markus Armbruster
2013-04-12 3:25 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-11 9:31 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, Stefan Hajnoczi, pbonzini, qemu-devel, Luiz Capitulino
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 于 2013-4-11 2:57, Luiz Capitulino 写道:
>> On Wed, 10 Apr 2013 18:17:04 +0200
>> Markus Armbruster <armbru@redhat.com> wrote:
>>
>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>
>>>> On Tue, Apr 02, 2013 at 07:47:24PM +0800, Wenchao Xia wrote:
>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>>> index 6b20684..b856be7 100644
>>>>> --- a/qmp-commands.hx
>>>>> +++ b/qmp-commands.hx
>>>>> @@ -1703,6 +1703,47 @@ Each json-object contain the following:
>>>>> - "iops": limit total I/O operations per second (json-int)
>>>>> - "iops_rd": limit read operations per second (json-int)
>>>>> - "iops_wr": limit write operations per second (json-int)
>>>>> + - "image": the detail of the image, it is a json-object
>>>>> containing
>>>>> + the following:
>>>>> + - "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, not present if image
>>>>> + format does not support it (json-int, optional)
>>>>> + - "encrypted": true if the image is encrypted, not present
>>>>> means
>>>>> + false or the image format does not support
>>>>> + encryption (json-bool, optional)
>>>>> + - "backing_file": backing file name, not present means no backing
>>>>> + file is used or the image format does not
>>>>> + support backing file chain
>>>>> + (json-string, optional)
>>>>> + - "full-backing-filename": full path of the backing file, not
>>>>> + present if it equals backing_file or no
>>>>> + backing file is used
>>>>> + (json-string, optional)
>>>>> + - "backing-filename-format": the format of the backing file,
>>>>> not
>>>>> + present means unknown or no 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 nanoseconds 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 nanoseconds to be used
>>>>> + with vm-clock-sec (json-int)
>>>>> + - "backing-image": the detail of the backing image, it is an
>>>>> + optional json-object only present when a
>>>>> + backing image present for this image
>>>>
>>>> Please don't duplicate the ImageInfo documentation from
>>>> qapi-schema.json.
>>>
>>> No, this is actually how it still needs to be done.
>>>
>>> qmp-commands.hx predates qapi-schema.json. We still generate
>>> QMP/qmp-commands.txt from qmp-commands.hx, not from qapi-schema.json.
>>
>> Unfortunately, yes. However, as there are just a few commands missing
>> to be converted to the qapi, we could start generating the docs from
>> qapi-schema.json and avoid the duplication.
>>
>> There's a problem though. qapi-schema.json is not tied to the QMP protocol
>> as qmp-commands.hx is, so I'm not sure it's a good place for the examples,
> I agree that example should not get mixed with qapi-schema.json, which
> faces developer instead of user. Personally I think a standalone example
> file is better(only call in and out, no text for the structure defines).
How do we ensure the examples stay up-to-date?
Crazy idea: generate them as part of "make check".
[...]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 12/17] block: move bdrv_snapshot_dump() and dump_human_image_info() to block/qapi.c
2013-04-11 6:30 ` Wenchao Xia
@ 2013-04-11 9:50 ` Markus Armbruster
0 siblings, 0 replies; 67+ messages in thread
From: Markus Armbruster @ 2013-04-11 9:50 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, lcapitulino, qemu-devel, pbonzini
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 于 2013-4-11 0:49, Markus Armbruster 写道:
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>
>>> They are needed later in hmp command, dump_human_image_info()
>>> is renamed to bdrv_image_info_dump().
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>
>> I'd be tempted move everything that needs to move to block/qapi.c in one
>> go.
>>
> That is quite a lot of rebasing, and this patch shows tight relation
> with following patch(for hmp only), could it be kept as what it is?
Your choice.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-11 6:35 ` Wenchao Xia
@ 2013-04-11 12:05 ` Markus Armbruster
2013-04-12 4:51 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-11 12:05 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, lcapitulino, qemu-devel, pbonzini
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>>
>>> -void bdrv_image_info_dump(ImageInfo *info)
>>> +void bdrv_image_info_dump(GString *buf, ImageInfo *info)
>>> {
>>> char size_buf[128], dsize_buf[128];
>>> if (!info->has_actual_size) {
>>> @@ -370,43 +369,48 @@ void bdrv_image_info_dump(ImageInfo *info)
>>
>> I don't like this change, because it introduces buffering for no
>> discernible reason. Unless you can show me one, I'd like you to keep
>> printing directly.
>>
> HMP code later need to call this function, and then print buf to
> monitor console, which is the goal of this patch.
Aha, the actual problem is "print either to stdout or to current
monitor", so that we can share code among qemu-img and monitor commands.
Such sharing is good, and the problem is real.
bdrv_snapshot_dump() solves it this way: print to a fixed-size buffer,
which the caller either prints to stdout (qemu-img) or to the current
monitor (info snapshots).
Your patch makes the buffer flexible. Improvement.
But in my opinion, the detour through the buffer is silly. Why not
simply use a print function that does the right thing?
We already have such a function for "print either to stderr or to
current monitor": error_printf(). Could easily add one for stdout.
[...]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-11 6:03 ` Wenchao Xia
@ 2013-04-11 12:11 ` Markus Armbruster
2013-04-11 12:44 ` Luiz Capitulino
0 siblings, 1 reply; 67+ messages in thread
From: Markus Armbruster @ 2013-04-11 12:11 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, lcapitulino, qemu-devel, pbonzini
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>> # Statistics of a virtual block device or a block backing device.
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index 1e0e11e..6b20684 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -1765,6 +1765,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 nanoseconds 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 nanoseconds 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
>>
>> Not your patch's fault, but here goes anyway: I dislike this
>> representation of time.
>>
>> QMP has time in seconds, milliseconds, nanoseconds, (seconds,
>> milliseconds) and (seconds, nanoseconds). There has been no adult
>> supervision, obviously (I may say that, because it's as much my fault as
>> it's anybody else's).
>>
>> The sanest one by far is nanoseconds. Good for 2^63 of them. Since pi
>> seconds is a nanocentury, good for 2^63 / (pi * 1e9) centuries, which
>> should be safely beyond your retirement age.
>>
> OK, will insert a patch before removing "vm-clock-sec".
Before you do that, let's get Luiz's blessing.
> Have one
> question: how to declare uint64_t in qmp-schema.json?
You can't. All we got is 'int'. The implementation restricts 'int'
values to 64 bits signed. Plenty of range for this purpose.
[...]
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (19 preceding siblings ...)
2013-04-08 13:43 ` Stefan Hajnoczi
@ 2013-04-11 12:17 ` Markus Armbruster
20 siblings, 0 replies; 67+ messages in thread
From: Markus Armbruster @ 2013-04-11 12:17 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini
Out of steam, got until 13/17. Rest is HMP; hope to cover that in the
next round. I had a few comments here and there, but nevertheless the
series feels pretty close to commit now.
Related to Pavel's work, cc'ing him.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-11 12:11 ` Markus Armbruster
@ 2013-04-11 12:44 ` Luiz Capitulino
2013-04-11 13:08 ` Eric Blake
0 siblings, 1 reply; 67+ messages in thread
From: Luiz Capitulino @ 2013-04-11 12:44 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, qemu-devel, Wenchao Xia, pbonzini
On Thu, 11 Apr 2013 14:11:23 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
> >>> # Statistics of a virtual block device or a block backing device.
> >>> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >>> index 1e0e11e..6b20684 100644
> >>> --- a/qmp-commands.hx
> >>> +++ b/qmp-commands.hx
> >>> @@ -1765,6 +1765,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 nanoseconds 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 nanoseconds 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
> >>
> >> Not your patch's fault, but here goes anyway: I dislike this
> >> representation of time.
> >>
> >> QMP has time in seconds, milliseconds, nanoseconds, (seconds,
> >> milliseconds) and (seconds, nanoseconds). There has been no adult
> >> supervision, obviously (I may say that, because it's as much my fault as
> >> it's anybody else's).
> >>
> >> The sanest one by far is nanoseconds. Good for 2^63 of them. Since pi
> >> seconds is a nanocentury, good for 2^63 / (pi * 1e9) centuries, which
> >> should be safely beyond your retirement age.
> >>
> > OK, will insert a patch before removing "vm-clock-sec".
Are you saying you're going to drop vm-clock-sec?
> Before you do that, let's get Luiz's blessing.
Fine with me if I got it right.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-11 12:44 ` Luiz Capitulino
@ 2013-04-11 13:08 ` Eric Blake
2013-04-11 13:39 ` Luiz Capitulino
0 siblings, 1 reply; 67+ messages in thread
From: Eric Blake @ 2013-04-11 13:08 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, stefanha, Markus Armbruster, qemu-devel, pbonzini,
Wenchao Xia
[-- Attachment #1: Type: text/plain, Size: 2353 bytes --]
On 04/11/2013 06:44 AM, Luiz Capitulino wrote:
>>>>> +-> { "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
>>>>
>>>> Not your patch's fault, but here goes anyway: I dislike this
>>>> representation of time.
>>>>
>>>> QMP has time in seconds, milliseconds, nanoseconds, (seconds,
> Are you saying you're going to drop vm-clock-sec?
>
>> Before you do that, let's get Luiz's blessing.
>
> Fine with me if I got it right.
Problem. Let's go back to the original definition that we are talking
about modifying:
+#
+# Since: 1.5
+##
+{ 'command': 'query-snapshots',
+ 'returns': ['SnapshotInfo'] }
+
Now let's look for that type:
# @vm-clock-sec: VM clock relative to boot in seconds
#
# @vm-clock-nsec: fractional part in nano seconds to be used with
vm-clock-sec
#
# Since: 1.3
#
##
{ 'type': 'SnapshotInfo',
'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int',
'date-sec': 'int', 'date-nsec': 'int',
'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
And that type is already used in 'ImageInfo'.
In other words, we're stuck. We've already cemented the mistake. You
_can't_ drop vm-clock-sec, without breaking the behavior of management
apps written against the 1.3 interface when dealing with ImageInfo. If
you want to avoid a (sec/nsec) pair, you would have to invent a new type
instead of reusing the already-existing SnapshotInfo.
Hmm, as I typed that, I did another search of qemu-schema.json - we have
the type 'ImageInfo' defined, but none of the existing 'command's ever
call out the use of that type. Is it a type we are only using
internally to date, and where this is the first QMP command that would
actually expose SnapshotInfo or ImageInfo to a management app? Then
maybe we _CAN_ modify SnapshotInfo, clean up all the internal code, and
provide a saner type for the first time that QMP can actually use it.
--
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] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-11 13:08 ` Eric Blake
@ 2013-04-11 13:39 ` Luiz Capitulino
2013-04-12 3:17 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Luiz Capitulino @ 2013-04-11 13:39 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, stefanha, Markus Armbruster, qemu-devel, pbonzini,
Wenchao Xia
On Thu, 11 Apr 2013 07:08:41 -0600
Eric Blake <eblake@redhat.com> wrote:
> On 04/11/2013 06:44 AM, Luiz Capitulino wrote:
>
> >>>>> +-> { "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
> >>>>
> >>>> Not your patch's fault, but here goes anyway: I dislike this
> >>>> representation of time.
> >>>>
> >>>> QMP has time in seconds, milliseconds, nanoseconds, (seconds,
>
> > Are you saying you're going to drop vm-clock-sec?
> >
> >> Before you do that, let's get Luiz's blessing.
> >
> > Fine with me if I got it right.
>
> Problem. Let's go back to the original definition that we are talking
> about modifying:
>
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'query-snapshots',
> + 'returns': ['SnapshotInfo'] }
> +
>
> Now let's look for that type:
>
> # @vm-clock-sec: VM clock relative to boot in seconds
> #
> # @vm-clock-nsec: fractional part in nano seconds to be used with
> vm-clock-sec
> #
> # Since: 1.3
> #
> ##
>
> { 'type': 'SnapshotInfo',
> 'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int',
> 'date-sec': 'int', 'date-nsec': 'int',
> 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>
>
> And that type is already used in 'ImageInfo'.
>
> In other words, we're stuck. We've already cemented the mistake. You
> _can't_ drop vm-clock-sec, without breaking the behavior of management
> apps written against the 1.3 interface when dealing with ImageInfo. If
> you want to avoid a (sec/nsec) pair, you would have to invent a new type
> instead of reusing the already-existing SnapshotInfo.
You're right. I actually replied too quickly and thought that we were talking
about the types being introduced by this patch. Sorry for that.
> Hmm, as I typed that, I did another search of qemu-schema.json - we have
> the type 'ImageInfo' defined, but none of the existing 'command's ever
> call out the use of that type. Is it a type we are only using
> internally to date, and where this is the first QMP command that would
> actually expose SnapshotInfo or ImageInfo to a management app? Then
> maybe we _CAN_ modify SnapshotInfo, clean up all the internal code, and
> provide a saner type for the first time that QMP can actually use it.
IIRC, it's being used by qemu-img. As it's not trivial to determine if
the field has users and as what goes in the schema is to be considered stable,
it's better to keep the field. Instead, we could add a better field and
deprecate the current one.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-11 13:39 ` Luiz Capitulino
@ 2013-04-12 3:17 ` Wenchao Xia
2013-04-12 3:22 ` Eric Blake
0 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-12 3:17 UTC (permalink / raw)
To: Luiz Capitulino; +Cc: kwolf, stefanha, Markus Armbruster, qemu-devel, pbonzini
于 2013-4-11 21:39, Luiz Capitulino 写道:
> On Thu, 11 Apr 2013 07:08:41 -0600
> Eric Blake <eblake@redhat.com> wrote:
>
>> On 04/11/2013 06:44 AM, Luiz Capitulino wrote:
>>
>>>>>>> +-> { "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
>>>>>>
>>>>>> Not your patch's fault, but here goes anyway: I dislike this
>>>>>> representation of time.
>>>>>>
>>>>>> QMP has time in seconds, milliseconds, nanoseconds, (seconds,
>>
>>> Are you saying you're going to drop vm-clock-sec?
>>>
>>>> Before you do that, let's get Luiz's blessing.
>>>
>>> Fine with me if I got it right.
>>
>> Problem. Let's go back to the original definition that we are talking
>> about modifying:
>>
>> +#
>> +# Since: 1.5
>> +##
>> +{ 'command': 'query-snapshots',
>> + 'returns': ['SnapshotInfo'] }
>> +
>>
>> Now let's look for that type:
>>
>> # @vm-clock-sec: VM clock relative to boot in seconds
>> #
>> # @vm-clock-nsec: fractional part in nano seconds to be used with
>> vm-clock-sec
>> #
>> # Since: 1.3
>> #
>> ##
>>
>> { 'type': 'SnapshotInfo',
>> 'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int',
>> 'date-sec': 'int', 'date-nsec': 'int',
>> 'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
>>
>>
>> And that type is already used in 'ImageInfo'.
>>
>> In other words, we're stuck. We've already cemented the mistake. You
>> _can't_ drop vm-clock-sec, without breaking the behavior of management
>> apps written against the 1.3 interface when dealing with ImageInfo. If
>> you want to avoid a (sec/nsec) pair, you would have to invent a new type
>> instead of reusing the already-existing SnapshotInfo.
>
> You're right. I actually replied too quickly and thought that we were talking
> about the types being introduced by this patch. Sorry for that.
>
>> Hmm, as I typed that, I did another search of qemu-schema.json - we have
>> the type 'ImageInfo' defined, but none of the existing 'command's ever
>> call out the use of that type. Is it a type we are only using
>> internally to date, and where this is the first QMP command that would
>> actually expose SnapshotInfo or ImageInfo to a management app? Then
>> maybe we _CAN_ modify SnapshotInfo, clean up all the internal code, and
>> provide a saner type for the first time that QMP can actually use it.
>
> IIRC, it's being used by qemu-img. As it's not trivial to determine if
> the field has users and as what goes in the schema is to be considered stable,
> it's better to keep the field. Instead, we could add a better field and
> deprecate the current one.
>
I remember there is a patch laster year made "qemu-img info" dump out
json strings, where vm-clock-sec is dumped out.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-12 3:17 ` Wenchao Xia
@ 2013-04-12 3:22 ` Eric Blake
2013-04-12 9:39 ` Wenchao Xia
0 siblings, 1 reply; 67+ messages in thread
From: Eric Blake @ 2013-04-12 3:22 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, stefanha, qemu-devel, Markus Armbruster, pbonzini,
Luiz Capitulino
[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]
On 04/11/2013 09:17 PM, Wenchao Xia wrote:
>>
>>> Hmm, as I typed that, I did another search of qemu-schema.json - we have
>>> the type 'ImageInfo' defined, but none of the existing 'command's ever
>>> call out the use of that type. Is it a type we are only using
>>> internally to date, and where this is the first QMP command that would
>>> actually expose SnapshotInfo or ImageInfo to a management app? Then
>>> maybe we _CAN_ modify SnapshotInfo, clean up all the internal code, and
>>> provide a saner type for the first time that QMP can actually use it.
>>
>> IIRC, it's being used by qemu-img. As it's not trivial to determine if
>> the field has users and as what goes in the schema is to be considered
>> stable,
>> it's better to keep the field. Instead, we could add a better field and
>> deprecate the current one.
>>
> I remember there is a patch laster year made "qemu-img info" dump out
> json strings, where vm-clock-sec is dumped out.
Ah, so it's not QMP exposing it, but qemu-info in JSON mode. Still,
changing that output to delete a field might break existing tools;
adding a field might be safer, but then you'd have redundant information
in the old style for old tools, as well as the added field.
--
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] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block
2013-04-11 9:31 ` Markus Armbruster
@ 2013-04-12 3:25 ` Wenchao Xia
0 siblings, 0 replies; 67+ messages in thread
From: Wenchao Xia @ 2013-04-12 3:25 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, Stefan Hajnoczi, pbonzini, qemu-devel, Luiz Capitulino
于 2013-4-11 17:31, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>> 于 2013-4-11 2:57, Luiz Capitulino 写道:
>>> On Wed, 10 Apr 2013 18:17:04 +0200
>>> Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>>> Stefan Hajnoczi <stefanha@gmail.com> writes:
>>>>
>>>>> On Tue, Apr 02, 2013 at 07:47:24PM +0800, Wenchao Xia wrote:
>>>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>>>> index 6b20684..b856be7 100644
>>>>>> --- a/qmp-commands.hx
>>>>>> +++ b/qmp-commands.hx
>>>>>> @@ -1703,6 +1703,47 @@ Each json-object contain the following:
>>>>>> - "iops": limit total I/O operations per second (json-int)
>>>>>> - "iops_rd": limit read operations per second (json-int)
>>>>>> - "iops_wr": limit write operations per second (json-int)
>>>>>> + - "image": the detail of the image, it is a json-object
>>>>>> containing
>>>>>> + the following:
>>>>>> + - "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, not present if image
>>>>>> + format does not support it (json-int, optional)
>>>>>> + - "encrypted": true if the image is encrypted, not present
>>>>>> means
>>>>>> + false or the image format does not support
>>>>>> + encryption (json-bool, optional)
>>>>>> + - "backing_file": backing file name, not present means no backing
>>>>>> + file is used or the image format does not
>>>>>> + support backing file chain
>>>>>> + (json-string, optional)
>>>>>> + - "full-backing-filename": full path of the backing file, not
>>>>>> + present if it equals backing_file or no
>>>>>> + backing file is used
>>>>>> + (json-string, optional)
>>>>>> + - "backing-filename-format": the format of the backing file,
>>>>>> not
>>>>>> + present means unknown or no 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 nanoseconds 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 nanoseconds to be used
>>>>>> + with vm-clock-sec (json-int)
>>>>>> + - "backing-image": the detail of the backing image, it is an
>>>>>> + optional json-object only present when a
>>>>>> + backing image present for this image
>>>>>
>>>>> Please don't duplicate the ImageInfo documentation from
>>>>> qapi-schema.json.
>>>>
>>>> No, this is actually how it still needs to be done.
>>>>
>>>> qmp-commands.hx predates qapi-schema.json. We still generate
>>>> QMP/qmp-commands.txt from qmp-commands.hx, not from qapi-schema.json.
>>>
>>> Unfortunately, yes. However, as there are just a few commands missing
>>> to be converted to the qapi, we could start generating the docs from
>>> qapi-schema.json and avoid the duplication.
>>>
>>> There's a problem though. qapi-schema.json is not tied to the QMP protocol
>>> as qmp-commands.hx is, so I'm not sure it's a good place for the examples,
>> I agree that example should not get mixed with qapi-schema.json, which
>> faces developer instead of user. Personally I think a standalone example
>> file is better(only call in and out, no text for the structure defines).
>
> How do we ensure the examples stay up-to-date?
>
> Crazy idea: generate them as part of "make check".
>
> [...]
>
Generate example from qapi-schema.json is cool, maybe do it
in "make".
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-11 12:05 ` Markus Armbruster
@ 2013-04-12 4:51 ` Wenchao Xia
2013-04-12 11:51 ` Markus Armbruster
0 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-12 4:51 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, stefanha, pbonzini, qemu-devel, lcapitulino
于 2013-4-11 20:05, Markus Armbruster 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>
>>>>
>>>> -void bdrv_image_info_dump(ImageInfo *info)
>>>> +void bdrv_image_info_dump(GString *buf, ImageInfo *info)
>>>> {
>>>> char size_buf[128], dsize_buf[128];
>>>> if (!info->has_actual_size) {
>>>> @@ -370,43 +369,48 @@ void bdrv_image_info_dump(ImageInfo *info)
>>>
>>> I don't like this change, because it introduces buffering for no
>>> discernible reason. Unless you can show me one, I'd like you to keep
>>> printing directly.
>>>
>> HMP code later need to call this function, and then print buf to
>> monitor console, which is the goal of this patch.
>
> Aha, the actual problem is "print either to stdout or to current
> monitor", so that we can share code among qemu-img and monitor commands.
> Such sharing is good, and the problem is real.
>
> bdrv_snapshot_dump() solves it this way: print to a fixed-size buffer,
> which the caller either prints to stdout (qemu-img) or to the current
> monitor (info snapshots).
>
> Your patch makes the buffer flexible. Improvement.
>
> But in my opinion, the detour through the buffer is silly. Why not
> simply use a print function that does the right thing?
>
> We already have such a function for "print either to stderr or to
> current monitor": error_printf(). Could easily add one for stdout.
>
> [...]
>
Indeed it is a better way, how about:
void message_printf(const char *fmt, ...);
I am not sure where should this function be placed,
hmp.c?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-12 3:22 ` Eric Blake
@ 2013-04-12 9:39 ` Wenchao Xia
2013-04-12 11:45 ` Markus Armbruster
0 siblings, 1 reply; 67+ messages in thread
From: Wenchao Xia @ 2013-04-12 9:39 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, stefanha, qemu-devel, Markus Armbruster, pbonzini,
Luiz Capitulino
于 2013-4-12 11:22, Eric Blake 写道:
> On 04/11/2013 09:17 PM, Wenchao Xia wrote:
>
>>>
>>>> Hmm, as I typed that, I did another search of qemu-schema.json - we have
>>>> the type 'ImageInfo' defined, but none of the existing 'command's ever
>>>> call out the use of that type. Is it a type we are only using
>>>> internally to date, and where this is the first QMP command that would
>>>> actually expose SnapshotInfo or ImageInfo to a management app? Then
>>>> maybe we _CAN_ modify SnapshotInfo, clean up all the internal code, and
>>>> provide a saner type for the first time that QMP can actually use it.
>>>
>>> IIRC, it's being used by qemu-img. As it's not trivial to determine if
>>> the field has users and as what goes in the schema is to be considered
>>> stable,
>>> it's better to keep the field. Instead, we could add a better field and
>>> deprecate the current one.
>>>
>> I remember there is a patch laster year made "qemu-img info" dump out
>> json strings, where vm-clock-sec is dumped out.
>
> Ah, so it's not QMP exposing it, but qemu-info in JSON mode. Still,
> changing that output to delete a field might break existing tools;
> adding a field might be safer, but then you'd have redundant information
> in the old style for old tools, as well as the added field.
>
It seems keeping it, is what best we can do now. Markus, are you OK
with it?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots
2013-04-12 9:39 ` Wenchao Xia
@ 2013-04-12 11:45 ` Markus Armbruster
0 siblings, 0 replies; 67+ messages in thread
From: Markus Armbruster @ 2013-04-12 11:45 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, Luiz Capitulino, pbonzini
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 于 2013-4-12 11:22, Eric Blake 写道:
>> On 04/11/2013 09:17 PM, Wenchao Xia wrote:
>>
>>>>
>>>>> Hmm, as I typed that, I did another search of qemu-schema.json - we have
>>>>> the type 'ImageInfo' defined, but none of the existing 'command's ever
>>>>> call out the use of that type. Is it a type we are only using
>>>>> internally to date, and where this is the first QMP command that would
>>>>> actually expose SnapshotInfo or ImageInfo to a management app? Then
>>>>> maybe we _CAN_ modify SnapshotInfo, clean up all the internal code, and
>>>>> provide a saner type for the first time that QMP can actually use it.
>>>>
>>>> IIRC, it's being used by qemu-img. As it's not trivial to determine if
>>>> the field has users and as what goes in the schema is to be considered
>>>> stable,
>>>> it's better to keep the field. Instead, we could add a better field and
>>>> deprecate the current one.
>>>>
>>> I remember there is a patch laster year made "qemu-img info" dump out
>>> json strings, where vm-clock-sec is dumped out.
>>
>> Ah, so it's not QMP exposing it, but qemu-info in JSON mode. Still,
>> changing that output to delete a field might break existing tools;
>> adding a field might be safer, but then you'd have redundant information
>> in the old style for old tools, as well as the added field.
>>
> It seems keeping it, is what best we can do now. Markus, are you OK
> with it?
Yes, because:
"qemu-img info --output=json" exists since commit c054b3f, v1.3.0. I
agree we shouldn't change it incompatibly just because we hate the way
time is represented there.
Doesn't mean we *have* to duplicate the ugly time representation into
QMP. But avoiding it there involves duplicating SnapshotInfo and
ImageInfo in the schema. Not worth it, considering there's no
consistency in QMP's time representation anyway.
^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-12 4:51 ` Wenchao Xia
@ 2013-04-12 11:51 ` Markus Armbruster
0 siblings, 0 replies; 67+ messages in thread
From: Markus Armbruster @ 2013-04-12 11:51 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, lcapitulino, qemu-devel, pbonzini
Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
> 于 2013-4-11 20:05, Markus Armbruster 写道:
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> writes:
>>
>>>>>
>>>>> -void bdrv_image_info_dump(ImageInfo *info)
>>>>> +void bdrv_image_info_dump(GString *buf, ImageInfo *info)
>>>>> {
>>>>> char size_buf[128], dsize_buf[128];
>>>>> if (!info->has_actual_size) {
>>>>> @@ -370,43 +369,48 @@ void bdrv_image_info_dump(ImageInfo *info)
>>>>
>>>> I don't like this change, because it introduces buffering for no
>>>> discernible reason. Unless you can show me one, I'd like you to keep
>>>> printing directly.
>>>>
>>> HMP code later need to call this function, and then print buf to
>>> monitor console, which is the goal of this patch.
>>
>> Aha, the actual problem is "print either to stdout or to current
>> monitor", so that we can share code among qemu-img and monitor commands.
>> Such sharing is good, and the problem is real.
>>
>> bdrv_snapshot_dump() solves it this way: print to a fixed-size buffer,
>> which the caller either prints to stdout (qemu-img) or to the current
>> monitor (info snapshots).
>>
>> Your patch makes the buffer flexible. Improvement.
>>
>> But in my opinion, the detour through the buffer is silly. Why not
>> simply use a print function that does the right thing?
>>
>> We already have such a function for "print either to stderr or to
>> current monitor": error_printf(). Could easily add one for stdout.
>>
>> [...]
>>
> Indeed it is a better way, how about:
> void message_printf(const char *fmt, ...);
> I am not sure where should this function be placed,
> hmp.c?
I figure it needs to go in a file that's linked into the utilities, too.
I think these are in make variables util-obj-y, stub-obj-y.
I'd be tempted to put it into qemu-error.c. If you don't like that
because it's not about errors, consider renaming the file :)
^ permalink raw reply [flat|nested] 67+ messages in thread
end of thread, other threads:[~2013-04-12 11:51 UTC | newest]
Thread overview: 67+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-02 11:47 [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 01/17] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
2013-04-10 15:21 ` Markus Armbruster
2013-04-11 3:16 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 03/17] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 04/17] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 05/17] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
2013-04-03 1:17 ` Eric Blake
2013-04-03 5:24 ` Wenchao Xia
2013-04-10 15:11 ` Markus Armbruster
2013-04-11 3:19 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 06/17] block: add check for VM snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
2013-04-10 15:19 ` Markus Armbruster
2013-04-11 5:56 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 07/17] block: add image info query function bdrv_query_image_info() Wenchao Xia
2013-04-10 15:28 ` Markus Armbruster
2013-04-11 5:59 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 08/17] block: move qmp_query_block() and bdrv_query_info() to block/qapi.c Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 09/17] qmp: add interface query-snapshots Wenchao Xia
2013-04-08 13:28 ` Stefan Hajnoczi
2013-04-10 15:51 ` Markus Armbruster
2013-04-11 6:03 ` Wenchao Xia
2013-04-11 12:11 ` Markus Armbruster
2013-04-11 12:44 ` Luiz Capitulino
2013-04-11 13:08 ` Eric Blake
2013-04-11 13:39 ` Luiz Capitulino
2013-04-12 3:17 ` Wenchao Xia
2013-04-12 3:22 ` Eric Blake
2013-04-12 9:39 ` Wenchao Xia
2013-04-12 11:45 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 10/17] qmp: add recursive member in ImageInfo Wenchao Xia
2013-04-10 16:06 ` Markus Armbruster
2013-04-11 6:06 ` Wenchao Xia
2013-04-11 9:28 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 11/17] qmp: add ImageInfo in BlockDeviceInfo used by query-block Wenchao Xia
2013-04-08 10:04 ` Kevin Wolf
2013-04-08 13:33 ` Stefan Hajnoczi
2013-04-10 7:30 ` Wenchao Xia
2013-04-11 8:54 ` Stefan Hajnoczi
2013-04-10 16:17 ` Markus Armbruster
2013-04-10 18:57 ` Luiz Capitulino
2013-04-11 6:25 ` Wenchao Xia
2013-04-11 9:31 ` Markus Armbruster
2013-04-12 3:25 ` Wenchao Xia
2013-04-10 16:27 ` Markus Armbruster
2013-04-11 6:20 ` Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 12/17] block: move bdrv_snapshot_dump() and dump_human_image_info() to block/qapi.c Wenchao Xia
2013-04-10 16:49 ` Markus Armbruster
2013-04-11 6:30 ` Wenchao Xia
2013-04-11 9:50 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 13/17] block: dump to buffer for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
2013-04-08 13:36 ` Stefan Hajnoczi
2013-04-10 16:56 ` Markus Armbruster
2013-04-10 17:05 ` Markus Armbruster
2013-04-11 6:35 ` Wenchao Xia
2013-04-11 12:05 ` Markus Armbruster
2013-04-12 4:51 ` Wenchao Xia
2013-04-12 11:51 ` Markus Armbruster
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 14/17] hmp: add function hmp_info_snapshots() Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 15/17] hmp: switch snapshot info function to qmp based one Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 16/17] hmp: show ImageInfo in 'info block' Wenchao Xia
2013-04-02 11:47 ` [Qemu-devel] [PATCH V11 17/17] hmp: add parameters device and -v for info block Wenchao Xia
2013-04-07 5:54 ` [Qemu-devel] [PATCH V11 00/17] qmp/hmp interfaces for internal snapshot info Wenchao Xia
2013-04-08 10:18 ` Kevin Wolf
2013-04-08 13:43 ` Stefan Hajnoczi
2013-04-11 12:17 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).