* [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
@ 2013-03-11 11:23 Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
` (14 more replies)
0 siblings, 15 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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
following interfaces for qemu:
1) qmp: query-images, show image info for a block device
Example:
-> { "execute": "query-images" }
<- {
"return":[
{
"device":"ide0-hd0",
"image":{
"filename":"disks/test1.img",
"format":"qcow2",
"virtual-size":2048000,
"snapshots":[
{
"id": "1",
"name": "snapshot1",
"vm-state-size": 0,
"date-sec": 10000200,
"date-nsec": 12,
"vm-clock-sec": 206,
"vm-clock-nsec": 30
}
]
}
}
]
}
2) qmp: query-snapshots, show avaiable vm snapshot info which can be used
in loadvm.
Example:
-> { "execute": "query-snapshots" }
<- {
"return":[
{
"id": "1",
"name": "snapshot1",
"vm-state-size": 0,
"date-sec": 10000200,
"date-nsec": 12,
"vm-clock-sec": 206,
"vm-clock-nsec": 30
}
]
}
3) hmp: info images, show what got in qmp query-images in monitor
These patches follows the rule that use qmp to retieve information,
hmp layer just do a translation from qmp object it got.
To make code graceful, snapshot and image info retrieving code in
qemu and qemu-img are merged into block layer, and some function name was
adjusted to make it tips better. For the part touch by the serial, it works as:
qemu qemu-img
dump_monitor dump_stdout
|--------------|
|
block/qapi.c
v2:
Rename and adjusted qmp interface according to comments from Eric.
Spelling fix.
Information retrieving function in block layer goes to seperated patch.
Free qmp object after usage in hmp.
Added counterpart in qmp-commands.hx.
Better tips in qmp-schema.json.
v3:
Spelling fix in commit message, patch 03/11.
Spelling fix in code, patch 06/11.
Add comments that vm-state-size is in bytes, and change size of it in
example to a reasonable number, patch 08/11.
v4:
02/13: in bdrv_get_filename(), add const to parameter *bs.
03/13: new added, in which the function correct the behavior in info
retrieving.
04/13: in bdrv_query_snapshot_infolist(), remove NULL check before call
err_setg(), added TODO comments that let block layer function set error instead
of this layer to tip better for errors, Split out patch about image info to
patch 05/13.
05/13: new splitted, and it checks *bs by calling bdrv_can_read_snapshot()
before collect internal snasphot info to avoid *err is set unexpectly now.
06/13: check if error happens after calling bdrv_query_image_info().
08/13: rename info to image in DeviceImageInfo and make it optional,
when device is not inserted it will be empty, added error handling code
when met error in calling block layer API.
09/13: distinguish *id and *name in bdrv_find_snapshots(), caller
can choose what to search with. id_wellformed() should be called in
new snapshot creation interface above this function in the future.
10/13: now this interface have addtional parameter *device, which
enable showing internal snapshots on a single device. Also use
bdrv_can_read_snapshot() instead of bdrv_can_snapshot() now.
11/13: this function goes to hmp.c so hmp_handler_error is not exported
any more, split out patch that switch snapshot info function to patch 12/13.
12/13: new splitted.
13/13: use qmp API instead of directly calling block layer API, now
all hmp function have correspond qmp funtion in this serial.
v5:
04/13: spelling fix in commit message, better comments and better
tips of error, after calling bdrv_snapshot_list().
v6:
==Address Kevin's comments==:
04/14: seperate patch for code moving,
05/14: use strerror() for error tipping after call of bdrv_snapshot_list(),
use "switch" instead of "if else" for error checking, replace
info_list->value->* by info->*.
06/14: access bs->backing_file directly instead of calling of
bdrv_get_backing_filename in collect_image_info. Function switch patch
of last version is merged into this patch.
08/14: API change, add optional parameter of "device" and "backing",
which allow showing all image info in the backing file chain of block
device, add related implemention.
09/14: seperate patch for code moving.
10/14: seperate patch for function change, return what got from
bdrv_snapshot_list() instead of -ENOENT.
==Address Eric's comments==:
02/14: return bool instead of int for this new function.
03/14: return bool instead of int for old function bdrv_can_snapshot().
V7:
==Address Eric's comments==:
6/14: discard of static function bdrv_get_filename().
8/14: better doc and comments, trailing '.' is removed.
11/14: QMP API change: removed parameter *device, it only returns VM snapshot
now. Block device's internal snapshot info can be accessed by query-images.
12/14: related caller code change for query-snapshots. Seperate internal
static function for VM snapshot info.
14/14: HMP API change: added support of query internal snapshots of one image
in the backing file chain of a block device. use query-images instead of
query-snapshots. Seperate internal static mirror function for block snapshot
info.
v8:
General change:
discard bdrv_can_read_snapshot() for that return value can tip error. add
block/snapshot.c and block/qapi.c and function are moved there. Patches 17-20
are for clean and moved to tail.
Address Markus's comments:
3/20: function was moved to block/snapshot.c instead of block.c.
7/20: better commit message. Remove comments for the return value of
bdrv_snapshot_list(), remove period suffix in error message, better error
message, remove the filter callback of it. remove comments for *bs must be
open. It return negative value for error check.
8/20: add a flag parameter to filter out inconsistent vm snapshots instead
of a call back function.
9/20: function reorganized to return negative error value, and qmp object
will be got only on success.
10/20: better documents and remove suffix period.
11/20: better documents and remove suffix period, explain optional member
in qmp-commands.hx.
Address Stefan's comments:
6/20: add prefix 'bdrv_' to the moved functions.
7/20: remove the filter call back function. remove bdrv_can_reand_snapshot()
call in qemu-img. It return negative value for error check.
8/20: add a flag parameter to filter out inconsistent vm snapshots instead
of a call back function.
9/20: small trivial static function was removed.
Address Eric's comments:
6/20: add prefix 'bdrv_' to the moved functions. Added message in commit
about the patch's purpose.
Other:
14/20 - 16/20: new interface info image which show all image info
including internal snapshots for each device, share the dump code
with qemu-img.
18/20: new clean up patch for block/snapshot.c.
19/20: new clean up patch for block/qapi.c.
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.
Wenchao Xia (14):
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 qmp: add interface query-snapshots
9 qmp: add interface query-images
10 hmp: add function hmp_info_snapshots()
11 hmp: switch snapshot info function to qmp based one
12 block: move dump_human_image_info() to block/qapi.c
13 block: dump to buffer for bdrv_image_info_dump()
14 hmp: add command info images
block/Makefile.objs | 1 +
block/qapi.c | 382 ++++++++++++++++++++++++++++++++++++++++++++++
block/snapshot.c | 67 ++++++++
hmp.c | 80 ++++++++++
hmp.h | 2 +
include/block/qapi.h | 28 ++++
include/block/snapshot.h | 26 +++
monitor.c | 9 +-
qapi-schema.json | 47 ++++++
qemu-img.c | 162 +------------------
qmp-commands.hx | 145 ++++++++++++++++++
savevm.c | 95 +-----------
12 files changed, 800 insertions(+), 244 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] 47+ messages in thread
* [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-11 17:49 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 02/14] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
` (13 subsequent siblings)
14 siblings, 1 reply; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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>
---
block/Makefile.objs | 1 +
block/snapshot.c | 37 +++++++++++++++++++++++++++++++++++++
include/block/snapshot.h | 26 ++++++++++++++++++++++++++
savevm.c | 23 +----------------------
4 files changed, 65 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..8de73b4
--- /dev/null
+++ b/block/snapshot.c
@@ -0,0 +1,37 @@
+/*
+ * Snapshot related functions.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "block/snapshot.h"
+
+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..86891d1
--- /dev/null
+++ b/include/block/snapshot.h
@@ -0,0 +1,26 @@
+/*
+ * Snapshot related functions.
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef SNAPSHOT_H
+#define SNAPSHOT_H
+
+#include "qemu-common.h"
+/*
+ * block.h is needed for QEMUSnapshotInfo, it can be removed when define is
+ * moved here.
+ */
+#include "block.h"
+
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+ const char *name);
+#endif
diff --git a/savevm.c b/savevm.c
index a8a53ef..95f19ca 100644
--- a/savevm.c
+++ b/savevm.c
@@ -39,6 +39,7 @@
#include "qmp-commands.h"
#include "trace.h"
#include "qemu/bitops.h"
+#include "block/snapshot.h"
#define SELF_ANNOUNCE_ROUNDS 5
@@ -2060,28 +2061,6 @@ out:
return ret;
}
-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
- const char *name)
-{
- QEMUSnapshotInfo *sn_tab, *sn;
- int nb_sns, i, ret;
-
- ret = -ENOENT;
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
- if (nb_sns < 0)
- return ret;
- for(i = 0; i < nb_sns; i++) {
- sn = &sn_tab[i];
- if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
- *sn_info = *sn;
- ret = 0;
- break;
- }
- }
- g_free(sn_tab);
- return ret;
-}
-
/*
* Deletes snapshots of a given name in all opened images.
*/
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 02/14] block: distinguish id and name in bdrv_find_snapshot()
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 03/14] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
` (12 subsequent siblings)
14 siblings, 0 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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>
---
block/snapshot.c | 46 ++++++++++++++++++++++++++++++++++++++--------
include/block/snapshot.h | 2 +-
savevm.c | 10 +++++-----
3 files changed, 44 insertions(+), 14 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index 8de73b4..1449b3d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -13,8 +13,20 @@
#include "block/snapshot.h"
+/*
+ * Try find an internal snapshot with @id or @name, @id have higher priority
+ * in searching.
+ *
+ * @bs: block device to search on, must not be NULL.
+ * @sn_info: snapshot information to be filled in, must not be NULL.
+ * @id: snapshot id to search with, can be NULL.
+ * @name: snapshot name to search with, can be NULL.
+ *
+ * returns 0 and @sn_info is filled with related information if found,
+ * otherwise it returns negative value.
+ */
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
- const char *name)
+ const char *id, const char *name)
{
QEMUSnapshotInfo *sn_tab, *sn;
int nb_sns, i, ret;
@@ -22,16 +34,34 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
ret = -ENOENT;
nb_sns = bdrv_snapshot_list(bs, &sn_tab);
if (nb_sns < 0) {
- return ret;
+ return nb_sns;
}
- for (i = 0; i < nb_sns; i++) {
- sn = &sn_tab[i];
- if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
- *sn_info = *sn;
- ret = 0;
- break;
+
+ /* search by id */
+ if (id) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->id_str, id)) {
+ *sn_info = *sn;
+ ret = 0;
+ goto out;
+ }
}
}
+
+ /* search by name */
+ if (name) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ ret = 0;
+ goto out;
+ }
+ }
+ }
+
+ out:
g_free(sn_tab);
return ret;
}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 86891d1..fc9be7b 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -22,5 +22,5 @@
#include "block.h"
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
- const char *name);
+ const char *id, const char *name);
#endif
diff --git a/savevm.c b/savevm.c
index 95f19ca..6557750 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2073,7 +2073,7 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
bs = NULL;
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs) &&
- bdrv_snapshot_find(bs, snapshot, name) >= 0)
+ bdrv_snapshot_find(bs, snapshot, name, name) >= 0)
{
ret = bdrv_snapshot_delete(bs, name);
if (ret < 0) {
@@ -2133,7 +2133,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
if (name) {
- ret = bdrv_snapshot_find(bs, old_sn, name);
+ ret = bdrv_snapshot_find(bs, old_sn, name, name);
if (ret >= 0) {
pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
@@ -2224,7 +2224,7 @@ int load_vmstate(const char *name)
}
/* Don't even try to load empty VM states */
- ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
+ ret = bdrv_snapshot_find(bs_vm_state, &sn, name, name);
if (ret < 0) {
return ret;
} else if (sn.vm_state_size == 0) {
@@ -2248,7 +2248,7 @@ int load_vmstate(const char *name)
return -ENOTSUP;
}
- ret = bdrv_snapshot_find(bs, &sn, name);
+ ret = bdrv_snapshot_find(bs, &sn, name, name);
if (ret < 0) {
error_report("Device '%s' does not have the requested snapshot '%s'",
bdrv_get_device_name(bs), name);
@@ -2354,7 +2354,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
while ((bs1 = bdrv_next(bs1))) {
if (bdrv_can_snapshot(bs1) && bs1 != bs) {
- ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
+ ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
if (ret < 0) {
available = 0;
break;
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 03/14] qemu-img: remove unused parameter in collect_image_info()
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 02/14] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 04/14] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
` (11 subsequent siblings)
14 siblings, 0 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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>
---
qemu-img.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index 471de7d..f4e5d90 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1640,8 +1640,7 @@ static void dump_json_image_info(ImageInfo *info)
static void collect_image_info(BlockDriverState *bs,
ImageInfo *info,
- const char *filename,
- const char *fmt)
+ const char *filename)
{
uint64_t total_sectors;
char backing_filename[1024];
@@ -1815,7 +1814,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
}
info = g_new0(ImageInfo, 1);
- collect_image_info(bs, info, filename, fmt);
+ collect_image_info(bs, info, filename);
collect_snapshots(bs, info);
elem = g_new0(ImageInfoList, 1);
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 04/14] block: move collect_snapshots() and collect_image_info() to block/qapi.c
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (2 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 03/14] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-12 19:41 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 05/14] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
` (10 subsequent siblings)
14 siblings, 1 reply; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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>
---
block/Makefile.objs | 2 +-
block/qapi.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/block/qapi.h | 24 ++++++++++++
qemu-img.c | 86 ++-------------------------------------------
4 files changed, 124 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..51692d3
--- /dev/null
+++ b/block/qapi.c
@@ -0,0 +1,96 @@
+/*
+ * Block layer qmp related functions
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "block/qapi.h"
+#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..ecb5242
--- /dev/null
+++ b/include/block/qapi.h
@@ -0,0 +1,24 @@
+/*
+ * Block layer qmp related functions
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * Authors:
+ * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#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 f4e5d90..2e1698f 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] 47+ messages in thread
* [Qemu-devel] [PATCH V9 05/14] block: add snapshot info query function bdrv_query_snapshot_info_list()
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (3 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 04/14] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 06/14] block: add check for VM snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
` (9 subsequent siblings)
14 siblings, 0 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qapi.c | 52 +++++++++++++++++++++++++++++++++++++------------
include/block/qapi.h | 4 ++-
qemu-img.c | 4 ++-
3 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 51692d3..2da28db 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -14,29 +14,53 @@
#include "block/qapi.h"
#include "block/block_int.h"
-void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info)
+/* return 0 on success, and @p_list will be set only on success. */
+int bdrv_query_snapshot_info_list(BlockDriverState *bs,
+ SnapshotInfoList **p_list,
+ Error **errp)
{
int i, sn_count;
QEMUSnapshotInfo *sn_tab = NULL;
- SnapshotInfoList *info_list, *cur_item = NULL;
+ SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
+ SnapshotInfo *info;
+
sn_count = bdrv_snapshot_list(bs, &sn_tab);
+ if (sn_count < 0) {
+ const char *dev = bdrv_get_device_name(bs);
+ switch (sn_count) {
+ case -ENOMEDIUM:
+ error_setg(errp, "Device '%s' is not inserted", dev);
+ break;
+ case -ENOTSUP:
+ error_setg(errp,
+ "Device '%s' does not support internal snapshots",
+ dev);
+ break;
+ default:
+ error_setg(errp, "Can't list snapshots of device '%s': %s",
+ dev, strerror(-sn_count));
+ break;
+ }
+ return sn_count;
+ }
for (i = 0; i < sn_count; i++) {
- info->has_snapshots = true;
- info_list = g_new0(SnapshotInfoList, 1);
- info_list->value = g_new0(SnapshotInfo, 1);
- info_list->value->id = g_strdup(sn_tab[i].id_str);
- info_list->value->name = g_strdup(sn_tab[i].name);
- info_list->value->vm_state_size = sn_tab[i].vm_state_size;
- info_list->value->date_sec = sn_tab[i].date_sec;
- info_list->value->date_nsec = sn_tab[i].date_nsec;
- info_list->value->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000;
- info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000;
+ info = g_new0(SnapshotInfo, 1);
+ info->id = g_strdup(sn_tab[i].id_str);
+ info->name = g_strdup(sn_tab[i].name);
+ info->vm_state_size = sn_tab[i].vm_state_size;
+ info->date_sec = sn_tab[i].date_sec;
+ info->date_nsec = sn_tab[i].date_nsec;
+ info->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000;
+ info->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000;
+
+ info_list = g_new0(SnapshotInfoList, 1);
+ info_list->value = info;
/* XXX: waiting for the qapi to support qemu-queue.h types */
if (!cur_item) {
- info->snapshots = cur_item = info_list;
+ head = cur_item = info_list;
} else {
cur_item->next = info_list;
cur_item = info_list;
@@ -45,6 +69,8 @@ void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info)
}
g_free(sn_tab);
+ *p_list = head;
+ return 0;
}
void bdrv_collect_image_info(BlockDriverState *bs,
diff --git a/include/block/qapi.h b/include/block/qapi.h
index ecb5242..4b46165 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -17,7 +17,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 2e1698f..cb534c9 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1735,7 +1735,9 @@ static ImageInfoList *collect_image_info_list(const char *filename,
info = g_new0(ImageInfo, 1);
bdrv_collect_image_info(bs, info, filename);
- bdrv_collect_snapshots(bs, info);
+ if (!bdrv_query_snapshot_info_list(bs, &info->snapshots, NULL)) {
+ info->has_snapshots = true;
+ }
elem = g_new0(ImageInfoList, 1);
elem->value = info;
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 06/14] block: add check for VM snapshot in bdrv_query_snapshot_info_list()
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (4 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 05/14] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-12 19:45 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 07/14] block: add image info query function bdrv_query_image_info() Wenchao Xia
` (8 subsequent siblings)
14 siblings, 1 reply; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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>
---
block/qapi.c | 42 ++++++++++++++++++++++++++++++++++++++++--
include/block/qapi.h | 1 +
qemu-img.c | 3 ++-
3 files changed, 43 insertions(+), 3 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 2da28db..1d99f5a 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -12,11 +12,47 @@
*/
#include "block/qapi.h"
+#include "block/snapshot.h"
#include "block/block_int.h"
-/* return 0 on success, and @p_list will be set only on success. */
+/*
+ * check whether the snapshot is valid for whole vm.
+ *
+ * @sn: snapshot info 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 (bdrv_can_snapshot(bs1) && bs1 != bs) {
+ ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
+ if (ret < 0) {
+ return false;
+ }
+ }
+ }
+ return true;
+}
+
+/*
+ * return 0 on success, and @p_list will be set only 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;
@@ -45,7 +81,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 4b46165..092ad8c 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -19,6 +19,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 cb534c9..83532b1 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->has_snapshots = true;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 07/14] block: add image info query function bdrv_query_image_info()
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (5 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 06/14] block: add check for VM snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 08/14] qmp: add interface query-snapshots Wenchao Xia
` (7 subsequent siblings)
14 siblings, 0 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block/qapi.c | 39 ++++++++++++++++++++++++++++++++-------
include/block/qapi.h | 6 +++---
qemu-img.c | 7 ++-----
3 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 1d99f5a..b78ea5c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -111,18 +111,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);
@@ -139,8 +143,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,
@@ -157,4 +161,25 @@ void bdrv_collect_image_info(BlockDriverState *bs,
info->has_backing_filename_format = true;
}
}
+
+ ret = bdrv_query_snapshot_info_list(bs, &info->snapshots, false, &err);
+ switch (ret) {
+ case 0:
+ info->has_snapshots = true;
+ break;
+ /* recoverable error */
+ case -ENOMEDIUM:
+ error_free(err);
+ break;
+ case -ENOTSUP:
+ error_free(err);
+ break;
+ default:
+ error_propagate(errp, err);
+ qapi_free_ImageInfo(info);
+ return ret;
+ }
+
+ *p_info = info;
+ return 0;
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 092ad8c..7ff1f98 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -21,7 +21,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 83532b1..59d900a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1733,11 +1733,8 @@ static ImageInfoList *collect_image_info_list(const char *filename,
goto err;
}
- info = g_new0(ImageInfo, 1);
- bdrv_collect_image_info(bs, info, filename);
- if (!bdrv_query_snapshot_info_list(bs, &info->snapshots,
- false, NULL)) {
- info->has_snapshots = true;
+ if (bdrv_query_image_info(bs, &info, NULL)) {
+ goto err;
}
elem = g_new0(ImageInfoList, 1);
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 08/14] qmp: add interface query-snapshots
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (6 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 07/14] block: add image info query function bdrv_query_image_info() Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-12 21:12 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 09/14] qmp: add interface query-images Wenchao Xia
` (6 subsequent siblings)
14 siblings, 1 reply; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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>
---
block/qapi.c | 18 +++++++++++++++++
qapi-schema.json | 14 +++++++++++++
qmp-commands.hx | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 87 insertions(+), 0 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index b78ea5c..7bf2581 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -14,6 +14,7 @@
#include "block/qapi.h"
#include "block/snapshot.h"
#include "block/block_int.h"
+#include "qmp-commands.h"
/*
* check whether the snapshot is valid for whole vm.
@@ -183,3 +184,20 @@ int bdrv_query_image_info(BlockDriverState *bs,
*p_info = info;
return 0;
}
+
+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;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 28b070f..014365b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -839,6 +839,20 @@
{ 'command': 'query-block', 'returns': ['BlockInfo'] }
##
+# @query-snapshots:
+#
+# Get a list of internal snapshots for 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 95022e2..bd9e127 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1743,6 +1743,61 @@ EQMP
},
SQMP
+query-snapshots
+---------------
+
+Show the internal consistent snapshot information
+
+Each snapshot is represented by a json-object. The returned value
+is a json-array of all snapshots
+
+Each json-object contain the following:
+
+- "id": unique snapshot id (json-string)
+- "name": internal snapshot name (json-string)
+- "vm-state-size": size of the VM state in bytes (json-int)
+- "date-sec": UTC date of the snapshot in seconds (json-int)
+- "date-nsec": fractional part in 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] 47+ messages in thread
* [Qemu-devel] [PATCH V9 09/14] qmp: add interface query-images
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (7 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 08/14] qmp: add interface query-snapshots Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-12 21:24 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 10/14] hmp: add function hmp_info_snapshots() Wenchao Xia
` (5 subsequent siblings)
14 siblings, 1 reply; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This mirror function will return image info including snapshots,
and if specified backing image's info will also be returned. Now
Qemu have both query-images and query-block interfaces.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/qapi.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
qapi-schema.json | 33 ++++++++++++++++++++
qmp-commands.hx | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 206 insertions(+), 0 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 7bf2581..0cbbb8c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -201,3 +201,86 @@ SnapshotInfoList *qmp_query_snapshots(Error **errp)
return list;
}
+
+/* Collect all info for one bs, p_list point to the empty tail to be filled,
+ return the empty tail of the new list. @bs must be the top one. */
+static DeviceImageInfoList **
+collect_device_image_info_list(BlockDriverState *bs,
+ bool show_backing,
+ DeviceImageInfoList **p_list,
+ Error **errp)
+{
+ DeviceImageInfoList **p_next = p_list;
+ char *device_name = bs->device_name;
+
+ while (bs) {
+ DeviceImageInfo *dii = g_malloc0(sizeof(*dii));
+ DeviceImageInfoList *diil = g_malloc0(sizeof(*diil));
+ diil->value = dii;
+ *p_next = diil;
+ p_next = &diil->next;
+
+ dii->device = g_strdup(device_name);
+ if (!bdrv_is_inserted(bs)) {
+ dii->has_image = false;
+ break;
+ }
+ dii->has_image = true;
+ if (bdrv_query_image_info(bs, &dii->image, errp)) {
+ break;
+ }
+
+ if (show_backing && bs->drv && bs->backing_hd) {
+ bs = bs->backing_hd;
+ } else {
+ bs = NULL;
+ }
+ }
+ return p_next;
+}
+
+DeviceImageInfoList *qmp_query_images(bool has_device,
+ const char *device,
+ bool has_backing,
+ bool backing,
+ Error **errp)
+{
+ DeviceImageInfoList *head = NULL, **p_next = &head;
+ BlockDriverState *bs = NULL;
+ Error *err = NULL;
+ const char *target_device = NULL;
+ bool show_backing = false;
+
+ if (has_device) {
+ target_device = device;
+ }
+ if (has_backing) {
+ show_backing = backing;
+ }
+
+ if (target_device) {
+ bs = bdrv_find(device);
+ if (!bs) {
+ error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+ return NULL;
+ }
+ collect_device_image_info_list(bs, show_backing, p_next, errp);
+ if (error_is_set(&err)) {
+ goto err;
+ }
+ } else {
+ while ((bs = bdrv_next(bs))) {
+ p_next = collect_device_image_info_list(bs, show_backing,
+ p_next, errp);
+ if (error_is_set(&err)) {
+ goto err;
+ }
+ }
+ }
+
+ return head;
+
+ err:
+ qapi_free_DeviceImageInfoList(head);
+ return NULL;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 014365b..642a10a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -294,6 +294,21 @@
'*total-clusters': 'int', '*allocated-clusters': 'int',
'*fragmented-clusters': 'int', '*compressed-clusters': 'int' } }
+# @DeviceImageInfo:
+#
+# Information about an image used by a QEMU block device
+#
+# @device: name of the block device
+#
+# @image: #optional info of the image used
+#
+# Since: 1.5
+#
+##
+
+{ 'type': 'DeviceImageInfo',
+ 'data': {'device': 'str', '*image': 'ImageInfo' } }
+
##
# @StatusInfo:
#
@@ -853,6 +868,24 @@
'returns': ['SnapshotInfo'] }
##
+# @query-images:
+#
+# Get block device image information
+#
+# @device: #optional the name of the device to get image info from. If not
+# specified, all block devices will be queried
+# @backing: #optional true to show information on backing images, false or
+# omitted to show just the top image of a block device
+#
+# Returns: a list of @DeviceImageInfo describing each virtual block device
+#
+# Since: 1.5
+##
+{ 'command': 'query-images',
+ 'data': { '*device': 'str', '*backing': 'bool' },
+ 'returns': ['DeviceImageInfo'] }
+
+##
# @BlockDeviceStats:
#
# Statistics of a virtual block device or a block backing device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index bd9e127..0c4c08c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1798,6 +1798,96 @@ EQMP
},
SQMP
+query-images
+------------
+
+Show block device image information
+
+Each device is represented by a json-object. The returned value is a json-array
+of all devices' images
+
+Each json-object contain the following:
+
+- "device": device name (json-string)
+- "image": image information(json-object, optional) containing:
+ - "filename": image file name (json-string)
+ - "format": image format (json-string)
+ - "virtual-size": image capacity in bytes (json-int)
+ - "dirty-flag": true if image is not cleanly closed, not present means
+ clean (json-bool, optional)
+ - "actual-size": actual size on disk in bytes of the image, not
+ present when image does not support thin provision
+ (json-int, optional)
+ - "cluster-size": size of a cluster in bytes, 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)
+
+Example:
+
+-> { "execute": "query-images" }
+<- {
+ "return":[
+ {
+ "device":"ide0-hd0",
+ "image":{
+ "filename":"disks/test0.img",
+ "format":"qcow2",
+ "virtual-size":1024000
+ }
+ },
+ {
+ "device":"ide0-hd1",
+ "image":{
+ "filename":"disks/test1.img",
+ "format":"qcow2",
+ "virtual-size":2048000,
+ "snapshots":[
+ {
+ "id": "1",
+ "name": "snapshot1",
+ "vm-state-size": 0,
+ "date-sec": 10000200,
+ "date-nsec": 12,
+ "vm-clock-sec": 206,
+ "vm-clock-nsec": 30
+ }
+ ]
+ }
+ }
+ ]
+ }
+
+EQMP
+
+ {
+ .name = "query-images",
+ .args_type = "device:B?, backing:-b",
+ .mhandler.cmd_new = qmp_marshal_input_query_images,
+ },
+
+SQMP
query-blockstats
----------------
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 10/14] hmp: add function hmp_info_snapshots()
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (8 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 09/14] qmp: add interface query-images Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 11/14] hmp: switch snapshot info function to qmp based one Wenchao Xia
` (4 subsequent siblings)
14 siblings, 0 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
hmp.h | 1 +
2 files changed, 43 insertions(+), 0 deletions(-)
diff --git a/hmp.c b/hmp.c
index 2f47a8a..34f0691 100644
--- a/hmp.c
+++ b/hmp.c
@@ -607,6 +607,48 @@ void hmp_info_block_jobs(Monitor *mon, const QDict *qdict)
}
}
+/* assume list is valid */
+static void monitor_dump_snapshotinfolist(Monitor *mon, SnapshotInfoList *list)
+{
+ SnapshotInfoList *elem;
+ char buf[256];
+
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+
+ for (elem = list; elem; elem = elem->next) {
+ QEMUSnapshotInfo sn = {
+ .vm_state_size = elem->value->vm_state_size,
+ .date_sec = elem->value->date_sec,
+ .date_nsec = elem->value->date_nsec,
+ .vm_clock_nsec = elem->value->vm_clock_sec * 1000000000ULL +
+ elem->value->vm_clock_nsec,
+ };
+ pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
+ pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+ }
+}
+
+void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
+{
+ Error *err = NULL;
+ SnapshotInfoList *list;
+
+ list = qmp_query_snapshots(&err);
+ if (error_is_set(&err)) {
+ hmp_handle_error(mon, &err);
+ return;
+ }
+
+ if (list == NULL) {
+ monitor_printf(mon, "There is no suitable snapshot available\n");
+ return;
+ }
+
+ monitor_dump_snapshotinfolist(mon, list);
+ qapi_free_SnapshotInfoList(list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 30b3c20..0b0ae13 100644
--- a/hmp.h
+++ b/hmp.h
@@ -36,6 +36,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict);
void hmp_info_balloon(Monitor *mon, const QDict *qdict);
void hmp_info_pci(Monitor *mon, const QDict *qdict);
void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
+void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 11/14] hmp: switch snapshot info function to qmp based one
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (9 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 10/14] hmp: add function hmp_info_snapshots() Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 12/14] block: move dump_human_image_info() to block/qapi.c Wenchao Xia
` (3 subsequent siblings)
14 siblings, 0 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 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>
---
monitor.c | 2 +-
savevm.c | 64 -------------------------------------------------------------
2 files changed, 1 insertions(+), 65 deletions(-)
diff --git a/monitor.c b/monitor.c
index 32a6e74..5112375 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2594,7 +2594,7 @@ static mon_cmd_t info_cmds[] = {
.args_type = "",
.params = "",
.help = "show the currently saved VM snapshots",
- .mhandler.cmd = do_info_snapshots,
+ .mhandler.cmd = hmp_info_snapshots,
},
{
.name = "status",
diff --git a/savevm.c b/savevm.c
index 6557750..71e78dc 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2319,70 +2319,6 @@ void do_delvm(Monitor *mon, const QDict *qdict)
}
}
-void do_info_snapshots(Monitor *mon, const QDict *qdict)
-{
- BlockDriverState *bs, *bs1;
- QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
- int nb_sns, i, ret, available;
- int total;
- int *available_snapshots;
- char buf[256];
-
- bs = bdrv_snapshots();
- if (!bs) {
- monitor_printf(mon, "No available block device supports snapshots\n");
- return;
- }
-
- nb_sns = bdrv_snapshot_list(bs, &sn_tab);
- if (nb_sns < 0) {
- monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
- return;
- }
-
- if (nb_sns == 0) {
- monitor_printf(mon, "There is no snapshot available.\n");
- return;
- }
-
- available_snapshots = g_malloc0(sizeof(int) * nb_sns);
- total = 0;
- for (i = 0; i < nb_sns; i++) {
- sn = &sn_tab[i];
- available = 1;
- bs1 = NULL;
-
- while ((bs1 = bdrv_next(bs1))) {
- if (bdrv_can_snapshot(bs1) && bs1 != bs) {
- ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
- if (ret < 0) {
- available = 0;
- break;
- }
- }
- }
-
- if (available) {
- available_snapshots[total] = i;
- total++;
- }
- }
-
- if (total > 0) {
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
- for (i = 0; i < total; i++) {
- sn = &sn_tab[available_snapshots[i]];
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
- }
- } else {
- monitor_printf(mon, "There is no suitable snapshot available\n");
- }
-
- g_free(sn_tab);
- g_free(available_snapshots);
-
-}
-
void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
{
qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK,
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 12/14] block: move dump_human_image_info() to block/qapi.c
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (10 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 11/14] hmp: switch snapshot info function to qmp based one Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-13 20:38 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 13/14] block: dump to buffer for bdrv_image_info_dump() Wenchao Xia
` (2 subsequent siblings)
14 siblings, 1 reply; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This function is needed later in hmp command, it is also renamed to
bdrv_image_info_dump().
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/qapi.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++
include/block/qapi.h | 1 +
qemu-img.c | 69 +-------------------------------------------------
3 files changed, 69 insertions(+), 68 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 0cbbb8c..1eb3de1 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -284,3 +284,70 @@ DeviceImageInfoList *qmp_query_images(bool has_device,
qapi_free_DeviceImageInfoList(head);
return NULL;
}
+
+void bdrv_image_info_dump(ImageInfo *info)
+{
+ char size_buf[128], dsize_buf[128];
+ if (!info->has_actual_size) {
+ snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
+ } else {
+ get_human_readable_size(dsize_buf, sizeof(dsize_buf),
+ info->actual_size);
+ }
+ get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
+ printf("image: %s\n"
+ "file format: %s\n"
+ "virtual size: %s (%" PRId64 " bytes)\n"
+ "disk size: %s\n",
+ info->filename, info->format, size_buf,
+ info->virtual_size,
+ dsize_buf);
+
+ if (info->has_encrypted && info->encrypted) {
+ printf("encrypted: yes\n");
+ }
+
+ if (info->has_cluster_size) {
+ printf("cluster_size: %" PRId64 "\n", info->cluster_size);
+ }
+
+ if (info->has_dirty_flag && info->dirty_flag) {
+ printf("cleanly shut down: no\n");
+ }
+
+ if (info->has_backing_filename) {
+ printf("backing file: %s", info->backing_filename);
+ if (info->has_full_backing_filename) {
+ printf(" (actual path: %s)", info->full_backing_filename);
+ }
+ putchar('\n');
+ if (info->has_backing_filename_format) {
+ printf("backing file format: %s\n", info->backing_filename_format);
+ }
+ }
+
+ if (info->has_snapshots) {
+ SnapshotInfoList *elem;
+ char buf[256];
+
+ printf("Snapshot list:\n");
+ printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+
+ /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
+ * we convert to the block layer's native QEMUSnapshotInfo for now.
+ */
+ for (elem = info->snapshots; elem; elem = elem->next) {
+ QEMUSnapshotInfo sn = {
+ .vm_state_size = elem->value->vm_state_size,
+ .date_sec = elem->value->date_sec,
+ .date_nsec = elem->value->date_nsec,
+ .vm_clock_nsec = elem->value->vm_clock_sec * 1000000000ULL +
+ elem->value->vm_clock_nsec,
+ };
+
+ pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
+ pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
+ printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+ }
+ }
+}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 7ff1f98..cc55bda 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -24,4 +24,5 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
int bdrv_query_image_info(BlockDriverState *bs,
ImageInfo **p_info,
Error **errp);
+void bdrv_image_info_dump(ImageInfo *info);
#endif
diff --git a/qemu-img.c b/qemu-img.c
index 59d900a..e1d6bac 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1606,73 +1606,6 @@ static void dump_json_image_info(ImageInfo *info)
QDECREF(str);
}
-static void dump_human_image_info(ImageInfo *info)
-{
- char size_buf[128], dsize_buf[128];
- if (!info->has_actual_size) {
- snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
- } else {
- get_human_readable_size(dsize_buf, sizeof(dsize_buf),
- info->actual_size);
- }
- get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
- printf("image: %s\n"
- "file format: %s\n"
- "virtual size: %s (%" PRId64 " bytes)\n"
- "disk size: %s\n",
- info->filename, info->format, size_buf,
- info->virtual_size,
- dsize_buf);
-
- if (info->has_encrypted && info->encrypted) {
- printf("encrypted: yes\n");
- }
-
- if (info->has_cluster_size) {
- printf("cluster_size: %" PRId64 "\n", info->cluster_size);
- }
-
- if (info->has_dirty_flag && info->dirty_flag) {
- printf("cleanly shut down: no\n");
- }
-
- if (info->has_backing_filename) {
- printf("backing file: %s", info->backing_filename);
- if (info->has_full_backing_filename) {
- printf(" (actual path: %s)", info->full_backing_filename);
- }
- putchar('\n');
- if (info->has_backing_filename_format) {
- printf("backing file format: %s\n", info->backing_filename_format);
- }
- }
-
- if (info->has_snapshots) {
- SnapshotInfoList *elem;
- char buf[256];
-
- printf("Snapshot list:\n");
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
-
- /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
- * we convert to the block layer's native QEMUSnapshotInfo for now.
- */
- for (elem = info->snapshots; elem; elem = elem->next) {
- QEMUSnapshotInfo sn = {
- .vm_state_size = elem->value->vm_state_size,
- .date_sec = elem->value->date_sec,
- .date_nsec = elem->value->date_nsec,
- .vm_clock_nsec = elem->value->vm_clock_sec * 1000000000ULL +
- elem->value->vm_clock_nsec,
- };
-
- pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
- pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
- }
- }
-}
-
static void dump_human_image_info_list(ImageInfoList *list)
{
ImageInfoList *elem;
@@ -1684,7 +1617,7 @@ static void dump_human_image_info_list(ImageInfoList *list)
}
delim = true;
- dump_human_image_info(elem->value);
+ bdrv_image_info_dump(elem->value);
}
}
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 13/14] block: dump to buffer for bdrv_image_info_dump()
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (11 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 12/14] block: move dump_human_image_info() to block/qapi.c Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 14/14] hmp: add command info images Wenchao Xia
2013-03-12 10:07 ` [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Stefan Hajnoczi
14 siblings, 0 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This allow hmp use this function, just like qemu-img.
It also returns a pointer now to make it easy to use.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/qapi.c | 67 +++++++++++++++++++++++++++++++++++--------------
include/block/qapi.h | 2 +-
qemu-img.c | 6 +++-
3 files changed, 54 insertions(+), 21 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 1eb3de1..e6a6ec6 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -285,9 +285,30 @@ DeviceImageInfoList *qmp_query_images(bool has_device,
return NULL;
}
-void bdrv_image_info_dump(ImageInfo *info)
+GCC_FMT_ATTR(3, 4)
+static void snprintf_tail(char **p_buf, int *p_size, const char *fmt, ...)
+{
+ int l;
+ if (*p_size < 1) {
+ return;
+ }
+
+ va_list ap;
+ va_start(ap, fmt);
+ l = vsnprintf(*p_buf, *p_size, fmt, ap);
+ va_end(ap);
+ if (l > 0) {
+ *p_buf += l;
+ *p_size -= l;
+ }
+}
+
+/* Use buf instead of asprintf to reduce memory fragility. */
+char *bdrv_image_info_dump(char *buf, int buf_size, ImageInfo *info)
{
char size_buf[128], dsize_buf[128];
+ char *buf1 = buf;
+
if (!info->has_actual_size) {
snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
} else {
@@ -295,43 +316,49 @@ void bdrv_image_info_dump(ImageInfo *info)
info->actual_size);
}
get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
- printf("image: %s\n"
- "file format: %s\n"
- "virtual size: %s (%" PRId64 " bytes)\n"
- "disk size: %s\n",
- info->filename, info->format, size_buf,
- info->virtual_size,
- dsize_buf);
+ snprintf_tail(&buf1, &buf_size,
+ "image: %s\n"
+ "file format: %s\n"
+ "virtual size: %s (%" PRId64 " bytes)\n"
+ "disk size: %s\n",
+ info->filename, info->format, size_buf,
+ info->virtual_size,
+ dsize_buf);
if (info->has_encrypted && info->encrypted) {
- printf("encrypted: yes\n");
+ snprintf_tail(&buf1, &buf_size, "encrypted: yes\n");
}
if (info->has_cluster_size) {
- printf("cluster_size: %" PRId64 "\n", info->cluster_size);
+ snprintf_tail(&buf1, &buf_size, "cluster_size: %" PRId64 "\n",
+ info->cluster_size);
}
if (info->has_dirty_flag && info->dirty_flag) {
- printf("cleanly shut down: no\n");
+ snprintf_tail(&buf1, &buf_size, "cleanly shut down: no\n");
}
if (info->has_backing_filename) {
- printf("backing file: %s", info->backing_filename);
+ snprintf_tail(&buf1, &buf_size, "backing file: %s",
+ info->backing_filename);
if (info->has_full_backing_filename) {
- printf(" (actual path: %s)", info->full_backing_filename);
+ snprintf_tail(&buf1, &buf_size, " (actual path: %s)",
+ info->full_backing_filename);
}
- putchar('\n');
+ snprintf_tail(&buf1, &buf_size, "\n");
if (info->has_backing_filename_format) {
- printf("backing file format: %s\n", info->backing_filename_format);
+ snprintf_tail(&buf1, &buf_size, "backing file format: %s\n",
+ info->backing_filename_format);
}
}
if (info->has_snapshots) {
SnapshotInfoList *elem;
- char buf[256];
+ char buf_sn[256];
- printf("Snapshot list:\n");
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ snprintf_tail(&buf1, &buf_size, "Snapshot list:\n");
+ snprintf_tail(&buf1, &buf_size, "%s\n",
+ bdrv_snapshot_dump(buf_sn, sizeof(buf_sn), NULL));
/* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
* we convert to the block layer's native QEMUSnapshotInfo for now.
@@ -347,7 +374,9 @@ void bdrv_image_info_dump(ImageInfo *info)
pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+ snprintf_tail(&buf1, &buf_size, "%s\n",
+ bdrv_snapshot_dump(buf_sn, sizeof(buf_sn), &sn));
}
}
+ return buf;
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index cc55bda..7962871 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -24,5 +24,5 @@ int bdrv_query_snapshot_info_list(BlockDriverState *bs,
int bdrv_query_image_info(BlockDriverState *bs,
ImageInfo **p_info,
Error **errp);
-void bdrv_image_info_dump(ImageInfo *info);
+char *bdrv_image_info_dump(char *buf, int buf_size, ImageInfo *info);
#endif
diff --git a/qemu-img.c b/qemu-img.c
index e1d6bac..6db2b38 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1606,10 +1606,12 @@ static void dump_json_image_info(ImageInfo *info)
QDECREF(str);
}
+#define IMAGE_INFO_BUF_SIZE (2048)
static void dump_human_image_info_list(ImageInfoList *list)
{
ImageInfoList *elem;
bool delim = false;
+ char *buf = g_malloc0(IMAGE_INFO_BUF_SIZE);
for (elem = list; elem; elem = elem->next) {
if (delim) {
@@ -1617,8 +1619,10 @@ static void dump_human_image_info_list(ImageInfoList *list)
}
delim = true;
- bdrv_image_info_dump(elem->value);
+ printf("%s",
+ bdrv_image_info_dump(buf, IMAGE_INFO_BUF_SIZE, elem->value));
}
+ g_free(buf);
}
static gboolean str_equal_func(gconstpointer a, gconstpointer b)
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* [Qemu-devel] [PATCH V9 14/14] hmp: add command info images
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (12 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 13/14] block: dump to buffer for bdrv_image_info_dump() Wenchao Xia
@ 2013-03-11 11:23 ` Wenchao Xia
2013-03-12 10:07 ` [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Stefan Hajnoczi
14 siblings, 0 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-11 11:23 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This command will show block image's information, including
internal snapshots.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
hmp.c | 38 ++++++++++++++++++++++++++++++++++++++
hmp.h | 1 +
monitor.c | 7 +++++++
3 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/hmp.c b/hmp.c
index 34f0691..05d9b3b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -22,6 +22,7 @@
#include "qemu/sockets.h"
#include "monitor/monitor.h"
#include "ui/console.h"
+#include "block/qapi.h"
static void hmp_handle_error(Monitor *mon, Error **errp)
{
@@ -649,6 +650,43 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
qapi_free_SnapshotInfoList(list);
}
+#define IMAGE_INFO_BUF_SIZE (2048)
+static void mon_dump_device_image_info_list(Monitor *mon,
+ DeviceImageInfoList *list)
+{
+ char *buf = g_malloc0(IMAGE_INFO_BUF_SIZE);
+ DeviceImageInfoList *p = list;
+ DeviceImageInfo *dii;
+ while (p) {
+ dii = p->value;
+ monitor_printf(mon, "Device : %s\n", dii->device);
+ if (dii->has_image) {
+ bdrv_image_info_dump(buf, IMAGE_INFO_BUF_SIZE, dii->image);
+ monitor_printf(mon, "%s", buf);
+ }
+ monitor_printf(mon, "\n");
+
+ p = p->next;
+ }
+ g_free(buf);
+}
+
+void hmp_info_images(Monitor *mon, const QDict *qdict)
+{
+ const char *device = qdict_get_try_str(qdict, "device");
+ int backing = qdict_get_try_bool(qdict, "backing", 0);
+ Error *err = NULL;
+ DeviceImageInfoList *list = NULL;
+
+ list = qmp_query_images(!!device, device, true, backing, &err);
+ if (!error_is_set(&err) && list) {
+ mon_dump_device_image_info_list(mon, list);
+ }
+
+ hmp_handle_error(mon, &err);
+ qapi_free_DeviceImageInfoList(list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 0b0ae13..9d9ada3 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
void hmp_info_pci(Monitor *mon, const QDict *qdict);
void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
+void hmp_info_images(Monitor *mon, const QDict *qdict);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index 5112375..b470963 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2597,6 +2597,13 @@ static mon_cmd_t info_cmds[] = {
.mhandler.cmd = hmp_info_snapshots,
},
{
+ .name = "images",
+ .args_type = "backing:-b,device:B?",
+ .params = "[-b] [device]",
+ .help = "show the image info",
+ .mhandler.cmd = hmp_info_images,
+ },
+ {
.name = "status",
.args_type = "",
.params = "",
--
1.7.1
^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
@ 2013-03-11 17:49 ` Eric Blake
2013-03-12 5:01 ` Wenchao Xia
2013-03-13 18:08 ` Markus Armbruster
0 siblings, 2 replies; 47+ messages in thread
From: Eric Blake @ 2013-03-11 17:49 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]
On 03/11/2013 05:23 AM, Wenchao Xia wrote:
> 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>
> ---
> block/Makefile.objs | 1 +
> block/snapshot.c | 37 +++++++++++++++++++++++++++++++++++++
> include/block/snapshot.h | 26 ++++++++++++++++++++++++++
> savevm.c | 23 +----------------------
> 4 files changed, 65 insertions(+), 22 deletions(-)
> create mode 100644 block/snapshot.c
> create mode 100644 include/block/snapshot.h
> +++ b/block/snapshot.c
> @@ -0,0 +1,37 @@
> +/*
> + * Snapshot related functions.
> + *
> + * Copyright IBM, Corp. 2013
Technically, since you are moving code from savevm.c, you should also
preserve the copyright on that moved code:
* Copyright (c) 2003-2008 Fabrice Bellard
> + *
> + * Authors:
> + * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
Furthermore, the code you are moving was under BSD license, so by moving
the code, you have changed its license to something more restrictive.
Personally, I like LGPL better than BSD, but as I'm not the copyright
holder of the original code, and neither are you, I'm not sure that
either of us is qualified to make that change. Therefore, I'm unwilling
to add my Reviewed-by, even though the code motion itself is correct,
without a maintainer chiming in on whether your licensing is appropriate
or needs an adjustment.
--
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] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-11 17:49 ` Eric Blake
@ 2013-03-12 5:01 ` Wenchao Xia
2013-03-12 16:15 ` Eric Blake
2013-03-13 12:41 ` Kevin Wolf
2013-03-13 18:08 ` Markus Armbruster
1 sibling, 2 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-12 5:01 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
于 2013-3-12 1:49, Eric Blake 写道:
> On 03/11/2013 05:23 AM, Wenchao Xia wrote:
>> 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>
>> ---
>> block/Makefile.objs | 1 +
>> block/snapshot.c | 37 +++++++++++++++++++++++++++++++++++++
>> include/block/snapshot.h | 26 ++++++++++++++++++++++++++
>> savevm.c | 23 +----------------------
>> 4 files changed, 65 insertions(+), 22 deletions(-)
>> create mode 100644 block/snapshot.c
>> create mode 100644 include/block/snapshot.h
>
>> +++ b/block/snapshot.c
>> @@ -0,0 +1,37 @@
>> +/*
>> + * Snapshot related functions.
>> + *
>> + * Copyright IBM, Corp. 2013
>
> Technically, since you are moving code from savevm.c, you should also
> preserve the copyright on that moved code:
>
> * Copyright (c) 2003-2008 Fabrice Bellard
>
>> + *
>> + * Authors:
>> + * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
>
> Furthermore, the code you are moving was under BSD license, so by moving
> the code, you have changed its license to something more restrictive.
> Personally, I like LGPL better than BSD, but as I'm not the copyright
> holder of the original code, and neither are you, I'm not sure that
> either of us is qualified to make that change. Therefore, I'm unwilling
> to add my Reviewed-by, even though the code motion itself is correct,
> without a maintainer chiming in on whether your licensing is appropriate
> or needs an adjustment.
>
Oops, Since it belongs to block layer I hope it can be LGPL2. Do you
know how to contact Fabrice Bellard to ask for a change?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
` (13 preceding siblings ...)
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 14/14] hmp: add command info images Wenchao Xia
@ 2013-03-12 10:07 ` Stefan Hajnoczi
2013-03-12 16:16 ` Eric Blake
14 siblings, 1 reply; 47+ messages in thread
From: Stefan Hajnoczi @ 2013-03-12 10:07 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, armbru, qemu-devel, lcapitulino, pbonzini
On Mon, Mar 11, 2013 at 07:23:02PM +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
> following interfaces for qemu:
>
> 1) qmp: query-images, show image info for a block device
> Example:
> -> { "execute": "query-images" }
> <- {
> "return":[
> {
> "device":"ide0-hd0",
> "image":{
> "filename":"disks/test1.img",
> "format":"qcow2",
> "virtual-size":2048000,
> "snapshots":[
> {
> "id": "1",
> "name": "snapshot1",
> "vm-state-size": 0,
> "date-sec": 10000200,
> "date-nsec": 12,
> "vm-clock-sec": 206,
> "vm-clock-nsec": 30
> }
> ]
> }
> }
> ]
> }
See my recent RFC patch to add a virtual_size to query-block:
http://permalink.gmane.org/gmane.comp.emulators.qemu/199443
Is there a reason you added the new query-images command instead of
extending BlockDeviceInfo with an ImageInfo field? Then query-block,
which is already used today, just provides ImageInfo as well.
I suggest this because it makes life easier for clients - they can issue
a single command to extract all image information. If we have two
commands they may have to invoke both and then match the results
together.
Stefan
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-12 5:01 ` Wenchao Xia
@ 2013-03-12 16:15 ` Eric Blake
2013-03-12 16:22 ` Eric Blake
2013-03-13 12:41 ` Kevin Wolf
1 sibling, 1 reply; 47+ messages in thread
From: Eric Blake @ 2013-03-12 16:15 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]
On 03/11/2013 11:01 PM, Wenchao Xia wrote:
> 于 2013-3-12 1:49, Eric Blake 写道:
>> On 03/11/2013 05:23 AM, Wenchao Xia wrote:
>>> This patch adds block/snapshot.c and then moves the function
>>> there. It also fixes small code style errors reported by check script.
>>>
>>> + * This work is licensed under the terms of the GNU LGPL, version 2
>>> or later.
>>
>> Furthermore, the code you are moving was under BSD license, so by moving
>> the code, you have changed its license to something more restrictive.
>> Personally, I like LGPL better than BSD, but as I'm not the copyright
>> holder of the original code, and neither are you, I'm not sure that
>> either of us is qualified to make that change. Therefore, I'm unwilling
>> to add my Reviewed-by, even though the code motion itself is correct,
>> without a maintainer chiming in on whether your licensing is appropriate
>> or needs an adjustment.
>>
> Oops, Since it belongs to block layer I hope it can be LGPL2. Do you
> know how to contact Fabrice Bellard to ask for a change?
As far as I can tell, there is nothing wrong with leaving the file as
BSD licensed instead of trying to insist that it be LGPL. The block
layer will still be [L]GPL because of other files linked together, but
there is nothing inherently wrong with linking a BSD file into an [L]GPL
product. In other words, if you are okay with keeping the existing
looser BSD license on this file only, it still won't change the license
of the overall block layer, and it would save you the hassle of tracking
down earlier authors to ask for a relicense.
--
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] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-12 10:07 ` [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Stefan Hajnoczi
@ 2013-03-12 16:16 ` Eric Blake
2013-03-13 2:54 ` Wenchao Xia
0 siblings, 1 reply; 47+ messages in thread
From: Eric Blake @ 2013-03-12 16:16 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, qemu-devel, armbru, lcapitulino, pbonzini, Wenchao Xia
[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]
On 03/12/2013 04:07 AM, Stefan Hajnoczi wrote:
> On Mon, Mar 11, 2013 at 07:23:02PM +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
>> following interfaces for qemu:
>>
> Is there a reason you added the new query-images command instead of
> extending BlockDeviceInfo with an ImageInfo field? Then query-block,
> which is already used today, just provides ImageInfo as well.
>
> I suggest this because it makes life easier for clients - they can issue
> a single command to extract all image information. If we have two
> commands they may have to invoke both and then match the results
> together.
Indeed, from the libvirt perspective, I would much prefer that a single
command gives me everything, instead of having to call two commands to
build up the complete picture.
--
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] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-12 16:15 ` Eric Blake
@ 2013-03-12 16:22 ` Eric Blake
2013-03-13 1:57 ` Wenchao Xia
0 siblings, 1 reply; 47+ messages in thread
From: Eric Blake @ 2013-03-12 16:22 UTC (permalink / raw)
Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, Wenchao Xia,
armbru
[-- Attachment #1: Type: text/plain, Size: 1309 bytes --]
On 03/12/2013 10:15 AM, Eric Blake wrote:
> As far as I can tell, there is nothing wrong with leaving the file as
> BSD licensed instead of trying to insist that it be LGPL. The block
> layer will still be [L]GPL because of other files linked together, but
> there is nothing inherently wrong with linking a BSD file into an [L]GPL
> product. In other words, if you are okay with keeping the existing
> looser BSD license on this file only, it still won't change the license
> of the overall block layer, and it would save you the hassle of tracking
> down earlier authors to ask for a relicense.
Another alternative is to have two licenses covering appropriate
portions of the file. For example, aio-win32.c has two licenses: a
GPL2-only license for older history, and a GPLv2+ license for all new
changes. In your case, you might be able to write a license that states
that contents of code copied from other files is BSD, but all new
contributions are LGPLv2+.
But again, this is something where I suggest you get an official answer
from a maintainer, and not just opinions from a random reviewer,
regarding what approach you should take to licensing your code motion.
--
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] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 04/14] block: move collect_snapshots() and collect_image_info() to block/qapi.c
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 04/14] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
@ 2013-03-12 19:41 ` Eric Blake
2013-03-13 20:34 ` Eric Blake
0 siblings, 1 reply; 47+ messages in thread
From: Eric Blake @ 2013-03-12 19:41 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]
On 03/11/2013 05:23 AM, Wenchao Xia wrote:
> 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>
> ---
> block/Makefile.objs | 2 +-
> block/qapi.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++
> include/block/qapi.h | 24 ++++++++++++
> qemu-img.c | 86 ++-------------------------------------------
> 4 files changed, 124 insertions(+), 84 deletions(-)
> create mode 100644 block/qapi.c
> create mode 100644 include/block/qapi.h
>
> +++ b/block/qapi.c
> @@ -0,0 +1,96 @@
> +/*
> + * Block layer qmp related functions
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * Authors:
> + * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> + * See the COPYING.LIB file in the top-level directory.
This is another case of moving code from a BSD file into an LGPLv2+
file; depending on what the resolution is for 1/14, you should do the
same thing here.
Everything else looks okay, but I'm reluctant to add reviewed-by without
the license issue sorted.
--
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] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 06/14] block: add check for VM snapshot in bdrv_query_snapshot_info_list()
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 06/14] block: add check for VM snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
@ 2013-03-12 19:45 ` Eric Blake
0 siblings, 0 replies; 47+ messages in thread
From: Eric Blake @ 2013-03-12 19:45 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 1078 bytes --]
On 03/11/2013 05:23 AM, Wenchao Xia wrote:
> This patch adds a parameter to tell whether return valid snapshots
> for whole VM only.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qapi.c | 42 ++++++++++++++++++++++++++++++++++++++++--
> include/block/qapi.h | 1 +
> qemu-img.c | 3 ++-
> 3 files changed, 43 insertions(+), 3 deletions(-)
> + /* Check logic is connected with load_vmstate():
> + Only check the devices that can snapshot, other devices that can't
> + take snapshot, for example, readonly ones, will be ignored in
> + load_vmstate(). */
> + while ((bs1 = bdrv_next(bs1))) {
> + if (bdrv_can_snapshot(bs1) && bs1 != bs) {
Minor optimization - you could rearrange the conjunct and check 'bs1 !=
bs' first, to avoid the overhead of bdrv_can_snapshot on bs. But that
doesn't stop me from giving:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 08/14] qmp: add interface query-snapshots
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 08/14] qmp: add interface query-snapshots Wenchao Xia
@ 2013-03-12 21:12 ` Eric Blake
0 siblings, 0 replies; 47+ messages in thread
From: Eric Blake @ 2013-03-12 21:12 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 572 bytes --]
On 03/11/2013 05:23 AM, Wenchao Xia wrote:
> This interface returns info of valid internal snapshots for whole vm.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qapi.c | 18 +++++++++++++++++
> qapi-schema.json | 14 +++++++++++++
> qmp-commands.hx | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 87 insertions(+), 0 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 09/14] qmp: add interface query-images
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 09/14] qmp: add interface query-images Wenchao Xia
@ 2013-03-12 21:24 ` Eric Blake
0 siblings, 0 replies; 47+ messages in thread
From: Eric Blake @ 2013-03-12 21:24 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]
On 03/11/2013 05:23 AM, Wenchao Xia wrote:
> This mirror function will return image info including snapshots,
> and if specified backing image's info will also be returned. Now
> Qemu have both query-images and query-block interfaces.
In the middle of a sentence, we prefer QEMU or qemu, not Qemu.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qapi.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
> qapi-schema.json | 33 ++++++++++++++++++++
> qmp-commands.hx | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 206 insertions(+), 0 deletions(-)
>
As pointed out at the top level, why do we need a new command? Could
query-devices be enhanced instead to provide this information as
additional dictionary members, so that a single query provides entire
backing chain and internal snapshot data alongside everything else?
> ##
> +# @query-images:
> +#
> +# Get block device image information
> +#
> +# @device: #optional the name of the device to get image info from. If not
> +# specified, all block devices will be queried
Even if we decide to add a new command, I still think this is overkill
for QMP. Just return the data on all the devices, all the time.
> +# @backing: #optional true to show information on backing images, false or
> +# omitted to show just the top image of a block device
Likewise. Calling apps can filter out what they need, without you
having to add filtering into the qmp code. HMP can do filtering, since
it is supposed to be human-friendly, but I don't see a reason to make
QMP give reduced information.
--
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] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-12 16:22 ` Eric Blake
@ 2013-03-13 1:57 ` Wenchao Xia
0 siblings, 0 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-13 1:57 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, stefanha, qemu-devel, armbru, pbonzini, lcapitulino
于 2013-3-13 0:22, Eric Blake 写道:
> On 03/12/2013 10:15 AM, Eric Blake wrote:
>> As far as I can tell, there is nothing wrong with leaving the file as
>> BSD licensed instead of trying to insist that it be LGPL. The block
>> layer will still be [L]GPL because of other files linked together, but
>> there is nothing inherently wrong with linking a BSD file into an [L]GPL
>> product. In other words, if you are okay with keeping the existing
>> looser BSD license on this file only, it still won't change the license
>> of the overall block layer, and it would save you the hassle of tracking
>> down earlier authors to ask for a relicense.
>
> Another alternative is to have two licenses covering appropriate
> portions of the file. For example, aio-win32.c has two licenses: a
> GPL2-only license for older history, and a GPLv2+ license for all new
> changes. In your case, you might be able to write a license that states
> that contents of code copied from other files is BSD, but all new
> contributions are LGPLv2+.
>
> But again, this is something where I suggest you get an official answer
> from a maintainer, and not just opinions from a random reviewer,
> regarding what approach you should take to licensing your code motion.
>
Thanks for your comments, I am ignorant about licenses.
Dual license seems good to me. Paolo, can I have your opinion on
this?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-12 16:16 ` Eric Blake
@ 2013-03-13 2:54 ` Wenchao Xia
2013-03-13 11:34 ` Stefan Hajnoczi
0 siblings, 1 reply; 47+ messages in thread
From: Wenchao Xia @ 2013-03-13 2:54 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, Stefan Hajnoczi, qemu-devel, lcapitulino, pbonzini, armbru
于 2013-3-13 0:16, Eric Blake 写道:
> On 03/12/2013 04:07 AM, Stefan Hajnoczi wrote:
>> On Mon, Mar 11, 2013 at 07:23:02PM +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
>>> following interfaces for qemu:
>>>
>
>> Is there a reason you added the new query-images command instead of
>> extending BlockDeviceInfo with an ImageInfo field? Then query-block,
>> which is already used today, just provides ImageInfo as well.
>>
>> I suggest this because it makes life easier for clients - they can issue
>> a single command to extract all image information. If we have two
>> commands they may have to invoke both and then match the results
>> together.
>
> Indeed, from the libvirt perspective, I would much prefer that a single
> command gives me everything, instead of having to call two commands to
> build up the complete picture.
>
I have no special reason for new interface query-images, it just
show up during the work, and I am pretty OK to merge the information
into query-block especially when the user of it, Eric hope so. Then
the qmp interface need to designed carefully, my idea is adding an item
in BlockDeviceInfo:
{ 'type': 'BlockDeviceInfo',
'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
'*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',
'images': ['ImageInfo']} }}
In this way, new define of structure is not needed, and no break
of API I guess, but disadvantage is there will be some duplicated info
in ImageInfo and other structure in BlockInfo, such as "file,
encrypted".
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-13 2:54 ` Wenchao Xia
@ 2013-03-13 11:34 ` Stefan Hajnoczi
2013-03-13 12:29 ` Eric Blake
0 siblings, 1 reply; 47+ messages in thread
From: Stefan Hajnoczi @ 2013-03-13 11:34 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, armbru, qemu-devel, lcapitulino, pbonzini
On Wed, Mar 13, 2013 at 10:54:50AM +0800, Wenchao Xia wrote:
> 于 2013-3-13 0:16, Eric Blake 写道:
> >On 03/12/2013 04:07 AM, Stefan Hajnoczi wrote:
> >>On Mon, Mar 11, 2013 at 07:23:02PM +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
> >>>following interfaces for qemu:
> >>>
> >
> >>Is there a reason you added the new query-images command instead of
> >>extending BlockDeviceInfo with an ImageInfo field? Then query-block,
> >>which is already used today, just provides ImageInfo as well.
> >>
> >>I suggest this because it makes life easier for clients - they can issue
> >>a single command to extract all image information. If we have two
> >>commands they may have to invoke both and then match the results
> >>together.
> >
> >Indeed, from the libvirt perspective, I would much prefer that a single
> >command gives me everything, instead of having to call two commands to
> >build up the complete picture.
> >
>
> I have no special reason for new interface query-images, it just
> show up during the work, and I am pretty OK to merge the information
> into query-block especially when the user of it, Eric hope so. Then
> the qmp interface need to designed carefully, my idea is adding an item
> in BlockDeviceInfo:
>
> { 'type': 'BlockDeviceInfo',
> 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> '*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',
> 'images': ['ImageInfo']} }}
>
> In this way, new define of structure is not needed, and no break
> of API I guess, but disadvantage is there will be some duplicated info
> in ImageInfo and other structure in BlockInfo, such as "file,
> encrypted".
I'm happy with adding a ImageInfo field into BlockDeviceInfo.
Why did you make it an array and what is the array's layout?
Stefan
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-13 11:34 ` Stefan Hajnoczi
@ 2013-03-13 12:29 ` Eric Blake
2013-03-13 12:35 ` Kevin Wolf
2013-03-13 12:40 ` Stefan Hajnoczi
0 siblings, 2 replies; 47+ messages in thread
From: Eric Blake @ 2013-03-13 12:29 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, qemu-devel, armbru, lcapitulino, pbonzini, Wenchao Xia
[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]
On 03/13/2013 05:34 AM, Stefan Hajnoczi wrote:
>>
>> { 'type': 'BlockDeviceInfo',
>> 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
>> '*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',
>> 'images': ['ImageInfo']} }}
>>
>> In this way, new define of structure is not needed, and no break
>> of API I guess, but disadvantage is there will be some duplicated info
>> in ImageInfo and other structure in BlockInfo, such as "file,
>> encrypted".
>
> I'm happy with adding a ImageInfo field into BlockDeviceInfo.
>
> Why did you make it an array and what is the array's layout?
I think the point of an array of ImageInfo is to let us return data on
an entire backing chain. That is, if I have:
image.raw <- middle.qcow2 (with internal snapshot "a") <- top.qcow2
(with internal snapshots "b", "c")
then I want to see data on image.raw, as well as on snapshots a, b, and
c, all in one command. For my example, 'images' would be a
three-element array, with element 0 describing top.qcow2 and its two
internal snapshots, element 1 describing middle.qcow2 and its snapshot,
and element 2 describing image.raw.
However, it's also worth remembering that there has been a proposal for
having quorums, where a single block device can have multiple images
associated with the top level, and where we would then want a recursive
structure rather than an array for representing backing chain
information of a given information within the array of multiple images
associated with a single quorum device.
--
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] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-13 12:29 ` Eric Blake
@ 2013-03-13 12:35 ` Kevin Wolf
2013-03-13 12:40 ` Stefan Hajnoczi
1 sibling, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2013-03-13 12:35 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, Stefan Hajnoczi, armbru, lcapitulino, pbonzini,
Wenchao Xia
Am 13.03.2013 um 13:29 hat Eric Blake geschrieben:
> On 03/13/2013 05:34 AM, Stefan Hajnoczi wrote:
> >>
> >> { 'type': 'BlockDeviceInfo',
> >> 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> >> '*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',
> >> 'images': ['ImageInfo']} }}
> >>
> >> In this way, new define of structure is not needed, and no break
> >> of API I guess, but disadvantage is there will be some duplicated info
> >> in ImageInfo and other structure in BlockInfo, such as "file,
> >> encrypted".
> >
> > I'm happy with adding a ImageInfo field into BlockDeviceInfo.
> >
> > Why did you make it an array and what is the array's layout?
>
> I think the point of an array of ImageInfo is to let us return data on
> an entire backing chain. That is, if I have:
>
> image.raw <- middle.qcow2 (with internal snapshot "a") <- top.qcow2
> (with internal snapshots "b", "c")
>
> then I want to see data on image.raw, as well as on snapshots a, b, and
> c, all in one command. For my example, 'images' would be a
> three-element array, with element 0 describing top.qcow2 and its two
> internal snapshots, element 1 describing middle.qcow2 and its snapshot,
> and element 2 describing image.raw.
>
> However, it's also worth remembering that there has been a proposal for
> having quorums, where a single block device can have multiple images
> associated with the top level, and where we would then want a recursive
> structure rather than an array for representing backing chain
> information of a given information within the array of multiple images
> associated with a single quorum device.
Yes, please go for a recursive structure. You might also want to get
information about bs->file like query-blockstats already provides, so
you already have a second link today.
I wonder if we could somehow add driver-specific fields to this struct
or if QAPI just breaks down when you try to do something like this. It
is what Quorum would have to use. But that's a separate problem and
shouldn't block this series.
Kevin
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-13 12:29 ` Eric Blake
2013-03-13 12:35 ` Kevin Wolf
@ 2013-03-13 12:40 ` Stefan Hajnoczi
2013-03-15 6:07 ` Wenchao Xia
1 sibling, 1 reply; 47+ messages in thread
From: Stefan Hajnoczi @ 2013-03-13 12:40 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, qemu-devel, armbru, lcapitulino, pbonzini, Wenchao Xia
On Wed, Mar 13, 2013 at 06:29:18AM -0600, Eric Blake wrote:
> On 03/13/2013 05:34 AM, Stefan Hajnoczi wrote:
> >>
> >> { 'type': 'BlockDeviceInfo',
> >> 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> >> '*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',
> >> 'images': ['ImageInfo']} }}
> >>
> >> In this way, new define of structure is not needed, and no break
> >> of API I guess, but disadvantage is there will be some duplicated info
> >> in ImageInfo and other structure in BlockInfo, such as "file,
> >> encrypted".
> >
> > I'm happy with adding a ImageInfo field into BlockDeviceInfo.
> >
> > Why did you make it an array and what is the array's layout?
>
> I think the point of an array of ImageInfo is to let us return data on
> an entire backing chain. That is, if I have:
>
> image.raw <- middle.qcow2 (with internal snapshot "a") <- top.qcow2
> (with internal snapshots "b", "c")
>
> then I want to see data on image.raw, as well as on snapshots a, b, and
> c, all in one command. For my example, 'images' would be a
> three-element array, with element 0 describing top.qcow2 and its two
> internal snapshots, element 1 describing middle.qcow2 and its snapshot,
> and element 2 describing image.raw.
>
> However, it's also worth remembering that there has been a proposal for
> having quorums, where a single block device can have multiple images
> associated with the top level, and where we would then want a recursive
> structure rather than an array for representing backing chain
> information of a given information within the array of multiple images
> associated with a single quorum device.
The same applies for VMDK where one .vmdk can reference multiple extent
files.
This is why I asked about the array. A dict of ImageInfos is needed,
they can refer to each other by key.
If we go with an array we're painting ourselves into a corner.
If we go with a dict it's more complicated and may not be used much.
I'd go for the dict but we need a policy for choosing keys (names) for
the ImageInfo elements. Perhaps BlockDeviceInfo['file'] is the key for
the top-level image and then ImageInfo['backing-filename'] for the
backing chain.
Stefan
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-12 5:01 ` Wenchao Xia
2013-03-12 16:15 ` Eric Blake
@ 2013-03-13 12:41 ` Kevin Wolf
2013-03-13 18:19 ` Markus Armbruster
1 sibling, 1 reply; 47+ messages in thread
From: Kevin Wolf @ 2013-03-13 12:41 UTC (permalink / raw)
To: Wenchao Xia; +Cc: stefanha, qemu-devel, lcapitulino, pbonzini, armbru
Am 12.03.2013 um 06:01 hat Wenchao Xia geschrieben:
> 于 2013-3-12 1:49, Eric Blake 写道:
> >On 03/11/2013 05:23 AM, Wenchao Xia wrote:
> >> 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>
> >>---
> >> block/Makefile.objs | 1 +
> >> block/snapshot.c | 37 +++++++++++++++++++++++++++++++++++++
> >> include/block/snapshot.h | 26 ++++++++++++++++++++++++++
> >> savevm.c | 23 +----------------------
> >> 4 files changed, 65 insertions(+), 22 deletions(-)
> >> create mode 100644 block/snapshot.c
> >> create mode 100644 include/block/snapshot.h
> >
> >>+++ b/block/snapshot.c
> >>@@ -0,0 +1,37 @@
> >>+/*
> >>+ * Snapshot related functions.
> >>+ *
> >>+ * Copyright IBM, Corp. 2013
> >
> >Technically, since you are moving code from savevm.c, you should also
> >preserve the copyright on that moved code:
> >
> > * Copyright (c) 2003-2008 Fabrice Bellard
> >
> >>+ *
> >>+ * Authors:
> >>+ * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>+ *
> >>+ * This work is licensed under the terms of the GNU LGPL, version 2 or later.
> >
> >Furthermore, the code you are moving was under BSD license, so by moving
> >the code, you have changed its license to something more restrictive.
> >Personally, I like LGPL better than BSD, but as I'm not the copyright
> >holder of the original code, and neither are you, I'm not sure that
> >either of us is qualified to make that change. Therefore, I'm unwilling
> >to add my Reviewed-by, even though the code motion itself is correct,
> >without a maintainer chiming in on whether your licensing is appropriate
> >or needs an adjustment.
> >
> Oops, Since it belongs to block layer I hope it can be LGPL2. Do you
> know how to contact Fabrice Bellard to ask for a change?
Fabrice is not the only copyright owner of this file.
Just copy the license as it is, changing licenses is always a nasty
thing and as I'm not a lawyer I prefer to stay on the safe side. The MIT
license works well enough, there's no real reason to change it.
Kevin
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-11 17:49 ` Eric Blake
2013-03-12 5:01 ` Wenchao Xia
@ 2013-03-13 18:08 ` Markus Armbruster
1 sibling, 0 replies; 47+ messages in thread
From: Markus Armbruster @ 2013-03-13 18:08 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, Wenchao Xia
Eric Blake <eblake@redhat.com> writes:
> On 03/11/2013 05:23 AM, Wenchao Xia wrote:
>> 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>
>> ---
>> block/Makefile.objs | 1 +
>> block/snapshot.c | 37 +++++++++++++++++++++++++++++++++++++
>> include/block/snapshot.h | 26 ++++++++++++++++++++++++++
>> savevm.c | 23 +----------------------
>> 4 files changed, 65 insertions(+), 22 deletions(-)
>> create mode 100644 block/snapshot.c
>> create mode 100644 include/block/snapshot.h
>
>> +++ b/block/snapshot.c
>> @@ -0,0 +1,37 @@
>> +/*
>> + * Snapshot related functions.
>> + *
>> + * Copyright IBM, Corp. 2013
>
> Technically, since you are moving code from savevm.c, you should also
> preserve the copyright on that moved code:
>
> * Copyright (c) 2003-2008 Fabrice Bellard
>
>> + *
>> + * Authors:
>> + * Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2 or later.
>
> Furthermore, the code you are moving was under BSD license, so by moving
> the code, you have changed its license to something more restrictive.
> Personally, I like LGPL better than BSD, but as I'm not the copyright
> holder of the original code, and neither are you, I'm not sure that
> either of us is qualified to make that change. Therefore, I'm unwilling
> to add my Reviewed-by, even though the code motion itself is correct,
> without a maintainer chiming in on whether your licensing is appropriate
> or needs an adjustment.
Code motion from a single file to a new file is the simplest case. You
can't go wrong by preserving the license notice exactly then.
Things get a bit messy when you move code to an existing file that
already has a different license notice. Whether that's even possible
depends on the licenses.
An important special case is adding copyrightable GPL'ed modifications
to a permissively-licensed file. This need not be just code motion, it
could also happen when you start with a bit of code moved out of a
permissively- licensed file, then add copyrightable code of your own,
which you want protected by the GPL.
For how to do that properly, check out the Software Freedom Law Center's
white paper "Maintaining Permissive-Licensed Files in a GPL-Licensed
Project: Guidelines for Developers"[*] section 2.2 "Adding GPL’d
modifications to permissive-licensed files".
An in-tree example is hw/hd-geometry.c, which I rewrote substantially
after moving it out of block.c,
[*] https://www.softwarefreedom.org/resources/2007/gpl-non-gpl-collaboration.html
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-13 12:41 ` Kevin Wolf
@ 2013-03-13 18:19 ` Markus Armbruster
2013-03-13 20:28 ` Eric Blake
2013-03-14 9:01 ` Kevin Wolf
0 siblings, 2 replies; 47+ messages in thread
From: Markus Armbruster @ 2013-03-13 18:19 UTC (permalink / raw)
To: Kevin Wolf; +Cc: stefanha, lcapitulino, Wenchao Xia, pbonzini, qemu-devel
Kevin Wolf <kwolf@redhat.com> writes:
> Am 12.03.2013 um 06:01 hat Wenchao Xia geschrieben:
>> Oops, Since it belongs to block layer I hope it can be LGPL2. Do you
>> know how to contact Fabrice Bellard to ask for a change?
>
> Fabrice is not the only copyright owner of this file.
>
> Just copy the license as it is, changing licenses is always a nasty
> thing and as I'm not a lawyer I prefer to stay on the safe side. The MIT
> license works well enough, there's no real reason to change it.
*Relicensing* a file is indeed "nasty" in the sense that it's a huge
hassle: you have to track down all copyright holders and get their
permission.
But this isn't relicensing. This is exercising your *right* to
incorporate permissively-licensed stuff into work covered by a
compatible, stronger license. That right was irrevocably granted to you
by the copyright holders. You don't have to ask anyone to exercise it.
Of course, the stronger license still has to be compatible with GPLv2,
so we can accept the result into QEMU.
If a subsystem has additional requirements on licenses, its maintainers
will explain them to you. For what it's worth, substantial parts of the
block layer are already GPLv2+.
You don't *have* to switch to a stronger license, of course. It's your
choice. Myself, I prefer to protect any substantial work I do with a
strong copyleft license such as GPLv2+.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-13 18:19 ` Markus Armbruster
@ 2013-03-13 20:28 ` Eric Blake
2013-03-14 9:01 ` Kevin Wolf
1 sibling, 0 replies; 47+ messages in thread
From: Eric Blake @ 2013-03-13 20:28 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, stefanha, qemu-devel, lcapitulino, pbonzini,
Wenchao Xia
[-- Attachment #1: Type: text/plain, Size: 600 bytes --]
On 03/13/2013 12:19 PM, Markus Armbruster wrote:
> But this isn't relicensing. This is exercising your *right* to
> incorporate permissively-licensed stuff into work covered by a
> compatible, stronger license. That right was irrevocably granted to you
> by the copyright holders. You don't have to ask anyone to exercise it.
Cool - I like that explanation. In that case, my concern has been
answered, and you are free to add:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 04/14] block: move collect_snapshots() and collect_image_info() to block/qapi.c
2013-03-12 19:41 ` Eric Blake
@ 2013-03-13 20:34 ` Eric Blake
0 siblings, 0 replies; 47+ messages in thread
From: Eric Blake @ 2013-03-13 20:34 UTC (permalink / raw)
Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, Wenchao Xia,
armbru
[-- Attachment #1: Type: text/plain, Size: 968 bytes --]
On 03/12/2013 01:41 PM, Eric Blake wrote:
> On 03/11/2013 05:23 AM, Wenchao Xia wrote:
>> 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.
>>
>
> This is another case of moving code from a BSD file into an LGPLv2+
> file; depending on what the resolution is for 1/14, you should do the
> same thing here.
>
> Everything else looks okay, but I'm reluctant to add reviewed-by without
> the license issue sorted.
Same argument as for 1/14 - you exercised your right to copy code from a
permissive license into a stronger license, which is an acceptable use
of open source code. Hence:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 12/14] block: move dump_human_image_info() to block/qapi.c
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 12/14] block: move dump_human_image_info() to block/qapi.c Wenchao Xia
@ 2013-03-13 20:38 ` Eric Blake
0 siblings, 0 replies; 47+ messages in thread
From: Eric Blake @ 2013-03-13 20:38 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 625 bytes --]
On 03/11/2013 05:23 AM, Wenchao Xia wrote:
> This function is needed later in hmp command, it is also renamed to
> bdrv_image_info_dump().
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qapi.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/block/qapi.h | 1 +
> qemu-img.c | 69 +-------------------------------------------------
> 3 files changed, 69 insertions(+), 68 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-13 18:19 ` Markus Armbruster
2013-03-13 20:28 ` Eric Blake
@ 2013-03-14 9:01 ` Kevin Wolf
2013-03-14 12:10 ` Markus Armbruster
1 sibling, 1 reply; 47+ messages in thread
From: Kevin Wolf @ 2013-03-14 9:01 UTC (permalink / raw)
To: Markus Armbruster
Cc: stefanha, lcapitulino, Wenchao Xia, pbonzini, qemu-devel
Am 13.03.2013 um 19:19 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 12.03.2013 um 06:01 hat Wenchao Xia geschrieben:
> >> Oops, Since it belongs to block layer I hope it can be LGPL2. Do you
> >> know how to contact Fabrice Bellard to ask for a change?
> >
> > Fabrice is not the only copyright owner of this file.
> >
> > Just copy the license as it is, changing licenses is always a nasty
> > thing and as I'm not a lawyer I prefer to stay on the safe side. The MIT
> > license works well enough, there's no real reason to change it.
>
> *Relicensing* a file is indeed "nasty" in the sense that it's a huge
> hassle: you have to track down all copyright holders and get their
> permission.
>
> But this isn't relicensing. This is exercising your *right* to
> incorporate permissively-licensed stuff into work covered by a
> compatible, stronger license. That right was irrevocably granted to you
> by the copyright holders. You don't have to ask anyone to exercise it.
But you have to do it right. This specific patch would introduce a
copyright violation. It's really not that hard to conform to the terms
of the MIT license, but that doesn't mean that you can ignore it. There
is exactly one requirement and it reads like this:
The above copyright notice and this permission notice shall be
included in all copies or substantial portions of the Software.
(I'm still waiting for a patch to blockdev.c, for which you did it
wrong, by the way)
> Of course, the stronger license still has to be compatible with GPLv2,
> so we can accept the result into QEMU.
>
> If a subsystem has additional requirements on licenses, its maintainers
> will explain them to you. For what it's worth, substantial parts of the
> block layer are already GPLv2+.
What parts exactly? As long as there are plans for a libqblock and as
long as it doesn't seem completely impossible to have it under LGPL, I
will ask to use either MIT or LGPL for block layer code (this doesn't
apply to qemu-only code that isn't used in the tools - in this sense,
things like blockdev.c are not part of the block layer)
> You don't *have* to switch to a stronger license, of course. It's your
> choice. Myself, I prefer to protect any substantial work I do with a
> strong copyleft license such as GPLv2+.
And it's my choice if I accept a patch that does nothing except moving
code and switching to a stronger license. It feels wrong to do this,
even though it may be legal. If you want to change the license for
whatever reason, you should at least add substantial code of your own to
justify this.
Kevin
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-14 9:01 ` Kevin Wolf
@ 2013-03-14 12:10 ` Markus Armbruster
2013-03-14 12:53 ` Kevin Wolf
0 siblings, 1 reply; 47+ messages in thread
From: Markus Armbruster @ 2013-03-14 12:10 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, stefanha, pbonzini, Wenchao Xia, lcapitulino
Kevin Wolf <kwolf@redhat.com> writes:
> Am 13.03.2013 um 19:19 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 12.03.2013 um 06:01 hat Wenchao Xia geschrieben:
>> >> Oops, Since it belongs to block layer I hope it can be LGPL2. Do you
>> >> know how to contact Fabrice Bellard to ask for a change?
>> >
>> > Fabrice is not the only copyright owner of this file.
>> >
>> > Just copy the license as it is, changing licenses is always a nasty
>> > thing and as I'm not a lawyer I prefer to stay on the safe side. The MIT
>> > license works well enough, there's no real reason to change it.
>>
>> *Relicensing* a file is indeed "nasty" in the sense that it's a huge
>> hassle: you have to track down all copyright holders and get their
>> permission.
>>
>> But this isn't relicensing. This is exercising your *right* to
>> incorporate permissively-licensed stuff into work covered by a
>> compatible, stronger license. That right was irrevocably granted to you
>> by the copyright holders. You don't have to ask anyone to exercise it.
>
> But you have to do it right. This specific patch would introduce a
> copyright violation. It's really not that hard to conform to the terms
> of the MIT license, but that doesn't mean that you can ignore it. There
> is exactly one requirement and it reads like this:
>
> The above copyright notice and this permission notice shall be
> included in all copies or substantial portions of the Software.
That's why I pointed to resources and examples on how to do it properly.
> (I'm still waiting for a patch to blockdev.c, for which you did it
> wrong, by the way)
Oops, that one fell through the cracks. Patch coming.
>> Of course, the stronger license still has to be compatible with GPLv2,
>> so we can accept the result into QEMU.
>>
>> If a subsystem has additional requirements on licenses, its maintainers
>> will explain them to you. For what it's worth, substantial parts of the
>> block layer are already GPLv2+.
>
> What parts exactly? As long as there are plans for a libqblock and as
> long as it doesn't seem completely impossible to have it under LGPL, I
> will ask to use either MIT or LGPL for block layer code (this doesn't
> apply to qemu-only code that isn't used in the tools - in this sense,
> things like blockdev.c are not part of the block layer)
$ git-grep -lw GPL block block*
block-migration.c
block/blkverify.c
block/gluster.c
block/linux-aio.c
block/raw-aio.h
block/rbd.c
block/sheepdog.c
blockdev-nbd.c
blockdev.c
>> You don't *have* to switch to a stronger license, of course. It's your
>> choice. Myself, I prefer to protect any substantial work I do with a
>> strong copyleft license such as GPLv2+.
>
> And it's my choice if I accept a patch that does nothing except moving
> code and switching to a stronger license. It feels wrong to do this,
> even though it may be legal. If you want to change the license for
> whatever reason, you should at least add substantial code of your own to
> justify this.
I wouldn't submit such a patch. Not everything that's legal is proper.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-14 12:10 ` Markus Armbruster
@ 2013-03-14 12:53 ` Kevin Wolf
2013-03-15 6:23 ` Wenchao Xia
0 siblings, 1 reply; 47+ messages in thread
From: Kevin Wolf @ 2013-03-14 12:53 UTC (permalink / raw)
To: Markus Armbruster
Cc: qemu-devel, stefanha, pbonzini, Wenchao Xia, lcapitulino
Am 14.03.2013 um 13:10 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > But you have to do it right. This specific patch would introduce a
> > copyright violation. It's really not that hard to conform to the terms
> > of the MIT license, but that doesn't mean that you can ignore it. There
> > is exactly one requirement and it reads like this:
> >
> > The above copyright notice and this permission notice shall be
> > included in all copies or substantial portions of the Software.
>
> That's why I pointed to resources and examples on how to do it properly.
>
> > (I'm still waiting for a patch to blockdev.c, for which you did it
> > wrong, by the way)
>
> Oops, that one fell through the cracks. Patch coming.
Thanks.
> >> Of course, the stronger license still has to be compatible with GPLv2,
> >> so we can accept the result into QEMU.
> >>
> >> If a subsystem has additional requirements on licenses, its maintainers
> >> will explain them to you. For what it's worth, substantial parts of the
> >> block layer are already GPLv2+.
> >
> > What parts exactly? As long as there are plans for a libqblock and as
> > long as it doesn't seem completely impossible to have it under LGPL, I
> > will ask to use either MIT or LGPL for block layer code (this doesn't
> > apply to qemu-only code that isn't used in the tools - in this sense,
> > things like blockdev.c are not part of the block layer)
>
> $ git-grep -lw GPL block block*
> block-migration.c
> block/blkverify.c
> block/gluster.c
> block/linux-aio.c
> block/raw-aio.h
> block/rbd.c
> block/sheepdog.c
> blockdev-nbd.c
> blockdev.c
Luckily, none of these are really critical for a libqblock library, even
though they would be nice to have.
If we can't license such a library as LGPL, we would lose the most
important potential user, which is libvirt. In which case I probably
wouldn't want to bother with providing a library at all.
Kevin
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-13 12:40 ` Stefan Hajnoczi
@ 2013-03-15 6:07 ` Wenchao Xia
2013-03-15 8:07 ` Kevin Wolf
2013-03-15 8:44 ` Stefan Hajnoczi
0 siblings, 2 replies; 47+ messages in thread
From: Wenchao Xia @ 2013-03-15 6:07 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, armbru, qemu-devel, lcapitulino, pbonzini
于 2013-3-13 20:40, Stefan Hajnoczi 写道:
> On Wed, Mar 13, 2013 at 06:29:18AM -0600, Eric Blake wrote:
>> On 03/13/2013 05:34 AM, Stefan Hajnoczi wrote:
>>>>
>>>> { 'type': 'BlockDeviceInfo',
>>>> 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
>>>> '*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',
>>>> 'images': ['ImageInfo']} }}
>>>>
>>>> In this way, new define of structure is not needed, and no break
>>>> of API I guess, but disadvantage is there will be some duplicated info
>>>> in ImageInfo and other structure in BlockInfo, such as "file,
>>>> encrypted".
>>>
>>> I'm happy with adding a ImageInfo field into BlockDeviceInfo.
>>>
>>> Why did you make it an array and what is the array's layout?
>>
>> I think the point of an array of ImageInfo is to let us return data on
>> an entire backing chain. That is, if I have:
>>
>> image.raw <- middle.qcow2 (with internal snapshot "a") <- top.qcow2
>> (with internal snapshots "b", "c")
>>
>> then I want to see data on image.raw, as well as on snapshots a, b, and
>> c, all in one command. For my example, 'images' would be a
>> three-element array, with element 0 describing top.qcow2 and its two
>> internal snapshots, element 1 describing middle.qcow2 and its snapshot,
>> and element 2 describing image.raw.
>>
>> However, it's also worth remembering that there has been a proposal for
>> having quorums, where a single block device can have multiple images
>> associated with the top level, and where we would then want a recursive
>> structure rather than an array for representing backing chain
>> information of a given information within the array of multiple images
>> associated with a single quorum device.
>
> The same applies for VMDK where one .vmdk can reference multiple extent
> files.
>
I'd like to confirm: This means a block device can have multiple
images at top level, but one image can still have only one backing
image at most? If my understanding is correct,following should be the
case:
1 device hda have two active images: a.qcow2 and b.qcow2(maybe vmdk
format now, just an example)
2 a.qcow2 file's backing file will be a_base0.qcow2, never a_base0.qcow2
and a_base1.qcow2.
> This is why I asked about the array. A dict of ImageInfos is needed,
> they can refer to each other by key.
>
> If we go with an array we're painting ourselves into a corner.
>
> If we go with a dict it's more complicated and may not be used much.
>
> I'd go for the dict but we need a policy for choosing keys (names) for
> the ImageInfo elements. Perhaps BlockDeviceInfo['file'] is the key for
> the top-level image and then ImageInfo['backing-filename'] for the
> backing chain.
>
> Stefan
>
Since ImageInfo already have key 'backing-filename', which stands for
the information got in this image, not from the backing image,
add a new key 'bakcing-image' for recursive linking:
{ 'type': 'ImageInfo',
'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
'*actual-size': 'int', 'virtual-size': 'int',
'*cluster-size': 'int', '*encrypted': 'bool',
'*backing-filename': 'str', '*full-backing-filename': 'str',
'*backing-filename-format': 'str',
'*backing-image": 'ImageInfo',
'*snapshots': ['SnapshotInfo'] } }
Then insert ImageInfo into BlockDeviceInfo, with key 'images', since
'file' is already used which may break compatibility if we change it.
'images' uses an array for the case when multiple images exist in
top level, not for backing chain.
{ 'type': 'BlockDeviceInfo',
'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
'*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',
'images': ['ImageInfo']} }}
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-14 12:53 ` Kevin Wolf
@ 2013-03-15 6:23 ` Wenchao Xia
2013-03-15 8:00 ` Kevin Wolf
0 siblings, 1 reply; 47+ messages in thread
From: Wenchao Xia @ 2013-03-15 6:23 UTC (permalink / raw)
To: Kevin Wolf; +Cc: lcapitulino, stefanha, pbonzini, Markus Armbruster, qemu-devel
于 2013-3-14 20:53, Kevin Wolf 写道:
> Am 14.03.2013 um 13:10 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>>> But you have to do it right. This specific patch would introduce a
>>> copyright violation. It's really not that hard to conform to the terms
>>> of the MIT license, but that doesn't mean that you can ignore it. There
>>> is exactly one requirement and it reads like this:
>>>
>>> The above copyright notice and this permission notice shall be
>>> included in all copies or substantial portions of the Software.
>>
>> That's why I pointed to resources and examples on how to do it properly.
>>
>>> (I'm still waiting for a patch to blockdev.c, for which you did it
>>> wrong, by the way)
>>
>> Oops, that one fell through the cracks. Patch coming.
>
> Thanks.
>
>>>> Of course, the stronger license still has to be compatible with GPLv2,
>>>> so we can accept the result into QEMU.
>>>>
>>>> If a subsystem has additional requirements on licenses, its maintainers
>>>> will explain them to you. For what it's worth, substantial parts of the
>>>> block layer are already GPLv2+.
>>>
>>> What parts exactly? As long as there are plans for a libqblock and as
>>> long as it doesn't seem completely impossible to have it under LGPL, I
>>> will ask to use either MIT or LGPL for block layer code (this doesn't
>>> apply to qemu-only code that isn't used in the tools - in this sense,
>>> things like blockdev.c are not part of the block layer)
>>
>> $ git-grep -lw GPL block block*
>> block-migration.c
>> block/blkverify.c
>> block/gluster.c
>> block/linux-aio.c
>> block/raw-aio.h
>> block/rbd.c
>> block/sheepdog.c
>> blockdev-nbd.c
>> blockdev.c
>
> Luckily, none of these are really critical for a libqblock library, even
> though they would be nice to have.
>
> If we can't license such a library as LGPL, we would lose the most
> important potential user, which is libvirt. In which case I probably
> wouldn't want to bother with providing a library at all.
>
> Kevin
>
As the discuss, I feel a direct copy of the license is the
easiest way, the original one is a good enough MIT license, if
my understanding is correct.
One more question: should the date be changed to 2003-2013 in
new file?
/*
* QEMU System Emulator
*
* Copyright (c) 2003-2008 Fabrice Bellard
.....
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c
2013-03-15 6:23 ` Wenchao Xia
@ 2013-03-15 8:00 ` Kevin Wolf
0 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2013-03-15 8:00 UTC (permalink / raw)
To: Wenchao Xia
Cc: lcapitulino, stefanha, pbonzini, Markus Armbruster, qemu-devel
Am 15.03.2013 um 07:23 hat Wenchao Xia geschrieben:
> 于 2013-3-14 20:53, Kevin Wolf 写道:
> >Am 14.03.2013 um 13:10 hat Markus Armbruster geschrieben:
> >>Kevin Wolf <kwolf@redhat.com> writes:
> >>>But you have to do it right. This specific patch would introduce a
> >>>copyright violation. It's really not that hard to conform to the terms
> >>>of the MIT license, but that doesn't mean that you can ignore it. There
> >>>is exactly one requirement and it reads like this:
> >>>
> >>> The above copyright notice and this permission notice shall be
> >>> included in all copies or substantial portions of the Software.
> >>
> >>That's why I pointed to resources and examples on how to do it properly.
> >>
> >>>(I'm still waiting for a patch to blockdev.c, for which you did it
> >>>wrong, by the way)
> >>
> >>Oops, that one fell through the cracks. Patch coming.
> >
> >Thanks.
> >
> >>>>Of course, the stronger license still has to be compatible with GPLv2,
> >>>>so we can accept the result into QEMU.
> >>>>
> >>>>If a subsystem has additional requirements on licenses, its maintainers
> >>>>will explain them to you. For what it's worth, substantial parts of the
> >>>>block layer are already GPLv2+.
> >>>
> >>>What parts exactly? As long as there are plans for a libqblock and as
> >>>long as it doesn't seem completely impossible to have it under LGPL, I
> >>>will ask to use either MIT or LGPL for block layer code (this doesn't
> >>>apply to qemu-only code that isn't used in the tools - in this sense,
> >>>things like blockdev.c are not part of the block layer)
> >>
> >>$ git-grep -lw GPL block block*
> >>block-migration.c
> >>block/blkverify.c
> >>block/gluster.c
> >>block/linux-aio.c
> >>block/raw-aio.h
> >>block/rbd.c
> >>block/sheepdog.c
> >>blockdev-nbd.c
> >>blockdev.c
> >
> >Luckily, none of these are really critical for a libqblock library, even
> >though they would be nice to have.
> >
> >If we can't license such a library as LGPL, we would lose the most
> >important potential user, which is libvirt. In which case I probably
> >wouldn't want to bother with providing a library at all.
> >
> >Kevin
> >
> As the discuss, I feel a direct copy of the license is the
> easiest way, the original one is a good enough MIT license, if
> my understanding is correct.
> One more question: should the date be changed to 2003-2013 in
> new file?
>
> /*
> * QEMU System Emulator
> *
> * Copyright (c) 2003-2008 Fabrice Bellard
> .....
Probably not that important, but I would leave it as it is.
Kevin
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-15 6:07 ` Wenchao Xia
@ 2013-03-15 8:07 ` Kevin Wolf
2013-03-18 10:30 ` Wenchao Xia
2013-03-15 8:44 ` Stefan Hajnoczi
1 sibling, 1 reply; 47+ messages in thread
From: Kevin Wolf @ 2013-03-15 8:07 UTC (permalink / raw)
To: Wenchao Xia; +Cc: Stefan Hajnoczi, qemu-devel, lcapitulino, pbonzini, armbru
Am 15.03.2013 um 07:07 hat Wenchao Xia geschrieben:
> 于 2013-3-13 20:40, Stefan Hajnoczi 写道:
> >The same applies for VMDK where one .vmdk can reference multiple extent
> >files.
> >
> I'd like to confirm: This means a block device can have multiple
> images at top level, but one image can still have only one backing
> image at most? If my understanding is correct,following should be the
> case:
>
> 1 device hda have two active images: a.qcow2 and b.qcow2(maybe vmdk
> format now, just an example)
> 2 a.qcow2 file's backing file will be a_base0.qcow2, never a_base0.qcow2
> and a_base1.qcow2.
I don't think this describes exactly what happens. Each qemu block
device refers to one BlockDriverState, let's say top.vmdk for the top
level image. This may refer to a backing file, backing.vmdk for example.
Now top.vmdk nd back.vmdk could both be split images, and the former
refers to its extents top_0.vmdk and top_1.vmdk, which contain the
actual data of the image, and the latter may refer to backing_0.vmdk,
backing_1.vmdk and backing_2.vmdk.
So this can happen in any position in the BlockDriverState graph.
> Since ImageInfo already have key 'backing-filename', which stands for
> the information got in this image, not from the backing image,
> add a new key 'bakcing-image' for recursive linking:
>
> { 'type': 'ImageInfo',
> 'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
> '*actual-size': 'int', 'virtual-size': 'int',
> '*cluster-size': 'int', '*encrypted': 'bool',
> '*backing-filename': 'str', '*full-backing-filename': 'str',
> '*backing-filename-format': 'str',
> '*backing-image": 'ImageInfo',
> '*snapshots': ['SnapshotInfo'] } }
>
> Then insert ImageInfo into BlockDeviceInfo, with key 'images', since
> 'file' is already used which may break compatibility if we change it.
> 'images' uses an array for the case when multiple images exist in
> top level, not for backing chain.
>
> { 'type': 'BlockDeviceInfo',
> 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> '*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',
> 'images': ['ImageInfo']} }}
Yes, I think this is what Stefan meant (at least it looks like what I
was thinking of).
Kevin
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-15 6:07 ` Wenchao Xia
2013-03-15 8:07 ` Kevin Wolf
@ 2013-03-15 8:44 ` Stefan Hajnoczi
1 sibling, 0 replies; 47+ messages in thread
From: Stefan Hajnoczi @ 2013-03-15 8:44 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, armbru, qemu-devel, lcapitulino, pbonzini
On Fri, Mar 15, 2013 at 02:07:01PM +0800, Wenchao Xia wrote:
> 于 2013-3-13 20:40, Stefan Hajnoczi 写道:
> >This is why I asked about the array. A dict of ImageInfos is needed,
> >they can refer to each other by key.
> >
> >If we go with an array we're painting ourselves into a corner.
> >
> >If we go with a dict it's more complicated and may not be used much.
> >
> >I'd go for the dict but we need a policy for choosing keys (names) for
> >the ImageInfo elements. Perhaps BlockDeviceInfo['file'] is the key for
> >the top-level image and then ImageInfo['backing-filename'] for the
> >backing chain.
> >
> >Stefan
> >
>
> Since ImageInfo already have key 'backing-filename', which stands for
> the information got in this image, not from the backing image,
> add a new key 'bakcing-image' for recursive linking:
>
> { 'type': 'ImageInfo',
> 'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
> '*actual-size': 'int', 'virtual-size': 'int',
> '*cluster-size': 'int', '*encrypted': 'bool',
> '*backing-filename': 'str', '*full-backing-filename': 'str',
> '*backing-filename-format': 'str',
> '*backing-image": 'ImageInfo',
> '*snapshots': ['SnapshotInfo'] } }
>
> Then insert ImageInfo into BlockDeviceInfo, with key 'images', since
> 'file' is already used which may break compatibility if we change it.
> 'images' uses an array for the case when multiple images exist in
> top level, not for backing chain.
>
> { 'type': 'BlockDeviceInfo',
> 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> '*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',
> 'images': ['ImageInfo']} }}
Sounds good.
Stefan
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-15 8:07 ` Kevin Wolf
@ 2013-03-18 10:30 ` Wenchao Xia
2013-03-18 16:48 ` Kevin Wolf
0 siblings, 1 reply; 47+ messages in thread
From: Wenchao Xia @ 2013-03-18 10:30 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Stefan Hajnoczi, pbonzini, qemu-devel, armbru, lcapitulino
于 2013-3-15 16:07, Kevin Wolf 写道:
> Am 15.03.2013 um 07:07 hat Wenchao Xia geschrieben:
>> 于 2013-3-13 20:40, Stefan Hajnoczi 写道:
>>> The same applies for VMDK where one .vmdk can reference multiple extent
>>> files.
>>>
>> I'd like to confirm: This means a block device can have multiple
>> images at top level, but one image can still have only one backing
>> image at most? If my understanding is correct,following should be the
>> case:
>>
>> 1 device hda have two active images: a.qcow2 and b.qcow2(maybe vmdk
>> format now, just an example)
>> 2 a.qcow2 file's backing file will be a_base0.qcow2, never a_base0.qcow2
>> and a_base1.qcow2.
>
> I don't think this describes exactly what happens. Each qemu block
> device refers to one BlockDriverState, let's say top.vmdk for the top
> level image. This may refer to a backing file, backing.vmdk for example.
>
> Now top.vmdk nd back.vmdk could both be split images, and the former
> refers to its extents top_0.vmdk and top_1.vmdk, which contain the
> actual data of the image, and the latter may refer to backing_0.vmdk,
> backing_1.vmdk and backing_2.vmdk.
>
> So this can happen in any position in the BlockDriverState graph.
>
Thank u for the detailed explaination. Currently BlockDriverState
have only one file name, so if my understanding is correct, the multiple
referred disk is actually an internal data struct in VMDK now. Qemu main
block layer only saw "main" disk in each layer.
an case:
BlockDriverState:
|
top.vmdk<-----------------------------------ref_top.vmdk
| |
backing_top.vmdk<---ref_backing_top.vmdk backing_ref_top.vmdk
This seems hard to be reflected in the structure I defined before,
and hard to get in currently block.c's API. what I can think is
improve ImageInfo:
{ 'type': 'ImageInfo',
'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
'*actual-size': 'int', 'virtual-size': 'int',
'*cluster-size': 'int', '*encrypted': 'bool',
'*backing-filename': 'str', '*full-backing-filename': 'str',
'*backing-filename-format': 'str',
'*backing-image": 'ImageInfo',
'*ref-images": '[ImageInfo]',
'*snapshots': ['SnapshotInfo'] } }
Backing-image reflect that one image can have one main backing file,
and one image can have multiple referenced images. Do you think this
structure is good enough?
>> Since ImageInfo already have key 'backing-filename', which stands for
>> the information got in this image, not from the backing image,
>> add a new key 'bakcing-image' for recursive linking:
>>
>> { 'type': 'ImageInfo',
>> 'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
>> '*actual-size': 'int', 'virtual-size': 'int',
>> '*cluster-size': 'int', '*encrypted': 'bool',
>> '*backing-filename': 'str', '*full-backing-filename': 'str',
>> '*backing-filename-format': 'str',
>> '*backing-image": 'ImageInfo',
>> '*snapshots': ['SnapshotInfo'] } }
>>
>> Then insert ImageInfo into BlockDeviceInfo, with key 'images', since
>> 'file' is already used which may break compatibility if we change it.
>> 'images' uses an array for the case when multiple images exist in
>> top level, not for backing chain.
>>
>> { 'type': 'BlockDeviceInfo',
>> 'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
>> '*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',
>> 'images': ['ImageInfo']} }}
>
> Yes, I think this is what Stefan meant (at least it looks like what I
> was thinking of).
>
> Kevin
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info
2013-03-18 10:30 ` Wenchao Xia
@ 2013-03-18 16:48 ` Kevin Wolf
0 siblings, 0 replies; 47+ messages in thread
From: Kevin Wolf @ 2013-03-18 16:48 UTC (permalink / raw)
To: Wenchao Xia; +Cc: Stefan Hajnoczi, pbonzini, qemu-devel, armbru, lcapitulino
Am 18.03.2013 um 11:30 hat Wenchao Xia geschrieben:
> 于 2013-3-15 16:07, Kevin Wolf 写道:
> >Am 15.03.2013 um 07:07 hat Wenchao Xia geschrieben:
> >>于 2013-3-13 20:40, Stefan Hajnoczi 写道:
> >>>The same applies for VMDK where one .vmdk can reference multiple extent
> >>>files.
> >>>
> >> I'd like to confirm: This means a block device can have multiple
> >>images at top level, but one image can still have only one backing
> >>image at most? If my understanding is correct,following should be the
> >>case:
> >>
> >>1 device hda have two active images: a.qcow2 and b.qcow2(maybe vmdk
> >>format now, just an example)
> >>2 a.qcow2 file's backing file will be a_base0.qcow2, never a_base0.qcow2
> >>and a_base1.qcow2.
> >
> >I don't think this describes exactly what happens. Each qemu block
> >device refers to one BlockDriverState, let's say top.vmdk for the top
> >level image. This may refer to a backing file, backing.vmdk for example.
> >
> >Now top.vmdk nd back.vmdk could both be split images, and the former
> >refers to its extents top_0.vmdk and top_1.vmdk, which contain the
> >actual data of the image, and the latter may refer to backing_0.vmdk,
> >backing_1.vmdk and backing_2.vmdk.
> >
> >So this can happen in any position in the BlockDriverState graph.
> >
> Thank u for the detailed explaination. Currently BlockDriverState
> have only one file name, so if my understanding is correct, the multiple
> referred disk is actually an internal data struct in VMDK now. Qemu main
> block layer only saw "main" disk in each layer.
>
> an case:
> BlockDriverState:
> |
> top.vmdk<-----------------------------------ref_top.vmdk
> | |
> backing_top.vmdk<---ref_backing_top.vmdk backing_ref_top.vmdk
>
> This seems hard to be reflected in the structure I defined before,
> and hard to get in currently block.c's API. what I can think is
> improve ImageInfo:
>
> { 'type': 'ImageInfo',
> 'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
> '*actual-size': 'int', 'virtual-size': 'int',
> '*cluster-size': 'int', '*encrypted': 'bool',
> '*backing-filename': 'str', '*full-backing-filename': 'str',
> '*backing-filename-format': 'str',
> '*backing-image": 'ImageInfo',
> '*ref-images": '[ImageInfo]',
> '*snapshots': ['SnapshotInfo'] } }
>
> Backing-image reflect that one image can have one main backing file,
> and one image can have multiple referenced images. Do you think this
> structure is good enough?
Yes, I think this is fine.
We don't need to introduce ref-images now, it's just good to have the
option to do it as soon as we need it. This is why we preferred using a
recursive structure instead of an array.
Kevin
^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2013-03-18 16:53 UTC | newest]
Thread overview: 47+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-11 11:23 [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 01/14] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
2013-03-11 17:49 ` Eric Blake
2013-03-12 5:01 ` Wenchao Xia
2013-03-12 16:15 ` Eric Blake
2013-03-12 16:22 ` Eric Blake
2013-03-13 1:57 ` Wenchao Xia
2013-03-13 12:41 ` Kevin Wolf
2013-03-13 18:19 ` Markus Armbruster
2013-03-13 20:28 ` Eric Blake
2013-03-14 9:01 ` Kevin Wolf
2013-03-14 12:10 ` Markus Armbruster
2013-03-14 12:53 ` Kevin Wolf
2013-03-15 6:23 ` Wenchao Xia
2013-03-15 8:00 ` Kevin Wolf
2013-03-13 18:08 ` Markus Armbruster
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 02/14] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 03/14] qemu-img: remove unused parameter in collect_image_info() Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 04/14] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
2013-03-12 19:41 ` Eric Blake
2013-03-13 20:34 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 05/14] block: add snapshot info query function bdrv_query_snapshot_info_list() Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 06/14] block: add check for VM snapshot in bdrv_query_snapshot_info_list() Wenchao Xia
2013-03-12 19:45 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 07/14] block: add image info query function bdrv_query_image_info() Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 08/14] qmp: add interface query-snapshots Wenchao Xia
2013-03-12 21:12 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 09/14] qmp: add interface query-images Wenchao Xia
2013-03-12 21:24 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 10/14] hmp: add function hmp_info_snapshots() Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 11/14] hmp: switch snapshot info function to qmp based one Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 12/14] block: move dump_human_image_info() to block/qapi.c Wenchao Xia
2013-03-13 20:38 ` Eric Blake
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 13/14] block: dump to buffer for bdrv_image_info_dump() Wenchao Xia
2013-03-11 11:23 ` [Qemu-devel] [PATCH V9 14/14] hmp: add command info images Wenchao Xia
2013-03-12 10:07 ` [Qemu-devel] [PATCH V9 00/14] qmp/hmp interfaces for internal snapshot info Stefan Hajnoczi
2013-03-12 16:16 ` Eric Blake
2013-03-13 2:54 ` Wenchao Xia
2013-03-13 11:34 ` Stefan Hajnoczi
2013-03-13 12:29 ` Eric Blake
2013-03-13 12:35 ` Kevin Wolf
2013-03-13 12:40 ` Stefan Hajnoczi
2013-03-15 6:07 ` Wenchao Xia
2013-03-15 8:07 ` Kevin Wolf
2013-03-18 10:30 ` Wenchao Xia
2013-03-18 16:48 ` Kevin Wolf
2013-03-15 8:44 ` Stefan Hajnoczi
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).