* [Qemu-devel] [PATCH 0/7] qapi and snapshot code clean up in block layer
@ 2013-04-26 9:31 Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 1/7] block: drop bs_snapshots global variable Wenchao Xia
` (6 more replies)
0 siblings, 7 replies; 34+ messages in thread
From: Wenchao Xia @ 2013-04-26 9:31 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
These patches are the common part of my hmp/qmp block query series and Pavel's
qmp snapshot command converion series. It mainly does following things:
1 move snapshot related code to block/snapshot.c, qmp and info dumping code to
block/qapi.c.
2 better bdrv_snapshot_find().
3 better info dumping function to get rid of buffer, avoid string truncation.
Note patch 2 and 3, 5 and 6 can be squashed. Sperating them to make review
easier, since some code is already reviewed before.
Stefan Hajnoczi (1):
1 block: drop bs_snapshots global variable
Wenchao Xia (6):
2 block: move bdrv_snapshot_find() to block/snapshot.c
3 block: move snapshot code in block.c to block/snapshot.c
4 block: distinguish id and name in bdrv_find_snapshot()
5 block: move collect_snapshots() and collect_image_info() to block/qapi.c
6 block: move qmp and info dump related code to block/qapi.c
7 block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
block.c | 318 -------------------------------------
block/Makefile.objs | 1 +
block/qapi.c | 362 +++++++++++++++++++++++++++++++++++++++++++
block/snapshot.c | 212 +++++++++++++++++++++++++
include/block/block.h | 29 +----
include/block/block_int.h | 1 +
include/block/qapi.h | 41 +++++
include/block/snapshot.h | 55 +++++++
include/qemu/error-report.h | 1 +
qemu-img.c | 163 +------------------
savevm.c | 75 ++++-----
util/qemu-error.c | 18 ++
12 files changed, 735 insertions(+), 541 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] 34+ messages in thread
* [Qemu-devel] [PATCH 1/7] block: drop bs_snapshots global variable
2013-04-26 9:31 [Qemu-devel] [PATCH 0/7] qapi and snapshot code clean up in block layer Wenchao Xia
@ 2013-04-26 9:31 ` Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 2/7] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
` (5 subsequent siblings)
6 siblings, 0 replies; 34+ messages in thread
From: Wenchao Xia @ 2013-04-26 9:31 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, Stefan Hajnoczi,
pbonzini, Wenchao Xia
From: Stefan Hajnoczi <stefanha@redhat.com>
The bs_snapshots global variable points to the BlockDriverState which
will be used to save vmstate. This is really a savevm.c concept but was
moved into block.c:bdrv_snapshots() when it became clear that hotplug
could result in a dangling pointer.
While auditing the block layer's global state I came upon bs_snapshots
and realized that a variable is not necessary here. Simply find the
first BlockDriverState capable of internal snapshots each time this is
needed.
The behavior of bdrv_snapshots() is preserved across hotplug because new
drives are always appended to the bdrv_states list. This means that
calling the new find_vmstate_bs() function is idempotent - it returns
the same BlockDriverState unless it was hot-unplugged.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block.c | 28 ----------------------------
include/block/block.h | 1 -
savevm.c | 19 +++++++++++++++----
3 files changed, 15 insertions(+), 33 deletions(-)
diff --git a/block.c b/block.c
index aa9a533..fd999e2 100644
--- a/block.c
+++ b/block.c
@@ -99,9 +99,6 @@ static QTAILQ_HEAD(, BlockDriverState) bdrv_states =
static QLIST_HEAD(, BlockDriver) bdrv_drivers =
QLIST_HEAD_INITIALIZER(bdrv_drivers);
-/* The device to use for VM snapshots */
-static BlockDriverState *bs_snapshots;
-
/* If non-zero, use only whitelisted block drivers */
static int use_bdrv_whitelist;
@@ -1357,9 +1354,6 @@ void bdrv_close(BlockDriverState *bs)
notifier_list_notify(&bs->close_notifiers, bs);
if (bs->drv) {
- if (bs == bs_snapshots) {
- bs_snapshots = NULL;
- }
if (bs->backing_hd) {
bdrv_delete(bs->backing_hd);
bs->backing_hd = NULL;
@@ -1591,7 +1585,6 @@ void bdrv_delete(BlockDriverState *bs)
bdrv_close(bs);
- assert(bs != bs_snapshots);
g_free(bs);
}
@@ -1635,9 +1628,6 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
{
bs->dev_ops = ops;
bs->dev_opaque = opaque;
- if (bdrv_dev_has_removable_media(bs) && bs == bs_snapshots) {
- bs_snapshots = NULL;
- }
}
void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
@@ -3381,24 +3371,6 @@ int bdrv_is_snapshot(BlockDriverState *bs)
return !!(bs->open_flags & BDRV_O_SNAPSHOT);
}
-BlockDriverState *bdrv_snapshots(void)
-{
- BlockDriverState *bs;
-
- if (bs_snapshots) {
- return bs_snapshots;
- }
-
- bs = NULL;
- while ((bs = bdrv_next(bs))) {
- if (bdrv_can_snapshot(bs)) {
- bs_snapshots = bs;
- return bs;
- }
- }
- return NULL;
-}
-
int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info)
{
diff --git a/include/block/block.h b/include/block/block.h
index 1251c5c..38263b9 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -332,7 +332,6 @@ BlockInfo *bdrv_query_info(BlockDriverState *s);
BlockStats *bdrv_query_stats(const BlockDriverState *bs);
int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_is_snapshot(BlockDriverState *bs);
-BlockDriverState *bdrv_snapshots(void);
int bdrv_snapshot_create(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/savevm.c b/savevm.c
index 31dcce9..baa1a09 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2262,6 +2262,17 @@ out:
return ret;
}
+static BlockDriverState *find_vmstate_bs(void)
+{
+ BlockDriverState *bs = NULL;
+ while ((bs = bdrv_next(bs))) {
+ if (bdrv_can_snapshot(bs)) {
+ return bs;
+ }
+ }
+ return NULL;
+}
+
static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
const char *name)
{
@@ -2338,7 +2349,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
}
}
- bs = bdrv_snapshots();
+ bs = find_vmstate_bs();
if (!bs) {
monitor_printf(mon, "No block device can accept snapshots\n");
return;
@@ -2440,7 +2451,7 @@ int load_vmstate(const char *name)
QEMUFile *f;
int ret;
- bs_vm_state = bdrv_snapshots();
+ bs_vm_state = find_vmstate_bs();
if (!bs_vm_state) {
error_report("No block device supports snapshots");
return -ENOTSUP;
@@ -2519,7 +2530,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
int ret;
const char *name = qdict_get_str(qdict, "name");
- bs = bdrv_snapshots();
+ bs = find_vmstate_bs();
if (!bs) {
monitor_printf(mon, "No block device supports snapshots\n");
return;
@@ -2551,7 +2562,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
int *available_snapshots;
char buf[256];
- bs = bdrv_snapshots();
+ bs = find_vmstate_bs();
if (!bs) {
monitor_printf(mon, "No available block device supports snapshots\n");
return;
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 2/7] block: move bdrv_snapshot_find() to block/snapshot.c
2013-04-26 9:31 [Qemu-devel] [PATCH 0/7] qapi and snapshot code clean up in block layer Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 1/7] block: drop bs_snapshots global variable Wenchao Xia
@ 2013-04-26 9:31 ` Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 3/7] block: move snapshot code in block.c " Wenchao Xia
` (4 subsequent siblings)
6 siblings, 0 replies; 34+ messages in thread
From: Wenchao Xia @ 2013-04-26 9:31 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This patch adds block/snapshot.c and then moves the function there.
It also fixes small code style errors reported by check script.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/Makefile.objs | 1 +
block/snapshot.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++
include/block/snapshot.h | 37 +++++++++++++++++++++++++++++++++++
savevm.c | 23 +---------------------
4 files changed, 87 insertions(+), 22 deletions(-)
create mode 100644 block/snapshot.c
create mode 100644 include/block/snapshot.h
diff --git a/block/Makefile.objs b/block/Makefile.objs
index 6c4b5bc..df55061 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -3,6 +3,7 @@ block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o qcow2-c
block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
block-obj-y += qed-check.o
block-obj-y += parallels.o blkdebug.o blkverify.o
+block-obj-y += snapshot.o
block-obj-$(CONFIG_WIN32) += raw-win32.o win32-aio.o
block-obj-$(CONFIG_POSIX) += raw-posix.o
block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
diff --git a/block/snapshot.c b/block/snapshot.c
new file mode 100644
index 0000000..c47a899
--- /dev/null
+++ b/block/snapshot.c
@@ -0,0 +1,48 @@
+/*
+ * Block layer snapshot related functions
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "block/snapshot.h"
+
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+ const char *name)
+{
+ QEMUSnapshotInfo *sn_tab, *sn;
+ int nb_sns, i, ret;
+
+ ret = -ENOENT;
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ if (nb_sns < 0) {
+ return ret;
+ }
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ ret = 0;
+ break;
+ }
+ }
+ g_free(sn_tab);
+ return ret;
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
new file mode 100644
index 0000000..4ad070c
--- /dev/null
+++ b/include/block/snapshot.h
@@ -0,0 +1,37 @@
+/*
+ * Block layer snapshot related functions
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef SNAPSHOT_H
+#define SNAPSHOT_H
+
+#include "qemu-common.h"
+/*
+ * block.h is needed for QEMUSnapshotInfo, it can be removed when define is
+ * moved here.
+ */
+#include "block.h"
+
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+ const char *name);
+#endif
diff --git a/savevm.c b/savevm.c
index baa1a09..ab53a02 100644
--- a/savevm.c
+++ b/savevm.c
@@ -40,6 +40,7 @@
#include "trace.h"
#include "qemu/bitops.h"
#include "qemu/iov.h"
+#include "block/snapshot.h"
#define SELF_ANNOUNCE_ROUNDS 5
@@ -2273,28 +2274,6 @@ static BlockDriverState *find_vmstate_bs(void)
return NULL;
}
-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] 34+ messages in thread
* [Qemu-devel] [PATCH 3/7] block: move snapshot code in block.c to block/snapshot.c
2013-04-26 9:31 [Qemu-devel] [PATCH 0/7] qapi and snapshot code clean up in block layer Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 1/7] block: drop bs_snapshots global variable Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 2/7] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
@ 2013-04-26 9:31 ` Wenchao Xia
2013-04-26 19:05 ` Eric Blake
2013-04-26 9:31 ` [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
` (3 subsequent siblings)
6 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-04-26 9:31 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
All snapshot related code, except bdrv_snapshot_dump(), is moved to
block/snapshot.c. bdrv_snapshot_dump() will be moved to another file later.
It also fixes small code style errors reported by check script.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block.c | 105 ------------------------------------------
block/snapshot.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 28 ++---------
include/block/snapshot.h | 27 +++++++++--
4 files changed, 142 insertions(+), 132 deletions(-)
diff --git a/block.c b/block.c
index fd999e2..06445c8 100644
--- a/block.c
+++ b/block.c
@@ -3346,111 +3346,6 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
return false;
}
-/**************************************************************/
-/* handling of snapshots */
-
-int bdrv_can_snapshot(BlockDriverState *bs)
-{
- BlockDriver *drv = bs->drv;
- if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
- return 0;
- }
-
- if (!drv->bdrv_snapshot_create) {
- if (bs->file != NULL) {
- return bdrv_can_snapshot(bs->file);
- }
- return 0;
- }
-
- return 1;
-}
-
-int bdrv_is_snapshot(BlockDriverState *bs)
-{
- return !!(bs->open_flags & BDRV_O_SNAPSHOT);
-}
-
-int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info)
-{
- BlockDriver *drv = bs->drv;
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_create)
- return drv->bdrv_snapshot_create(bs, sn_info);
- if (bs->file)
- return bdrv_snapshot_create(bs->file, sn_info);
- return -ENOTSUP;
-}
-
-int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id)
-{
- BlockDriver *drv = bs->drv;
- int ret, open_ret;
-
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_goto)
- return drv->bdrv_snapshot_goto(bs, snapshot_id);
-
- if (bs->file) {
- drv->bdrv_close(bs);
- ret = bdrv_snapshot_goto(bs->file, snapshot_id);
- open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
- if (open_ret < 0) {
- bdrv_delete(bs->file);
- bs->drv = NULL;
- return open_ret;
- }
- return ret;
- }
-
- return -ENOTSUP;
-}
-
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
-{
- BlockDriver *drv = bs->drv;
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_delete)
- return drv->bdrv_snapshot_delete(bs, snapshot_id);
- if (bs->file)
- return bdrv_snapshot_delete(bs->file, snapshot_id);
- return -ENOTSUP;
-}
-
-int bdrv_snapshot_list(BlockDriverState *bs,
- QEMUSnapshotInfo **psn_info)
-{
- BlockDriver *drv = bs->drv;
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_list)
- return drv->bdrv_snapshot_list(bs, psn_info);
- if (bs->file)
- return bdrv_snapshot_list(bs->file, psn_info);
- return -ENOTSUP;
-}
-
-int bdrv_snapshot_load_tmp(BlockDriverState *bs,
- const char *snapshot_name)
-{
- BlockDriver *drv = bs->drv;
- if (!drv) {
- return -ENOMEDIUM;
- }
- if (!bs->read_only) {
- return -EINVAL;
- }
- if (drv->bdrv_snapshot_load_tmp) {
- return drv->bdrv_snapshot_load_tmp(bs, snapshot_name);
- }
- return -ENOTSUP;
-}
-
/* backing_file can either be relative, or absolute, or a protocol. If it is
* relative, it must be relative to the chain. So, passing in bs->filename
* from a BDS as backing_file should not be done, as that may be relative to
diff --git a/block/snapshot.c b/block/snapshot.c
index c47a899..cdc2a76 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -23,6 +23,7 @@
*/
#include "block/snapshot.h"
+#include "block/block_int.h"
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
const char *name)
@@ -46,3 +47,116 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
g_free(sn_tab);
return ret;
}
+
+int bdrv_can_snapshot(BlockDriverState *bs)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+ return 0;
+ }
+
+ if (!drv->bdrv_snapshot_create) {
+ if (bs->file != NULL) {
+ return bdrv_can_snapshot(bs->file);
+ }
+ return 0;
+ }
+
+ return 1;
+}
+
+int bdrv_is_snapshot(BlockDriverState *bs)
+{
+ return !!(bs->open_flags & BDRV_O_SNAPSHOT);
+}
+
+int bdrv_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (drv->bdrv_snapshot_create) {
+ return drv->bdrv_snapshot_create(bs, sn_info);
+ }
+ if (bs->file) {
+ return bdrv_snapshot_create(bs->file, sn_info);
+ }
+ return -ENOTSUP;
+}
+
+int bdrv_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id)
+{
+ BlockDriver *drv = bs->drv;
+ int ret, open_ret;
+
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (drv->bdrv_snapshot_goto) {
+ return drv->bdrv_snapshot_goto(bs, snapshot_id);
+ }
+
+ if (bs->file) {
+ drv->bdrv_close(bs);
+ ret = bdrv_snapshot_goto(bs->file, snapshot_id);
+ open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
+ if (open_ret < 0) {
+ bdrv_delete(bs->file);
+ bs->drv = NULL;
+ return open_ret;
+ }
+ return ret;
+ }
+
+ return -ENOTSUP;
+}
+
+int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (drv->bdrv_snapshot_delete) {
+ return drv->bdrv_snapshot_delete(bs, snapshot_id);
+ }
+ if (bs->file) {
+ return bdrv_snapshot_delete(bs->file, snapshot_id);
+ }
+ return -ENOTSUP;
+}
+
+int bdrv_snapshot_list(BlockDriverState *bs,
+ QEMUSnapshotInfo **psn_info)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (drv->bdrv_snapshot_list) {
+ return drv->bdrv_snapshot_list(bs, psn_info);
+ }
+ if (bs->file) {
+ return bdrv_snapshot_list(bs->file, psn_info);
+ }
+ return -ENOTSUP;
+}
+
+int bdrv_snapshot_load_tmp(BlockDriverState *bs,
+ const char *snapshot_name)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (!bs->read_only) {
+ return -EINVAL;
+ }
+ if (drv->bdrv_snapshot_load_tmp) {
+ return drv->bdrv_snapshot_load_tmp(bs, snapshot_name);
+ }
+ return -ENOTSUP;
+}
diff --git a/include/block/block.h b/include/block/block.h
index 38263b9..fde6fb8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -7,6 +7,11 @@
#include "block/coroutine.h"
#include "qapi/qmp/qobject.h"
#include "qapi-types.h"
+/*
+ * snapshot.h is needed since bdrv_snapshot_dump(), it can be removed when the
+ * function is moved to other file.
+ */
+#include "block/snapshot.h"
/* block.c */
typedef struct BlockDriver BlockDriver;
@@ -27,17 +32,6 @@ typedef struct BlockFragInfo {
uint64_t compressed_clusters;
} BlockFragInfo;
-typedef struct QEMUSnapshotInfo {
- char id_str[128]; /* unique snapshot id */
- /* the following fields are informative. They are not needed for
- the consistency of the snapshot */
- char name[256]; /* user chosen name */
- uint64_t vm_state_size; /* VM state info size */
- uint32_t date_sec; /* UTC date of the snapshot */
- uint32_t date_nsec;
- uint64_t vm_clock_nsec; /* VM clock relative to boot */
-} QEMUSnapshotInfo;
-
/* Callbacks for block device models */
typedef struct BlockDevOps {
/*
@@ -330,17 +324,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
char *dest, size_t sz);
BlockInfo *bdrv_query_info(BlockDriverState *s);
BlockStats *bdrv_query_stats(const BlockDriverState *bs);
-int bdrv_can_snapshot(BlockDriverState *bs);
-int bdrv_is_snapshot(BlockDriverState *bs);
-int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info);
-int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id);
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
-int bdrv_snapshot_list(BlockDriverState *bs,
- QEMUSnapshotInfo **psn_info);
-int bdrv_snapshot_load_tmp(BlockDriverState *bs,
- const char *snapshot_name);
+
char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
char *get_human_readable_size(char *buf, int buf_size, int64_t size);
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 4ad070c..96d4a50 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -26,12 +26,29 @@
#define SNAPSHOT_H
#include "qemu-common.h"
-/*
- * block.h is needed for QEMUSnapshotInfo, it can be removed when define is
- * moved here.
- */
-#include "block.h"
+
+typedef struct QEMUSnapshotInfo {
+ char id_str[128]; /* unique snapshot id */
+ /* the following fields are informative. They are not needed for
+ the consistency of the snapshot */
+ char name[256]; /* user chosen name */
+ uint64_t vm_state_size; /* VM state info size */
+ uint32_t date_sec; /* UTC date of the snapshot */
+ uint32_t date_nsec;
+ uint64_t vm_clock_nsec; /* VM clock relative to boot */
+} QEMUSnapshotInfo;
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
const char *name);
+int bdrv_can_snapshot(BlockDriverState *bs);
+int bdrv_is_snapshot(BlockDriverState *bs);
+int bdrv_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info);
+int bdrv_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id);
+int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int bdrv_snapshot_list(BlockDriverState *bs,
+ QEMUSnapshotInfo **psn_info);
+int bdrv_snapshot_load_tmp(BlockDriverState *bs,
+ const char *snapshot_name);
#endif
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()
2013-04-26 9:31 [Qemu-devel] [PATCH 0/7] qapi and snapshot code clean up in block layer Wenchao Xia
` (2 preceding siblings ...)
2013-04-26 9:31 ` [Qemu-devel] [PATCH 3/7] block: move snapshot code in block.c " Wenchao Xia
@ 2013-04-26 9:31 ` Wenchao Xia
2013-04-26 14:34 ` Stefan Hajnoczi
2013-04-30 18:16 ` Eric Blake
2013-04-26 9:31 ` [Qemu-devel] [PATCH 5/7] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
` (2 subsequent siblings)
6 siblings, 2 replies; 34+ messages in thread
From: Wenchao Xia @ 2013-04-26 9:31 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, 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.
If not found, *errp will be set to tip why.
Note that the caller logic is changed a bit:
1) In del_existing_snapshots() called by do_savevm(), it travers twice
to find the snapshot, instead once, so matching sequence may change
if there are unwisely chosen, mixed id and names.
2) In do_savevm(), same with del_existing_snapshot(), when it tries to
find the snapshot to overwrite, matching sequence may change for same
reason.
3) In load_vmstate(), first when it tries to find the snapshot to be loaded,
sequence may change for the same reason of above. Later in validation, the
logic is changed to be more strict to require both id and name matching.
4) In do_info_snapshot(), in validation, the logic is changed to be more
strict to require both id and name matching.
Savevm, loadvm logic may need to be improved later, to avoid mixing of them.
Some code is borrowed from Pavel's patch.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
block/snapshot.c | 72 +++++++++++++++++++++++++++++++++++++++-------
include/block/snapshot.h | 5 ++-
savevm.c | 35 ++++++++++++----------
3 files changed, 83 insertions(+), 29 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index cdc2a76..fa02c4e 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -25,25 +25,75 @@
#include "block/snapshot.h"
#include "block/block_int.h"
-int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
- const char *name)
+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @sn_info: location to store information on the snapshot found
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * This function will travers snapshot list in @bs to search the matching
+ * one, @id and @name are the matching condition:
+ * If both @id and @name are specified, find the first one with id @id and
+ * name @name.
+ * If only @id is specified, find the first one with id @id.
+ * If only @name is specified, find the first one with name @name.
+ * if none is specified, return false.
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found with @errp filled if errp != NULL.
+ */
+bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+ const char *id, const char *name, Error **errp)
{
QEMUSnapshotInfo *sn_tab, *sn;
- int nb_sns, i, ret;
+ int nb_sns, i;
+ bool ret = false;
- ret = -ENOENT;
nb_sns = bdrv_snapshot_list(bs, &sn_tab);
if (nb_sns < 0) {
- return ret;
+ error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+ return false;
+ } else if (nb_sns == 0) {
+ error_setg(errp, "Device has no snapshots");
+ return false;
}
- 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;
+
+
+ if (id && name) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ ret = true;
+ break;
+ }
+ }
+ } else if (id) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->id_str, id)) {
+ *sn_info = *sn;
+ ret = true;
+ break;
+ }
+ }
+ } else if (name) {
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ if (!strcmp(sn->name, name)) {
+ *sn_info = *sn;
+ ret = true;
+ break;
+ }
}
}
+
+ if (!ret) {
+ error_setg(errp, "Device have no matching snapshot");
+ }
+
g_free(sn_tab);
return ret;
}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 96d4a50..6aac5ed 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -26,6 +26,7 @@
#define SNAPSHOT_H
#include "qemu-common.h"
+#include "qapi/error.h"
typedef struct QEMUSnapshotInfo {
char id_str[128]; /* unique snapshot id */
@@ -38,8 +39,8 @@ typedef struct QEMUSnapshotInfo {
uint64_t vm_clock_nsec; /* VM clock relative to boot */
} QEMUSnapshotInfo;
-int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
- const char *name);
+bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+ const char *id, const char *name, Error **errp);
int bdrv_can_snapshot(BlockDriverState *bs);
int bdrv_is_snapshot(BlockDriverState *bs);
int bdrv_snapshot_create(BlockDriverState *bs,
diff --git a/savevm.c b/savevm.c
index ab53a02..5dd2d14 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2286,8 +2286,8 @@ 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, NULL, NULL) ||
+ bdrv_snapshot_find(bs, snapshot, NULL, name, NULL))) {
ret = bdrv_snapshot_delete(bs, name);
if (ret < 0) {
monitor_printf(mon,
@@ -2346,8 +2346,8 @@ 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);
- if (ret >= 0) {
+ if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL) ||
+ bdrv_snapshot_find(bs, old_sn, NULL, name, NULL)) {
pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
} else {
@@ -2437,12 +2437,14 @@ int load_vmstate(const char *name)
}
/* Don't even try to load empty VM states */
- ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
- if (ret < 0) {
- return ret;
- } else if (sn.vm_state_size == 0) {
- error_report("This is a disk-only snapshot. Revert to it offline "
- "using qemu-img.");
+ if (bdrv_snapshot_find(bs_vm_state, &sn, name, NULL, NULL) ||
+ bdrv_snapshot_find(bs_vm_state, &sn, NULL, name, NULL)) {
+ if (sn.vm_state_size == 0) {
+ error_report("This is a disk-only snapshot. Revert to it "
+ "offline using qemu-img.");
+ return -EINVAL;
+ }
+ } else {
return -EINVAL;
}
@@ -2461,11 +2463,11 @@ int load_vmstate(const char *name)
return -ENOTSUP;
}
- ret = bdrv_snapshot_find(bs, &sn, name);
- if (ret < 0) {
+ /* vm snapshot will always have same id and name, check do_savevm(). */
+ if (!bdrv_snapshot_find(bs, &sn, sn.id_str, sn.name, NULL)) {
error_report("Device '%s' does not have the requested snapshot '%s'",
bdrv_get_device_name(bs), name);
- return ret;
+ return -ENOENT;
}
}
@@ -2536,7 +2538,7 @@ 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 nb_sns, i, available;
int total;
int *available_snapshots;
char buf[256];
@@ -2567,8 +2569,9 @@ 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);
- if (ret < 0) {
+ /* vm snapshot will always have same id and name */
+ if (!bdrv_snapshot_find(bs1, sn_info,
+ sn->id_str, sn->name, NULL)) {
available = 0;
break;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 5/7] block: move collect_snapshots() and collect_image_info() to block/qapi.c
2013-04-26 9:31 [Qemu-devel] [PATCH 0/7] qapi and snapshot code clean up in block layer Wenchao Xia
` (3 preceding siblings ...)
2013-04-26 9:31 ` [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
@ 2013-04-26 9:31 ` Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 6/7] block: move qmp and info dump related code " Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
6 siblings, 0 replies; 34+ messages in thread
From: Wenchao Xia @ 2013-04-26 9:31 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, 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.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
block/Makefile.objs | 2 +-
block/qapi.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/block/qapi.h | 35 ++++++++++++++++
qemu-img.c | 87 +---------------------------------------
4 files changed, 146 insertions(+), 85 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 df55061..1c757ff 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..0b57df2
--- /dev/null
+++ b/block/qapi.c
@@ -0,0 +1,107 @@
+/*
+ * Block layer qmp and info dump related functions
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "block/qapi.h"
+#include "block/block_int.h"
+
+void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info)
+{
+ int i, sn_count;
+ QEMUSnapshotInfo *sn_tab = NULL;
+ SnapshotInfoList *info_list, *cur_item = NULL;
+ sn_count = bdrv_snapshot_list(bs, &sn_tab);
+
+ for (i = 0; i < sn_count; i++) {
+ info->has_snapshots = true;
+ info_list = g_new0(SnapshotInfoList, 1);
+
+ info_list->value = g_new0(SnapshotInfo, 1);
+ info_list->value->id = g_strdup(sn_tab[i].id_str);
+ info_list->value->name = g_strdup(sn_tab[i].name);
+ info_list->value->vm_state_size = sn_tab[i].vm_state_size;
+ info_list->value->date_sec = sn_tab[i].date_sec;
+ info_list->value->date_nsec = sn_tab[i].date_nsec;
+ info_list->value->vm_clock_sec = sn_tab[i].vm_clock_nsec / 1000000000;
+ info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000;
+
+ /* XXX: waiting for the qapi to support qemu-queue.h types */
+ if (!cur_item) {
+ info->snapshots = cur_item = info_list;
+ } else {
+ cur_item->next = info_list;
+ cur_item = info_list;
+ }
+
+ }
+
+ g_free(sn_tab);
+}
+
+void bdrv_collect_image_info(BlockDriverState *bs,
+ ImageInfo *info,
+ const char *filename)
+{
+ uint64_t total_sectors;
+ char backing_filename[1024];
+ char backing_filename2[1024];
+ BlockDriverInfo bdi;
+
+ bdrv_get_geometry(bs, &total_sectors);
+
+ info->filename = g_strdup(filename);
+ info->format = g_strdup(bdrv_get_format_name(bs));
+ info->virtual_size = total_sectors * 512;
+ info->actual_size = bdrv_get_allocated_file_size(bs);
+ info->has_actual_size = info->actual_size >= 0;
+ if (bdrv_is_encrypted(bs)) {
+ info->encrypted = true;
+ info->has_encrypted = true;
+ }
+ if (bdrv_get_info(bs, &bdi) >= 0) {
+ if (bdi.cluster_size != 0) {
+ info->cluster_size = bdi.cluster_size;
+ info->has_cluster_size = true;
+ }
+ info->dirty_flag = bdi.is_dirty;
+ info->has_dirty_flag = true;
+ }
+ bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
+ if (backing_filename[0] != '\0') {
+ info->backing_filename = g_strdup(backing_filename);
+ info->has_backing_filename = true;
+ bdrv_get_full_backing_filename(bs, backing_filename2,
+ sizeof(backing_filename2));
+
+ if (strcmp(backing_filename, backing_filename2) != 0) {
+ info->full_backing_filename =
+ g_strdup(backing_filename2);
+ info->has_full_backing_filename = true;
+ }
+
+ if (bs->backing_format[0]) {
+ info->backing_filename_format = g_strdup(bs->backing_format);
+ info->has_backing_filename_format = true;
+ }
+ }
+}
diff --git a/include/block/qapi.h b/include/block/qapi.h
new file mode 100644
index 0000000..22a447f
--- /dev/null
+++ b/include/block/qapi.h
@@ -0,0 +1,35 @@
+/*
+ * Block layer qmp and info dump related functions
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef BLOCK_QAPI_H
+#define BLOCK_QAPI_H
+
+#include "qapi-types.h"
+#include "block/block.h"
+
+void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info);
+void bdrv_collect_image_info(BlockDriverState *bs,
+ ImageInfo *info,
+ const char *filename);
+#endif
diff --git a/qemu-img.c b/qemu-img.c
index cd096a1..c19ee58 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>
@@ -1584,39 +1585,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;
@@ -1634,55 +1602,6 @@ static void dump_json_image_info(ImageInfo *info)
QDECREF(str);
}
-static void collect_image_info(BlockDriverState *bs,
- ImageInfo *info,
- const char *filename,
- const char *fmt)
-{
- uint64_t total_sectors;
- char backing_filename[1024];
- char backing_filename2[1024];
- BlockDriverInfo bdi;
-
- bdrv_get_geometry(bs, &total_sectors);
-
- info->filename = g_strdup(filename);
- info->format = g_strdup(bdrv_get_format_name(bs));
- info->virtual_size = total_sectors * 512;
- info->actual_size = bdrv_get_allocated_file_size(bs);
- info->has_actual_size = info->actual_size >= 0;
- if (bdrv_is_encrypted(bs)) {
- info->encrypted = true;
- info->has_encrypted = true;
- }
- if (bdrv_get_info(bs, &bdi) >= 0) {
- if (bdi.cluster_size != 0) {
- info->cluster_size = bdi.cluster_size;
- info->has_cluster_size = true;
- }
- info->dirty_flag = bdi.is_dirty;
- info->has_dirty_flag = true;
- }
- bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
- if (backing_filename[0] != '\0') {
- info->backing_filename = g_strdup(backing_filename);
- info->has_backing_filename = true;
- bdrv_get_full_backing_filename(bs, backing_filename2,
- sizeof(backing_filename2));
-
- if (strcmp(backing_filename, backing_filename2) != 0) {
- info->full_backing_filename =
- g_strdup(backing_filename2);
- info->has_full_backing_filename = true;
- }
-
- if (bs->backing_format[0]) {
- info->backing_filename_format = g_strdup(bs->backing_format);
- info->has_backing_filename_format = true;
- }
- }
-}
-
static void dump_human_image_info(ImageInfo *info)
{
char size_buf[128], dsize_buf[128];
@@ -1811,8 +1730,8 @@ static ImageInfoList *collect_image_info_list(const char *filename,
}
info = g_new0(ImageInfo, 1);
- collect_image_info(bs, info, filename, fmt);
- 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] 34+ messages in thread
* [Qemu-devel] [PATCH 6/7] block: move qmp and info dump related code to block/qapi.c
2013-04-26 9:31 [Qemu-devel] [PATCH 0/7] qapi and snapshot code clean up in block layer Wenchao Xia
` (4 preceding siblings ...)
2013-04-26 9:31 ` [Qemu-devel] [PATCH 5/7] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
@ 2013-04-26 9:31 ` Wenchao Xia
2013-04-30 17:50 ` Eric Blake
2013-04-26 9:31 ` [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
6 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-04-26 9:31 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This patch is a pure code move patch, except following modification:
1 get_human_readable_size() is changed to static function.
2 dump_human_image_info() is renamed to bdrv_image_info_dump().
3 in qmp_query_block() and qmp_query_blockstats, use bdrv_next(bs)
instead of direct traverse of global array 'bdrv_states'.
4 code style fix.
Now block.h and snapshot.h are at the same level in include path,
block_int.h and qapi.h will both include them.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block.c | 185 ---------------------------------
block/qapi.c | 253 +++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 10 --
include/block/block_int.h | 1 +
include/block/qapi.h | 6 +
qemu-img.c | 69 +------------
savevm.c | 1 +
7 files changed, 262 insertions(+), 263 deletions(-)
diff --git a/block.c b/block.c
index 06445c8..e7ae26e 100644
--- a/block.c
+++ b/block.c
@@ -3089,128 +3089,6 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
return data.ret;
}
-BlockInfo *bdrv_query_info(BlockDriverState *bs)
-{
- BlockInfo *info = g_malloc0(sizeof(*info));
- info->device = g_strdup(bs->device_name);
- info->type = g_strdup("unknown");
- info->locked = bdrv_dev_is_medium_locked(bs);
- info->removable = bdrv_dev_has_removable_media(bs);
-
- if (bdrv_dev_has_removable_media(bs)) {
- info->has_tray_open = true;
- info->tray_open = bdrv_dev_is_tray_open(bs);
- }
-
- if (bdrv_iostatus_is_enabled(bs)) {
- info->has_io_status = true;
- info->io_status = bs->iostatus;
- }
-
- if (bs->dirty_bitmap) {
- info->has_dirty = true;
- info->dirty = g_malloc0(sizeof(*info->dirty));
- info->dirty->count = bdrv_get_dirty_count(bs) * BDRV_SECTOR_SIZE;
- info->dirty->granularity =
- ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bs->dirty_bitmap));
- }
-
- if (bs->drv) {
- info->has_inserted = true;
- info->inserted = g_malloc0(sizeof(*info->inserted));
- info->inserted->file = g_strdup(bs->filename);
- info->inserted->ro = bs->read_only;
- info->inserted->drv = g_strdup(bs->drv->format_name);
- info->inserted->encrypted = bs->encrypted;
- info->inserted->encryption_key_missing = bdrv_key_required(bs);
-
- if (bs->backing_file[0]) {
- info->inserted->has_backing_file = true;
- info->inserted->backing_file = g_strdup(bs->backing_file);
- }
-
- info->inserted->backing_file_depth = bdrv_get_backing_file_depth(bs);
-
- if (bs->io_limits_enabled) {
- info->inserted->bps =
- bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL];
- info->inserted->bps_rd =
- bs->io_limits.bps[BLOCK_IO_LIMIT_READ];
- info->inserted->bps_wr =
- bs->io_limits.bps[BLOCK_IO_LIMIT_WRITE];
- info->inserted->iops =
- bs->io_limits.iops[BLOCK_IO_LIMIT_TOTAL];
- info->inserted->iops_rd =
- bs->io_limits.iops[BLOCK_IO_LIMIT_READ];
- info->inserted->iops_wr =
- bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
- }
- }
- return info;
-}
-
-BlockInfoList *qmp_query_block(Error **errp)
-{
- BlockInfoList *head = NULL, **p_next = &head;
- BlockDriverState *bs;
-
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
- BlockInfoList *info = g_malloc0(sizeof(*info));
- info->value = bdrv_query_info(bs);
-
- *p_next = info;
- p_next = &info->next;
- }
-
- return head;
-}
-
-BlockStats *bdrv_query_stats(const BlockDriverState *bs)
-{
- BlockStats *s;
-
- s = g_malloc0(sizeof(*s));
-
- if (bs->device_name[0]) {
- s->has_device = true;
- s->device = g_strdup(bs->device_name);
- }
-
- s->stats = g_malloc0(sizeof(*s->stats));
- s->stats->rd_bytes = bs->nr_bytes[BDRV_ACCT_READ];
- s->stats->wr_bytes = bs->nr_bytes[BDRV_ACCT_WRITE];
- s->stats->rd_operations = bs->nr_ops[BDRV_ACCT_READ];
- s->stats->wr_operations = bs->nr_ops[BDRV_ACCT_WRITE];
- s->stats->wr_highest_offset = bs->wr_highest_sector * BDRV_SECTOR_SIZE;
- s->stats->flush_operations = bs->nr_ops[BDRV_ACCT_FLUSH];
- s->stats->wr_total_time_ns = bs->total_time_ns[BDRV_ACCT_WRITE];
- s->stats->rd_total_time_ns = bs->total_time_ns[BDRV_ACCT_READ];
- s->stats->flush_total_time_ns = bs->total_time_ns[BDRV_ACCT_FLUSH];
-
- if (bs->file) {
- s->has_parent = true;
- s->parent = bdrv_query_stats(bs->file);
- }
-
- return s;
-}
-
-BlockStatsList *qmp_query_blockstats(Error **errp)
-{
- BlockStatsList *head = NULL, **p_next = &head;
- BlockDriverState *bs;
-
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
- BlockStatsList *info = g_malloc0(sizeof(*info));
- info->value = bdrv_query_stats(bs);
-
- *p_next = info;
- p_next = &info->next;
- }
-
- return head;
-}
-
const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
{
if (bs->backing_hd && bs->backing_hd->encrypted)
@@ -3441,69 +3319,6 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
return curr_bs;
}
-#define NB_SUFFIXES 4
-
-char *get_human_readable_size(char *buf, int buf_size, int64_t size)
-{
- static const char suffixes[NB_SUFFIXES] = "KMGT";
- int64_t base;
- int i;
-
- if (size <= 999) {
- snprintf(buf, buf_size, "%" PRId64, size);
- } else {
- base = 1024;
- for(i = 0; i < NB_SUFFIXES; i++) {
- if (size < (10 * base)) {
- snprintf(buf, buf_size, "%0.1f%c",
- (double)size / base,
- suffixes[i]);
- break;
- } else if (size < (1000 * base) || i == (NB_SUFFIXES - 1)) {
- snprintf(buf, buf_size, "%" PRId64 "%c",
- ((size + (base >> 1)) / base),
- suffixes[i]);
- break;
- }
- base = base * 1024;
- }
- }
- return buf;
-}
-
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
-{
- char buf1[128], date_buf[128], clock_buf[128];
- struct tm tm;
- time_t ti;
- int64_t secs;
-
- if (!sn) {
- snprintf(buf, buf_size,
- "%-10s%-20s%7s%20s%15s",
- "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
- } else {
- ti = sn->date_sec;
- localtime_r(&ti, &tm);
- strftime(date_buf, sizeof(date_buf),
- "%Y-%m-%d %H:%M:%S", &tm);
- secs = sn->vm_clock_nsec / 1000000000;
- snprintf(clock_buf, sizeof(clock_buf),
- "%02d:%02d:%02d.%03d",
- (int)(secs / 3600),
- (int)((secs / 60) % 60),
- (int)(secs % 60),
- (int)((sn->vm_clock_nsec / 1000000) % 1000));
- snprintf(buf, buf_size,
- "%-10s%-20s%7s%20s%15s",
- sn->id_str, sn->name,
- get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
- date_buf,
- clock_buf);
- }
- return buf;
-}
-
/**************************************************************/
/* async I/Os */
diff --git a/block/qapi.c b/block/qapi.c
index 0b57df2..155e77e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -24,6 +24,7 @@
#include "block/qapi.h"
#include "block/block_int.h"
+#include "qmp-commands.h"
void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info)
{
@@ -105,3 +106,255 @@ void bdrv_collect_image_info(BlockDriverState *bs,
}
}
}
+
+BlockInfo *bdrv_query_info(BlockDriverState *bs)
+{
+ BlockInfo *info = g_malloc0(sizeof(*info));
+ info->device = g_strdup(bs->device_name);
+ info->type = g_strdup("unknown");
+ info->locked = bdrv_dev_is_medium_locked(bs);
+ info->removable = bdrv_dev_has_removable_media(bs);
+
+ if (bdrv_dev_has_removable_media(bs)) {
+ info->has_tray_open = true;
+ info->tray_open = bdrv_dev_is_tray_open(bs);
+ }
+
+ if (bdrv_iostatus_is_enabled(bs)) {
+ info->has_io_status = true;
+ info->io_status = bs->iostatus;
+ }
+
+ if (bs->dirty_bitmap) {
+ info->has_dirty = true;
+ info->dirty = g_malloc0(sizeof(*info->dirty));
+ info->dirty->count = bdrv_get_dirty_count(bs) * BDRV_SECTOR_SIZE;
+ info->dirty->granularity =
+ ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bs->dirty_bitmap));
+ }
+
+ if (bs->drv) {
+ info->has_inserted = true;
+ info->inserted = g_malloc0(sizeof(*info->inserted));
+ info->inserted->file = g_strdup(bs->filename);
+ info->inserted->ro = bs->read_only;
+ info->inserted->drv = g_strdup(bs->drv->format_name);
+ info->inserted->encrypted = bs->encrypted;
+ info->inserted->encryption_key_missing = bdrv_key_required(bs);
+
+ if (bs->backing_file[0]) {
+ info->inserted->has_backing_file = true;
+ info->inserted->backing_file = g_strdup(bs->backing_file);
+ }
+
+ info->inserted->backing_file_depth = bdrv_get_backing_file_depth(bs);
+
+ if (bs->io_limits_enabled) {
+ info->inserted->bps =
+ bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL];
+ info->inserted->bps_rd =
+ bs->io_limits.bps[BLOCK_IO_LIMIT_READ];
+ info->inserted->bps_wr =
+ bs->io_limits.bps[BLOCK_IO_LIMIT_WRITE];
+ info->inserted->iops =
+ bs->io_limits.iops[BLOCK_IO_LIMIT_TOTAL];
+ info->inserted->iops_rd =
+ bs->io_limits.iops[BLOCK_IO_LIMIT_READ];
+ info->inserted->iops_wr =
+ bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
+ }
+ }
+ return info;
+}
+
+BlockStats *bdrv_query_stats(const BlockDriverState *bs)
+{
+ BlockStats *s;
+
+ s = g_malloc0(sizeof(*s));
+
+ if (bs->device_name[0]) {
+ s->has_device = true;
+ s->device = g_strdup(bs->device_name);
+ }
+
+ s->stats = g_malloc0(sizeof(*s->stats));
+ s->stats->rd_bytes = bs->nr_bytes[BDRV_ACCT_READ];
+ s->stats->wr_bytes = bs->nr_bytes[BDRV_ACCT_WRITE];
+ s->stats->rd_operations = bs->nr_ops[BDRV_ACCT_READ];
+ s->stats->wr_operations = bs->nr_ops[BDRV_ACCT_WRITE];
+ s->stats->wr_highest_offset = bs->wr_highest_sector * BDRV_SECTOR_SIZE;
+ s->stats->flush_operations = bs->nr_ops[BDRV_ACCT_FLUSH];
+ s->stats->wr_total_time_ns = bs->total_time_ns[BDRV_ACCT_WRITE];
+ s->stats->rd_total_time_ns = bs->total_time_ns[BDRV_ACCT_READ];
+ s->stats->flush_total_time_ns = bs->total_time_ns[BDRV_ACCT_FLUSH];
+
+ if (bs->file) {
+ s->has_parent = true;
+ s->parent = bdrv_query_stats(bs->file);
+ }
+
+ return s;
+}
+
+BlockInfoList *qmp_query_block(Error **errp)
+{
+ BlockInfoList *head = NULL, **p_next = &head;
+ BlockDriverState *bs = NULL;
+
+ while ((bs = bdrv_next(bs))) {
+ BlockInfoList *info = g_malloc0(sizeof(*info));
+ info->value = bdrv_query_info(bs);
+
+ *p_next = info;
+ p_next = &info->next;
+ }
+
+ return head;
+}
+
+BlockStatsList *qmp_query_blockstats(Error **errp)
+{
+ BlockStatsList *head = NULL, **p_next = &head;
+ BlockDriverState *bs = NULL;
+
+ while ((bs = bdrv_next(bs))) {
+ BlockStatsList *info = g_malloc0(sizeof(*info));
+ info->value = bdrv_query_stats(bs);
+
+ *p_next = info;
+ p_next = &info->next;
+ }
+
+ return head;
+}
+
+#define NB_SUFFIXES 4
+
+static char *get_human_readable_size(char *buf, int buf_size, int64_t size)
+{
+ static const char suffixes[NB_SUFFIXES] = "KMGT";
+ int64_t base;
+ int i;
+
+ if (size <= 999) {
+ snprintf(buf, buf_size, "%" PRId64, size);
+ } else {
+ base = 1024;
+ for (i = 0; i < NB_SUFFIXES; i++) {
+ if (size < (10 * base)) {
+ snprintf(buf, buf_size, "%0.1f%c",
+ (double)size / base,
+ suffixes[i]);
+ break;
+ } else if (size < (1000 * base) || i == (NB_SUFFIXES - 1)) {
+ snprintf(buf, buf_size, "%" PRId64 "%c",
+ ((size + (base >> 1)) / base),
+ suffixes[i]);
+ break;
+ }
+ base = base * 1024;
+ }
+ }
+ return buf;
+}
+
+char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
+{
+ char buf1[128], date_buf[128], clock_buf[128];
+ struct tm tm;
+ time_t ti;
+ int64_t secs;
+
+ if (!sn) {
+ snprintf(buf, buf_size,
+ "%-10s%-20s%7s%20s%15s",
+ "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
+ } else {
+ ti = sn->date_sec;
+ localtime_r(&ti, &tm);
+ strftime(date_buf, sizeof(date_buf),
+ "%Y-%m-%d %H:%M:%S", &tm);
+ secs = sn->vm_clock_nsec / 1000000000;
+ snprintf(clock_buf, sizeof(clock_buf),
+ "%02d:%02d:%02d.%03d",
+ (int)(secs / 3600),
+ (int)((secs / 60) % 60),
+ (int)(secs % 60),
+ (int)((sn->vm_clock_nsec / 1000000) % 1000));
+ snprintf(buf, buf_size,
+ "%-10s%-20s%7s%20s%15s",
+ sn->id_str, sn->name,
+ get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
+ date_buf,
+ clock_buf);
+ }
+ return buf;
+}
+
+void bdrv_image_info_dump(ImageInfo *info)
+{
+ char size_buf[128], dsize_buf[128];
+ if (!info->has_actual_size) {
+ snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
+ } else {
+ get_human_readable_size(dsize_buf, sizeof(dsize_buf),
+ info->actual_size);
+ }
+ get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
+ printf("image: %s\n"
+ "file format: %s\n"
+ "virtual size: %s (%" PRId64 " bytes)\n"
+ "disk size: %s\n",
+ info->filename, info->format, size_buf,
+ info->virtual_size,
+ dsize_buf);
+
+ if (info->has_encrypted && info->encrypted) {
+ printf("encrypted: yes\n");
+ }
+
+ if (info->has_cluster_size) {
+ printf("cluster_size: %" PRId64 "\n", info->cluster_size);
+ }
+
+ if (info->has_dirty_flag && info->dirty_flag) {
+ printf("cleanly shut down: no\n");
+ }
+
+ if (info->has_backing_filename) {
+ printf("backing file: %s", info->backing_filename);
+ if (info->has_full_backing_filename) {
+ printf(" (actual path: %s)", info->full_backing_filename);
+ }
+ putchar('\n');
+ if (info->has_backing_filename_format) {
+ printf("backing file format: %s\n", info->backing_filename_format);
+ }
+ }
+
+ if (info->has_snapshots) {
+ SnapshotInfoList *elem;
+ char buf[256];
+
+ printf("Snapshot list:\n");
+ printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+
+ /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
+ * we convert to the block layer's native QEMUSnapshotInfo for now.
+ */
+ for (elem = info->snapshots; elem; elem = elem->next) {
+ QEMUSnapshotInfo sn = {
+ .vm_state_size = elem->value->vm_state_size,
+ .date_sec = elem->value->date_sec,
+ .date_nsec = elem->value->date_nsec,
+ .vm_clock_nsec = elem->value->vm_clock_sec * 1000000000ULL +
+ elem->value->vm_clock_nsec,
+ };
+
+ pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
+ pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
+ printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+ }
+ }
+}
diff --git a/include/block/block.h b/include/block/block.h
index fde6fb8..35f7958 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -7,11 +7,6 @@
#include "block/coroutine.h"
#include "qapi/qmp/qobject.h"
#include "qapi-types.h"
-/*
- * snapshot.h is needed since bdrv_snapshot_dump(), it can be removed when the
- * function is moved to other file.
- */
-#include "block/snapshot.h"
/* block.c */
typedef struct BlockDriver BlockDriver;
@@ -322,12 +317,7 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);
void bdrv_get_full_backing_filename(BlockDriverState *bs,
char *dest, size_t sz);
-BlockInfo *bdrv_query_info(BlockDriverState *s);
-BlockStats *bdrv_query_stats(const BlockDriverState *bs);
-
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
-char *get_human_readable_size(char *buf, int buf_size, int64_t size);
int path_is_absolute(const char *path);
void path_combine(char *dest, int dest_size,
const char *base_path,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6078dd3..ba52247 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -33,6 +33,7 @@
#include "qapi/qmp/qerror.h"
#include "monitor/monitor.h"
#include "qemu/hbitmap.h"
+#include "block/snapshot.h"
#define BLOCK_FLAG_ENCRYPT 1
#define BLOCK_FLAG_COMPAT6 4
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 22a447f..55d1848 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -27,9 +27,15 @@
#include "qapi-types.h"
#include "block/block.h"
+#include "block/snapshot.h"
void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info);
void bdrv_collect_image_info(BlockDriverState *bs,
ImageInfo *info,
const char *filename);
+BlockInfo *bdrv_query_info(BlockDriverState *s);
+BlockStats *bdrv_query_stats(const BlockDriverState *bs);
+
+char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
+void bdrv_image_info_dump(ImageInfo *info);
#endif
diff --git a/qemu-img.c b/qemu-img.c
index c19ee58..5d1e480 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1602,73 +1602,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;
@@ -1680,7 +1613,7 @@ static void dump_human_image_info_list(ImageInfoList *list)
}
delim = true;
- dump_human_image_info(elem->value);
+ bdrv_image_info_dump(elem->value);
}
}
diff --git a/savevm.c b/savevm.c
index 5dd2d14..ac81c9b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -41,6 +41,7 @@
#include "qemu/bitops.h"
#include "qemu/iov.h"
#include "block/snapshot.h"
+#include "block/qapi.h"
#define SELF_ANNOUNCE_ROUNDS 5
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-26 9:31 [Qemu-devel] [PATCH 0/7] qapi and snapshot code clean up in block layer Wenchao Xia
` (5 preceding siblings ...)
2013-04-26 9:31 ` [Qemu-devel] [PATCH 6/7] block: move qmp and info dump related code " Wenchao Xia
@ 2013-04-26 9:31 ` Wenchao Xia
2013-04-26 14:46 ` Stefan Hajnoczi
6 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-04-26 9:31 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
This patch introduce a new print function, which will output message to
monitor when it present. With it, bdrv_snapshot_dump() need no more buffer
and can avoid string truncation, bdrv_image_info_dump() can also be used by
hmp code later, besides qemu-img code.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/qapi.c | 60 ++++++++++++++++++++++--------------------
include/block/qapi.h | 2 +-
include/qemu/error-report.h | 1 +
qemu-img.c | 7 +++--
savevm.c | 7 +++--
util/qemu-error.c | 18 +++++++++++++
6 files changed, 59 insertions(+), 36 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 155e77e..15c7656 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -259,7 +259,8 @@ static char *get_human_readable_size(char *buf, int buf_size, int64_t size)
return buf;
}
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
+/* Dump to monitor if it present, otherwise to stdout. */
+void bdrv_snapshot_dump(QEMUSnapshotInfo *sn)
{
char buf1[128], date_buf[128], clock_buf[128];
struct tm tm;
@@ -267,9 +268,8 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
int64_t secs;
if (!sn) {
- snprintf(buf, buf_size,
- "%-10s%-20s%7s%20s%15s",
- "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
+ message_printf("%-10s%-20s%7s%20s%15s",
+ "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
} else {
ti = sn->date_sec;
localtime_r(&ti, &tm);
@@ -282,16 +282,16 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
(int)((secs / 60) % 60),
(int)(secs % 60),
(int)((sn->vm_clock_nsec / 1000000) % 1000));
- snprintf(buf, buf_size,
- "%-10s%-20s%7s%20s%15s",
- sn->id_str, sn->name,
- get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
- date_buf,
- clock_buf);
+ message_printf("%-10s%-20s%7s%20s%15s",
+ sn->id_str, sn->name,
+ get_human_readable_size(buf1, sizeof(buf1),
+ sn->vm_state_size),
+ date_buf,
+ clock_buf);
}
- return buf;
}
+/* Dump to monitor if it present, otherwise to stdout. */
void bdrv_image_info_dump(ImageInfo *info)
{
char size_buf[128], dsize_buf[128];
@@ -302,43 +302,44 @@ 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);
+ message_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");
+ message_printf("encrypted: yes\n");
}
if (info->has_cluster_size) {
- printf("cluster_size: %" PRId64 "\n", info->cluster_size);
+ message_printf("cluster_size: %" PRId64 "\n", info->cluster_size);
}
if (info->has_dirty_flag && info->dirty_flag) {
- printf("cleanly shut down: no\n");
+ message_printf("cleanly shut down: no\n");
}
if (info->has_backing_filename) {
- printf("backing file: %s", info->backing_filename);
+ message_printf("backing file: %s", info->backing_filename);
if (info->has_full_backing_filename) {
- printf(" (actual path: %s)", info->full_backing_filename);
+ message_printf(" (actual path: %s)", info->full_backing_filename);
}
- putchar('\n');
+ message_printf("\n");
if (info->has_backing_filename_format) {
- printf("backing file format: %s\n", info->backing_filename_format);
+ message_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));
+ message_printf("Snapshot list:\n");
+ bdrv_snapshot_dump(NULL);
+ message_printf("\n");
/* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
* we convert to the block layer's native QEMUSnapshotInfo for now.
@@ -354,7 +355,8 @@ void bdrv_image_info_dump(ImageInfo *info)
pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+ bdrv_snapshot_dump(&sn);
+ message_printf("\n");
}
}
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 55d1848..d93652d 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -36,6 +36,6 @@ void bdrv_collect_image_info(BlockDriverState *bs,
BlockInfo *bdrv_query_info(BlockDriverState *s);
BlockStats *bdrv_query_stats(const BlockDriverState *bs);
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
+void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
void bdrv_image_info_dump(ImageInfo *info);
#endif
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index c902cc1..90341bd 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -40,4 +40,5 @@ void error_set_progname(const char *argv0);
void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
const char *error_get_progname(void);
+void message_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
#endif
diff --git a/qemu-img.c b/qemu-img.c
index 5d1e480..f732b9a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1554,16 +1554,17 @@ static void dump_snapshots(BlockDriverState *bs)
{
QEMUSnapshotInfo *sn_tab, *sn;
int nb_sns, i;
- char buf[256];
nb_sns = bdrv_snapshot_list(bs, &sn_tab);
if (nb_sns <= 0)
return;
printf("Snapshot list:\n");
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ bdrv_snapshot_dump(NULL);
+ printf("\n");
for(i = 0; i < nb_sns; i++) {
sn = &sn_tab[i];
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+ bdrv_snapshot_dump(sn);
+ printf("\n");
}
g_free(sn_tab);
}
diff --git a/savevm.c b/savevm.c
index ac81c9b..c00ba00 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2542,7 +2542,6 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
int nb_sns, i, available;
int total;
int *available_snapshots;
- char buf[256];
bs = find_vmstate_bs();
if (!bs) {
@@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
}
if (total > 0) {
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ bdrv_snapshot_dump(NULL);
+ monitor_printf(mon, "\n");
for (i = 0; i < total; i++) {
sn = &sn_tab[available_snapshots[i]];
- monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+ bdrv_snapshot_dump(sn);
+ monitor_printf(mon, "\n");
}
} else {
monitor_printf(mon, "There is no suitable snapshot available\n");
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 08a36f4..a47bf32 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -213,3 +213,21 @@ void error_report(const char *fmt, ...)
va_end(ap);
error_printf("\n");
}
+
+/*
+ * Print to current monitor if we have one, else to stdout. It is similar with
+ * error_printf().
+ * TODO just like error_vprintf()
+ */
+void message_printf(const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ if (cur_mon) {
+ monitor_vprintf(cur_mon, fmt, ap);
+ } else {
+ vfprintf(stdout, fmt, ap);
+ }
+ va_end(ap);
+}
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()
2013-04-26 9:31 ` [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
@ 2013-04-26 14:34 ` Stefan Hajnoczi
2013-04-26 14:47 ` Eric Blake
2013-04-27 3:34 ` Wenchao Xia
2013-04-30 18:16 ` Eric Blake
1 sibling, 2 replies; 34+ messages in thread
From: Stefan Hajnoczi @ 2013-04-26 14:34 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, armbru, qemu-devel, lcapitulino, pbonzini
On Fri, Apr 26, 2013 at 05:31:12PM +0800, Wenchao Xia wrote:
> 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.
> If not found, *errp will be set to tip why.
>
> Note that the caller logic is changed a bit:
> 1) In del_existing_snapshots() called by do_savevm(), it travers twice
s/travers/traverse/
Also in comments in the code.
> to find the snapshot, instead once, so matching sequence may change
> if there are unwisely chosen, mixed id and names.
> 2) In do_savevm(), same with del_existing_snapshot(), when it tries to
> find the snapshot to overwrite, matching sequence may change for same
> reason.
> 3) In load_vmstate(), first when it tries to find the snapshot to be loaded,
> sequence may change for the same reason of above. Later in validation, the
> logic is changed to be more strict to require both id and name matching.
> 4) In do_info_snapshot(), in validation, the logic is changed to be more
> strict to require both id and name matching.
It's easy to avoid changing semantics: keep the old name or id behavior
around. Use the new name-and-id behavior for #3 and #4.
Please include a justification for breaking the search order.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-26 9:31 ` [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
@ 2013-04-26 14:46 ` Stefan Hajnoczi
2013-04-27 3:37 ` Wenchao Xia
2013-04-29 19:05 ` Luiz Capitulino
0 siblings, 2 replies; 34+ messages in thread
From: Stefan Hajnoczi @ 2013-04-26 14:46 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, armbru, qemu-devel, lcapitulino, pbonzini
On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> }
>
> if (total > 0) {
> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> + bdrv_snapshot_dump(NULL);
> + monitor_printf(mon, "\n");
Luiz: any issue with mixing monitor_printf(mon) and
monitor_vprintf(cur_mon) calls? I guess there was a reason for
explicitly passing mon instead of relying on cur_mon.
> for (i = 0; i < total; i++) {
> sn = &sn_tab[available_snapshots[i]];
> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
> + bdrv_snapshot_dump(sn);
> + monitor_printf(mon, "\n");
> }
> } else {
> monitor_printf(mon, "There is no suitable snapshot available\n");
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index 08a36f4..a47bf32 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -213,3 +213,21 @@ void error_report(const char *fmt, ...)
> va_end(ap);
> error_printf("\n");
> }
> +
> +/*
> + * Print to current monitor if we have one, else to stdout. It is similar with
> + * error_printf().
> + * TODO just like error_vprintf()
TODO?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()
2013-04-26 14:34 ` Stefan Hajnoczi
@ 2013-04-26 14:47 ` Eric Blake
2013-04-27 3:34 ` Wenchao Xia
1 sibling, 0 replies; 34+ messages in thread
From: Eric Blake @ 2013-04-26 14:47 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: kwolf, phrdina, qemu-devel, armbru, lcapitulino, pbonzini, Wenchao Xia
[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]
On 04/26/2013 08:34 AM, Stefan Hajnoczi wrote:
> On Fri, Apr 26, 2013 at 05:31:12PM +0800, Wenchao Xia wrote:
>> 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.
>> If not found, *errp will be set to tip why.
>>
>> Note that the caller logic is changed a bit:
>> 1) In del_existing_snapshots() called by do_savevm(), it travers twice
>
> s/travers/traverse/
>
> Also in comments in the code.
>
>> to find the snapshot, instead once, so matching sequence may change
>> if there are unwisely chosen, mixed id and names.
>> 2) In do_savevm(), same with del_existing_snapshot(), when it tries to
>> find the snapshot to overwrite, matching sequence may change for same
>> reason.
>> 3) In load_vmstate(), first when it tries to find the snapshot to be loaded,
>> sequence may change for the same reason of above. Later in validation, the
>> logic is changed to be more strict to require both id and name matching.
>> 4) In do_info_snapshot(), in validation, the logic is changed to be more
>> strict to require both id and name matching.
>
> It's easy to avoid changing semantics: keep the old name or id behavior
> around. Use the new name-and-id behavior for #3 and #4.
>
> Please include a justification for breaking the search order.
A good start for such a justification would be to edit parts of my email
[1] into your commit message, showing how the old semantics make it
IMPOSSIBLE to delete or load 'id 2 tag 1' without first getting 'id 1
tag 2' out of the way, assuming that a qcow2 file with poor naming
conventions exists (even if such a bad file can only be created
externally). The goal of a commit message is to convince the readers
that a change in semantics is appropriate because the alternative of
leaving things broken is worse.
[1]https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03501.html
--
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] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] block: move snapshot code in block.c to block/snapshot.c
2013-04-26 9:31 ` [Qemu-devel] [PATCH 3/7] block: move snapshot code in block.c " Wenchao Xia
@ 2013-04-26 19:05 ` Eric Blake
0 siblings, 0 replies; 34+ messages in thread
From: Eric Blake @ 2013-04-26 19:05 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, armbru, qemu-devel, lcapitulino, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 808 bytes --]
On 04/26/2013 03:31 AM, Wenchao Xia wrote:
> All snapshot related code, except bdrv_snapshot_dump(), is moved to
> block/snapshot.c. bdrv_snapshot_dump() will be moved to another file later.
> It also fixes small code style errors reported by check script.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block.c | 105 ------------------------------------------
> block/snapshot.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 28 ++---------
> include/block/snapshot.h | 27 +++++++++--
> 4 files changed, 142 insertions(+), 132 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] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()
2013-04-26 14:34 ` Stefan Hajnoczi
2013-04-26 14:47 ` Eric Blake
@ 2013-04-27 3:34 ` Wenchao Xia
2013-04-30 17:52 ` Eric Blake
1 sibling, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-04-27 3:34 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, phrdina, qemu-devel, armbru, lcapitulino, pbonzini
于 2013-4-26 22:34, Stefan Hajnoczi 写道:
> On Fri, Apr 26, 2013 at 05:31:12PM +0800, Wenchao Xia wrote:
>> 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.
>> If not found, *errp will be set to tip why.
>>
>> Note that the caller logic is changed a bit:
>> 1) In del_existing_snapshots() called by do_savevm(), it travers twice
>
> s/travers/traverse/
>
> Also in comments in the code.
>
OK.
>> to find the snapshot, instead once, so matching sequence may change
>> if there are unwisely chosen, mixed id and names.
>> 2) In do_savevm(), same with del_existing_snapshot(), when it tries to
>> find the snapshot to overwrite, matching sequence may change for same
>> reason.
>> 3) In load_vmstate(), first when it tries to find the snapshot to be loaded,
>> sequence may change for the same reason of above. Later in validation, the
>> logic is changed to be more strict to require both id and name matching.
>> 4) In do_info_snapshot(), in validation, the logic is changed to be more
>> strict to require both id and name matching.
>
> It's easy to avoid changing semantics: keep the old name or id behavior
> around. Use the new name-and-id behavior for #3 and #4.
You mean adding a new function, instead of change
bdrv_find_snapshot()?
>
> Please include a justification for breaking the search order.
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-26 14:46 ` Stefan Hajnoczi
@ 2013-04-27 3:37 ` Wenchao Xia
2013-04-29 19:05 ` Luiz Capitulino
1 sibling, 0 replies; 34+ messages in thread
From: Wenchao Xia @ 2013-04-27 3:37 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, phrdina, armbru, qemu-devel, lcapitulino, pbonzini
于 2013-4-26 22:46, Stefan Hajnoczi 写道:
> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>> }
>>
>> if (total > 0) {
>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
>> + bdrv_snapshot_dump(NULL);
>> + monitor_printf(mon, "\n");
>
> Luiz: any issue with mixing monitor_printf(mon) and
> monitor_vprintf(cur_mon) calls? I guess there was a reason for
> explicitly passing mon instead of relying on cur_mon.
>
>> for (i = 0; i < total; i++) {
>> sn = &sn_tab[available_snapshots[i]];
>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
>> + bdrv_snapshot_dump(sn);
>> + monitor_printf(mon, "\n");
>> }
>> } else {
>> monitor_printf(mon, "There is no suitable snapshot available\n");
>> diff --git a/util/qemu-error.c b/util/qemu-error.c
>> index 08a36f4..a47bf32 100644
>> --- a/util/qemu-error.c
>> +++ b/util/qemu-error.c
>> @@ -213,3 +213,21 @@ void error_report(const char *fmt, ...)
>> va_end(ap);
>> error_printf("\n");
>> }
>> +
>> +/*
>> + * Print to current monitor if we have one, else to stdout. It is similar with
>> + * error_printf().
>> + * TODO just like error_vprintf()
>
> TODO?
>
Same with error_vprintf's comments:
/*
* Print to current monitor if we have one, else to stderr.
* TODO should return int, so callers can calculate width, but that
* requires surgery to monitor_vprintf(). Left for another day.
*/
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-26 14:46 ` Stefan Hajnoczi
2013-04-27 3:37 ` Wenchao Xia
@ 2013-04-29 19:05 ` Luiz Capitulino
2013-05-02 2:05 ` Wenchao Xia
1 sibling, 1 reply; 34+ messages in thread
From: Luiz Capitulino @ 2013-04-29 19:05 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kwolf, phrdina, armbru, qemu-devel, pbonzini, Wenchao Xia
On Fri, 26 Apr 2013 16:46:57 +0200
Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
> > @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> > }
> >
> > if (total > 0) {
> > - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> > + bdrv_snapshot_dump(NULL);
> > + monitor_printf(mon, "\n");
>
> Luiz: any issue with mixing monitor_printf(mon) and
> monitor_vprintf(cur_mon) calls? I guess there was a reason for
> explicitly passing mon instead of relying on cur_mon.
where are they being mixed?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] block: move qmp and info dump related code to block/qapi.c
2013-04-26 9:31 ` [Qemu-devel] [PATCH 6/7] block: move qmp and info dump related code " Wenchao Xia
@ 2013-04-30 17:50 ` Eric Blake
0 siblings, 0 replies; 34+ messages in thread
From: Eric Blake @ 2013-04-30 17:50 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, armbru, qemu-devel, lcapitulino, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]
On 04/26/2013 03:31 AM, Wenchao Xia wrote:
> This patch is a pure code move patch, except following modification:
> 1 get_human_readable_size() is changed to static function.
> 2 dump_human_image_info() is renamed to bdrv_image_info_dump().
> 3 in qmp_query_block() and qmp_query_blockstats, use bdrv_next(bs)
> instead of direct traverse of global array 'bdrv_states'.
> 4 code style fix.
>
> Now block.h and snapshot.h are at the same level in include path,
> block_int.h and qapi.h will both include them.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block.c | 185 ---------------------------------
> block/qapi.c | 253 +++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 10 --
> include/block/block_int.h | 1 +
> include/block/qapi.h | 6 +
> qemu-img.c | 69 +------------
> savevm.c | 1 +
> 7 files changed, 262 insertions(+), 263 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] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()
2013-04-27 3:34 ` Wenchao Xia
@ 2013-04-30 17:52 ` Eric Blake
0 siblings, 0 replies; 34+ messages in thread
From: Eric Blake @ 2013-04-30 17:52 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, lcapitulino,
pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]
On 04/26/2013 09:34 PM, Wenchao Xia wrote:
>>> to find the snapshot, instead once, so matching sequence may change
>>> if there are unwisely chosen, mixed id and names.
>>> 2) In do_savevm(), same with del_existing_snapshot(), when it tries to
>>> find the snapshot to overwrite, matching sequence may change for same
>>> reason.
>>> 3) In load_vmstate(), first when it tries to find the snapshot to be
>>> loaded,
>>> sequence may change for the same reason of above. Later in
>>> validation, the
>>> logic is changed to be more strict to require both id and name matching.
>>> 4) In do_info_snapshot(), in validation, the logic is changed to be more
>>> strict to require both id and name matching.
>>
>> It's easy to avoid changing semantics: keep the old name or id behavior
>> around. Use the new name-and-id behavior for #3 and #4.
> You mean adding a new function, instead of change
> bdrv_find_snapshot()?
That's certainly an option. Although after Pavel's proposed series for
adding QMP counterpart to savevm, there are no longer any uses of the
old semantics.
--
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] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()
2013-04-26 9:31 ` [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
2013-04-26 14:34 ` Stefan Hajnoczi
@ 2013-04-30 18:16 ` Eric Blake
2013-05-02 2:02 ` Wenchao Xia
1 sibling, 1 reply; 34+ messages in thread
From: Eric Blake @ 2013-04-30 18:16 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, armbru, qemu-devel, lcapitulino, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 7839 bytes --]
On 04/26/2013 03:31 AM, Wenchao Xia wrote:
> 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.
> If not found, *errp will be set to tip why.
>
> Note that the caller logic is changed a bit:
> 1) In del_existing_snapshots() called by do_savevm(), it travers twice
> to find the snapshot, instead once, so matching sequence may change
> if there are unwisely chosen, mixed id and names.
> 2) In do_savevm(), same with del_existing_snapshot(), when it tries to
> find the snapshot to overwrite, matching sequence may change for same
> reason.
> 3) In load_vmstate(), first when it tries to find the snapshot to be loaded,
> sequence may change for the same reason of above. Later in validation, the
> logic is changed to be more strict to require both id and name matching.
> 4) In do_info_snapshot(), in validation, the logic is changed to be more
> strict to require both id and name matching.
>
> Savevm, loadvm logic may need to be improved later, to avoid mixing of them.
>
> Some code is borrowed from Pavel's patch.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
> block/snapshot.c | 72 +++++++++++++++++++++++++++++++++++++++-------
> include/block/snapshot.h | 5 ++-
> savevm.c | 35 ++++++++++++----------
> 3 files changed, 83 insertions(+), 29 deletions(-)
> + *
> + * Returns: true when a snapshot is found and @sn_info will be filled, false
> + * when error or not found with @errp filled if errp != NULL.
> + */
> +bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> + const char *id, const char *name, Error **errp)
Unusual convention to have (input, output, input, input, output)
parameters; as long as you are changing the signature, I'd consider
putting all input parameters (bs, id, name) firs, then output parameters
last (sn_info, errp).
> {
> QEMUSnapshotInfo *sn_tab, *sn;
> - int nb_sns, i, ret;
> + int nb_sns, i;
> + bool ret = false;
>
> - ret = -ENOENT;
> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> if (nb_sns < 0) {
> - return ret;
> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
> + return false;
> + } else if (nb_sns == 0) {
> + error_setg(errp, "Device has no snapshots");
> + return false;
> }
> - 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;
> +
No assertion that at least one of id or name is provided,...
> +
> + if (id && name) {
> + for (i = 0; i < nb_sns; i++) {
> + sn = &sn_tab[i];
> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + ret = true;
> + break;
> + }
> + }
> + } else if (id) {
> + for (i = 0; i < nb_sns; i++) {
> + sn = &sn_tab[i];
> + if (!strcmp(sn->id_str, id)) {
> + *sn_info = *sn;
> + ret = true;
> + break;
> + }
> + }
> + } else if (name) {
> + for (i = 0; i < nb_sns; i++) {
> + sn = &sn_tab[i];
> + if (!strcmp(sn->name, name)) {
> + *sn_info = *sn;
> + ret = true;
> + break;
> + }
> }
> }
> +
> + if (!ret) {
> + error_setg(errp, "Device have no matching snapshot");
> + }
...therefore, if I call bdrv_snapshot_find(bs, &info, NULL, NULL, errp),
I'll get this error. Seems okay.
> +++ b/savevm.c
> @@ -2286,8 +2286,8 @@ 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, NULL, NULL) ||
> + bdrv_snapshot_find(bs, snapshot, NULL, name, NULL))) {
This does an id lookup first, and falls back to a name lookup. Is that
what we want? Consider an image with the following snapshots:
id 1 name 2
id 2 name 3
id 3 name 1
id 4 name 5
Pre-patch, find(1) gives id 1, find(2) gives id 1, find(3) gives id 2,
find(4) gives id 4, find(5) gives id 4; no way to get id 3. Post-patch,
find(1,NULL) gives id 1, find(2,NULL) gives id 2, find(3,NULL) gives id
3, find(4,NULL) gives id 4, find(5,NULL) fails and you fall back to
find(NULL,5) to give id 4. Thus, it only makes a difference for
snapshots whose name is a numeric string that also matches an id, where
your change now favors the id lookup over the entire set instead of the
first name or id match while doing a single pass over the set.
Pavel's series on top of this would change the code to favor a name-only
lookup, or an explicit HMP option to do an id-only lookup, instead of
this code's double lookup.
At this point, I'm okay with the semantics of this patch (especially
since we may be cleaning it up further in Pavel's patch series), but it
deserves explicit documentation in the commit message on what semantics
are changing (favoring id more strongly) and why (so that we can select
all possible snapshots, instead of being unable to select snapshots
whose id was claimed as a name of an earlier snapshot).
> @@ -2437,12 +2437,14 @@ int load_vmstate(const char *name)
> @@ -2461,11 +2463,11 @@ int load_vmstate(const char *name)
> return -ENOTSUP;
> }
>
> - ret = bdrv_snapshot_find(bs, &sn, name);
> - if (ret < 0) {
> + /* vm snapshot will always have same id and name, check do_savevm(). */
> + if (!bdrv_snapshot_find(bs, &sn, sn.id_str, sn.name, NULL)) {
> error_report("Device '%s' does not have the requested snapshot '%s'",
> bdrv_get_device_name(bs), name);
> - return ret;
> + return -ENOENT;
> }
Are we 100% sure that a given snapshot name has the same id across all
block devices? Or is it possible to have:
disk a: [id 1 name A, id 2 name B]
disk b: [id 1 name B]
where it is possible to load snapshot [B] and get consistent state? If
it is possible to have non-matched ids across same-name snapshots, then
looking up by requiring a match of both id and name will fail, whereas
the pre-patch code would succeed.
> }
>
> @@ -2536,7 +2538,7 @@ 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 nb_sns, i, available;
> int total;
> int *available_snapshots;
> char buf[256];
> @@ -2567,8 +2569,9 @@ 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);
> - if (ret < 0) {
> + /* vm snapshot will always have same id and name */
> + if (!bdrv_snapshot_find(bs1, sn_info,
> + sn->id_str, sn->name, NULL)) {
Again, is this true, or are you needlessly filtering out snapshots that
have a consistent name but non-matching ids?
--
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] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()
2013-04-30 18:16 ` Eric Blake
@ 2013-05-02 2:02 ` Wenchao Xia
2013-05-02 21:12 ` Pavel Hrdina
0 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-05-02 2:02 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, phrdina, armbru, qemu-devel, lcapitulino, stefanha, pbonzini
于 2013-5-1 2:16, Eric Blake 写道:
> On 04/26/2013 03:31 AM, Wenchao Xia wrote:
>> 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.
>> If not found, *errp will be set to tip why.
>>
>> Note that the caller logic is changed a bit:
>> 1) In del_existing_snapshots() called by do_savevm(), it travers twice
>> to find the snapshot, instead once, so matching sequence may change
>> if there are unwisely chosen, mixed id and names.
>> 2) In do_savevm(), same with del_existing_snapshot(), when it tries to
>> find the snapshot to overwrite, matching sequence may change for same
>> reason.
>> 3) In load_vmstate(), first when it tries to find the snapshot to be loaded,
>> sequence may change for the same reason of above. Later in validation, the
>> logic is changed to be more strict to require both id and name matching.
>> 4) In do_info_snapshot(), in validation, the logic is changed to be more
>> strict to require both id and name matching.
>>
>> Savevm, loadvm logic may need to be improved later, to avoid mixing of them.
>>
>> Some code is borrowed from Pavel's patch.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>> block/snapshot.c | 72 +++++++++++++++++++++++++++++++++++++++-------
>> include/block/snapshot.h | 5 ++-
>> savevm.c | 35 ++++++++++++----------
>> 3 files changed, 83 insertions(+), 29 deletions(-)
>
>> + *
>> + * Returns: true when a snapshot is found and @sn_info will be filled, false
>> + * when error or not found with @errp filled if errp != NULL.
>> + */
>> +bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>> + const char *id, const char *name, Error **errp)
>
> Unusual convention to have (input, output, input, input, output)
> parameters; as long as you are changing the signature, I'd consider
> putting all input parameters (bs, id, name) firs, then output parameters
> last (sn_info, errp).
>
>> {
>> QEMUSnapshotInfo *sn_tab, *sn;
>> - int nb_sns, i, ret;
>> + int nb_sns, i;
>> + bool ret = false;
>>
>> - ret = -ENOENT;
>> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> if (nb_sns < 0) {
>> - return ret;
>> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
>> + return false;
>> + } else if (nb_sns == 0) {
>> + error_setg(errp, "Device has no snapshots");
>> + return false;
>> }
>> - 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;
>> +
>
> No assertion that at least one of id or name is provided,...
>
>> +
>> + if (id && name) {
>> + for (i = 0; i < nb_sns; i++) {
>> + sn = &sn_tab[i];
>> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + ret = true;
>> + break;
>> + }
>> + }
>> + } else if (id) {
>> + for (i = 0; i < nb_sns; i++) {
>> + sn = &sn_tab[i];
>> + if (!strcmp(sn->id_str, id)) {
>> + *sn_info = *sn;
>> + ret = true;
>> + break;
>> + }
>> + }
>> + } else if (name) {
>> + for (i = 0; i < nb_sns; i++) {
>> + sn = &sn_tab[i];
>> + if (!strcmp(sn->name, name)) {
>> + *sn_info = *sn;
>> + ret = true;
>> + break;
>> + }
>> }
>> }
>> +
>> + if (!ret) {
>> + error_setg(errp, "Device have no matching snapshot");
>> + }
>
> ...therefore, if I call bdrv_snapshot_find(bs, &info, NULL, NULL, errp),
> I'll get this error. Seems okay.
>
>> +++ b/savevm.c
>> @@ -2286,8 +2286,8 @@ 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, NULL, NULL) ||
>> + bdrv_snapshot_find(bs, snapshot, NULL, name, NULL))) {
>
> This does an id lookup first, and falls back to a name lookup. Is that
> what we want? Consider an image with the following snapshots:
>
> id 1 name 2
> id 2 name 3
> id 3 name 1
> id 4 name 5
>
> Pre-patch, find(1) gives id 1, find(2) gives id 1, find(3) gives id 2,
> find(4) gives id 4, find(5) gives id 4; no way to get id 3. Post-patch,
> find(1,NULL) gives id 1, find(2,NULL) gives id 2, find(3,NULL) gives id
> 3, find(4,NULL) gives id 4, find(5,NULL) fails and you fall back to
> find(NULL,5) to give id 4. Thus, it only makes a difference for
> snapshots whose name is a numeric string that also matches an id, where
> your change now favors the id lookup over the entire set instead of the
> first name or id match while doing a single pass over the set.
>
> Pavel's series on top of this would change the code to favor a name-only
> lookup, or an explicit HMP option to do an id-only lookup, instead of
> this code's double lookup.
>
> At this point, I'm okay with the semantics of this patch (especially
> since we may be cleaning it up further in Pavel's patch series), but it
> deserves explicit documentation in the commit message on what semantics
> are changing (favoring id more strongly) and why (so that we can select
> all possible snapshots, instead of being unable to select snapshots
> whose id was claimed as a name of an earlier snapshot).
>
To avoid trouble, I think a new function named
bdrv_snapshot_find_by_id_and_name() is better. Later Pavel
can directly call this new function, and after that we can
delete original bdrv_snapshot_find(). Pavel, what do you
think?
>> @@ -2437,12 +2437,14 @@ int load_vmstate(const char *name)
>> @@ -2461,11 +2463,11 @@ int load_vmstate(const char *name)
>> return -ENOTSUP;
>> }
>>
>> - ret = bdrv_snapshot_find(bs, &sn, name);
>> - if (ret < 0) {
>> + /* vm snapshot will always have same id and name, check do_savevm(). */
>> + if (!bdrv_snapshot_find(bs, &sn, sn.id_str, sn.name, NULL)) {
>> error_report("Device '%s' does not have the requested snapshot '%s'",
>> bdrv_get_device_name(bs), name);
>> - return ret;
>> + return -ENOENT;
>> }
>
> Are we 100% sure that a given snapshot name has the same id across all
> block devices? Or is it possible to have:
>
> disk a: [id 1 name A, id 2 name B]
> disk b: [id 1 name B]
>
> where it is possible to load snapshot [B] and get consistent state? If
> it is possible to have non-matched ids across same-name snapshots, then
> looking up by requiring a match of both id and name will fail, whereas
> the pre-patch code would succeed.
>
not possible, I checked the existing code, a loadable snapshot, that
is the one with vmstate, will always have same id and name, see savevm
logic and qcow2's snapshot creation code.
What changes is: previous, in "info snapshot", it will try show some
snapshot that may have the situation you described above, which may be
brought by qemu-img or hotplug operation, and which can't be loaded in
"loadvm". Now it will be filtered out.
Maybe there are more complicated case, but I think let management
stack handling it, is a better option.
>> }
>>
>> @@ -2536,7 +2538,7 @@ 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 nb_sns, i, available;
>> int total;
>> int *available_snapshots;
>> char buf[256];
>> @@ -2567,8 +2569,9 @@ 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);
>> - if (ret < 0) {
>> + /* vm snapshot will always have same id and name */
>> + if (!bdrv_snapshot_find(bs1, sn_info,
>> + sn->id_str, sn->name, NULL)) {
>
> Again, is this true, or are you needlessly filtering out snapshots that
> have a consistent name but non-matching ids?
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-04-29 19:05 ` Luiz Capitulino
@ 2013-05-02 2:05 ` Wenchao Xia
2013-05-02 12:02 ` Luiz Capitulino
0 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-05-02 2:05 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
于 2013-4-30 3:05, Luiz Capitulino 写道:
> On Fri, 26 Apr 2013 16:46:57 +0200
> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>>> }
>>>
>>> if (total > 0) {
>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
>>> + bdrv_snapshot_dump(NULL);
>>> + monitor_printf(mon, "\n");
>>
>> Luiz: any issue with mixing monitor_printf(mon) and
>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
>> explicitly passing mon instead of relying on cur_mon.
>
> where are they being mixed?
>
bdrv_snapshot_dump() used a global variable "cur_mon" inside, instead
of let caller pass in a explicit montior* "mon", I guess that is the
question.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-02 2:05 ` Wenchao Xia
@ 2013-05-02 12:02 ` Luiz Capitulino
2013-05-03 2:51 ` Wenchao Xia
0 siblings, 1 reply; 34+ messages in thread
From: Luiz Capitulino @ 2013-05-02 12:02 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
On Thu, 02 May 2013 10:05:08 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2013-4-30 3:05, Luiz Capitulino 写道:
> > On Fri, 26 Apr 2013 16:46:57 +0200
> > Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >
> >> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
> >>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> >>> }
> >>>
> >>> if (total > 0) {
> >>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> >>> + bdrv_snapshot_dump(NULL);
> >>> + monitor_printf(mon, "\n");
> >>
> >> Luiz: any issue with mixing monitor_printf(mon) and
> >> monitor_vprintf(cur_mon) calls? I guess there was a reason for
> >> explicitly passing mon instead of relying on cur_mon.
> >
> > where are they being mixed?
> >
> bdrv_snapshot_dump() used a global variable "cur_mon" inside, instead
> of let caller pass in a explicit montior* "mon", I guess that is the
> question.
I'd have to see the code to tell, but yes, what Stefan described is the
best practice for the Monitor.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()
2013-05-02 2:02 ` Wenchao Xia
@ 2013-05-02 21:12 ` Pavel Hrdina
0 siblings, 0 replies; 34+ messages in thread
From: Pavel Hrdina @ 2013-05-02 21:12 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, stefanha, qemu-devel, armbru, pbonzini, lcapitulino
On 2.5.2013 04:02, Wenchao Xia wrote:
> 于 2013-5-1 2:16, Eric Blake 写道:
>> On 04/26/2013 03:31 AM, Wenchao Xia wrote:
>>> 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.
>>> If not found, *errp will be set to tip why.
>>>
>>> Note that the caller logic is changed a bit:
>>> 1) In del_existing_snapshots() called by do_savevm(), it travers twice
>>> to find the snapshot, instead once, so matching sequence may change
>>> if there are unwisely chosen, mixed id and names.
>>> 2) In do_savevm(), same with del_existing_snapshot(), when it tries to
>>> find the snapshot to overwrite, matching sequence may change for same
>>> reason.
>>> 3) In load_vmstate(), first when it tries to find the snapshot to be
>>> loaded,
>>> sequence may change for the same reason of above. Later in
>>> validation, the
>>> logic is changed to be more strict to require both id and name matching.
>>> 4) In do_info_snapshot(), in validation, the logic is changed to be more
>>> strict to require both id and name matching.
>>>
>>> Savevm, loadvm logic may need to be improved later, to avoid mixing
>>> of them.
>>>
>>> Some code is borrowed from Pavel's patch.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>> block/snapshot.c | 72
>>> +++++++++++++++++++++++++++++++++++++++-------
>>> include/block/snapshot.h | 5 ++-
>>> savevm.c | 35 ++++++++++++----------
>>> 3 files changed, 83 insertions(+), 29 deletions(-)
>>
>>> + *
>>> + * Returns: true when a snapshot is found and @sn_info will be
>>> filled, false
>>> + * when error or not found with @errp filled if errp != NULL.
>>> + */
>>> +bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo
>>> *sn_info,
>>> + const char *id, const char *name, Error **errp)
>>
>> Unusual convention to have (input, output, input, input, output)
>> parameters; as long as you are changing the signature, I'd consider
>> putting all input parameters (bs, id, name) firs, then output parameters
>> last (sn_info, errp).
>>
>>> {
>>> QEMUSnapshotInfo *sn_tab, *sn;
>>> - int nb_sns, i, ret;
>>> + int nb_sns, i;
>>> + bool ret = false;
>>>
>>> - ret = -ENOENT;
>>> nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>>> if (nb_sns < 0) {
>>> - return ret;
>>> + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot
>>> list");
>>> + return false;
>>> + } else if (nb_sns == 0) {
>>> + error_setg(errp, "Device has no snapshots");
>>> + return false;
>>> }
>>> - 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;
>>> +
>>
>> No assertion that at least one of id or name is provided,...
>>
>>> +
>>> + if (id && name) {
>>> + for (i = 0; i < nb_sns; i++) {
>>> + sn = &sn_tab[i];
>>> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>>> + *sn_info = *sn;
>>> + ret = true;
>>> + break;
>>> + }
>>> + }
>>> + } else if (id) {
>>> + for (i = 0; i < nb_sns; i++) {
>>> + sn = &sn_tab[i];
>>> + if (!strcmp(sn->id_str, id)) {
>>> + *sn_info = *sn;
>>> + ret = true;
>>> + break;
>>> + }
>>> + }
>>> + } else if (name) {
>>> + for (i = 0; i < nb_sns; i++) {
>>> + sn = &sn_tab[i];
>>> + if (!strcmp(sn->name, name)) {
>>> + *sn_info = *sn;
>>> + ret = true;
>>> + break;
>>> + }
>>> }
>>> }
>>> +
>>> + if (!ret) {
>>> + error_setg(errp, "Device have no matching snapshot");
>>> + }
>>
>> ...therefore, if I call bdrv_snapshot_find(bs, &info, NULL, NULL, errp),
>> I'll get this error. Seems okay.
>>
>>> +++ b/savevm.c
>>> @@ -2286,8 +2286,8 @@ 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, NULL, NULL) ||
>>> + bdrv_snapshot_find(bs, snapshot, NULL, name, NULL))) {
>>
>> This does an id lookup first, and falls back to a name lookup. Is that
>> what we want? Consider an image with the following snapshots:
>>
>> id 1 name 2
>> id 2 name 3
>> id 3 name 1
>> id 4 name 5
>>
>> Pre-patch, find(1) gives id 1, find(2) gives id 1, find(3) gives id 2,
>> find(4) gives id 4, find(5) gives id 4; no way to get id 3. Post-patch,
>> find(1,NULL) gives id 1, find(2,NULL) gives id 2, find(3,NULL) gives id
>> 3, find(4,NULL) gives id 4, find(5,NULL) fails and you fall back to
>> find(NULL,5) to give id 4. Thus, it only makes a difference for
>> snapshots whose name is a numeric string that also matches an id, where
>> your change now favors the id lookup over the entire set instead of the
>> first name or id match while doing a single pass over the set.
>>
>> Pavel's series on top of this would change the code to favor a name-only
>> lookup, or an explicit HMP option to do an id-only lookup, instead of
>> this code's double lookup.
>>
>> At this point, I'm okay with the semantics of this patch (especially
>> since we may be cleaning it up further in Pavel's patch series), but it
>> deserves explicit documentation in the commit message on what semantics
>> are changing (favoring id more strongly) and why (so that we can select
>> all possible snapshots, instead of being unable to select snapshots
>> whose id was claimed as a name of an earlier snapshot).
>>
> To avoid trouble, I think a new function named
> bdrv_snapshot_find_by_id_and_name() is better. Later Pavel
> can directly call this new function, and after that we can
> delete original bdrv_snapshot_find(). Pavel, what do you
> think?
Yes, we can create a new function with the new logic and the old one
will be dropped in my patch series. That's why I removed the old find
logic in my patch series in the last patch.
>
>>> @@ -2437,12 +2437,14 @@ int load_vmstate(const char *name)
>>> @@ -2461,11 +2463,11 @@ int load_vmstate(const char *name)
>>> return -ENOTSUP;
>>> }
>>>
>>> - ret = bdrv_snapshot_find(bs, &sn, name);
>>> - if (ret < 0) {
>>> + /* vm snapshot will always have same id and name, check
>>> do_savevm(). */
>>> + if (!bdrv_snapshot_find(bs, &sn, sn.id_str, sn.name, NULL)) {
>>> error_report("Device '%s' does not have the requested
>>> snapshot '%s'",
>>> bdrv_get_device_name(bs), name);
>>> - return ret;
>>> + return -ENOENT;
>>> }
>>
>> Are we 100% sure that a given snapshot name has the same id across all
>> block devices? Or is it possible to have:
>>
>> disk a: [id 1 name A, id 2 name B]
>> disk b: [id 1 name B]
>>
>> where it is possible to load snapshot [B] and get consistent state? If
>> it is possible to have non-matched ids across same-name snapshots, then
>> looking up by requiring a match of both id and name will fail, whereas
>> the pre-patch code would succeed.
>>
> not possible, I checked the existing code, a loadable snapshot, that
> is the one with vmstate, will always have same id and name, see savevm
> logic and qcow2's snapshot creation code.
> What changes is: previous, in "info snapshot", it will try show some
> snapshot that may have the situation you described above, which may be
> brought by qemu-img or hotplug operation, and which can't be loaded in
> "loadvm". Now it will be filtered out.
> Maybe there are more complicated case, but I think let management
> stack handling it, is a better option.
>
>
>>> }
>>>
>>> @@ -2536,7 +2538,7 @@ 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 nb_sns, i, available;
>>> int total;
>>> int *available_snapshots;
>>> char buf[256];
>>> @@ -2567,8 +2569,9 @@ 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);
>>> - if (ret < 0) {
>>> + /* vm snapshot will always have same id and name */
>>> + if (!bdrv_snapshot_find(bs1, sn_info,
>>> + sn->id_str, sn->name, NULL)) {
>>
>> Again, is this true, or are you needlessly filtering out snapshots that
>> have a consistent name but non-matching ids?
>>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-02 12:02 ` Luiz Capitulino
@ 2013-05-03 2:51 ` Wenchao Xia
2013-05-06 2:09 ` Wenchao Xia
0 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-05-03 2:51 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
于 2013-5-2 20:02, Luiz Capitulino 写道:
> On Thu, 02 May 2013 10:05:08 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
>>> On Fri, 26 Apr 2013 16:46:57 +0200
>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>
>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
>>>>> }
>>>>>
>>>>> if (total > 0) {
>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
>>>>> + bdrv_snapshot_dump(NULL);
>>>>> + monitor_printf(mon, "\n");
>>>>
>>>> Luiz: any issue with mixing monitor_printf(mon) and
>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
>>>> explicitly passing mon instead of relying on cur_mon.
>>>
>>> where are they being mixed?
>>>
>> bdrv_snapshot_dump() used a global variable "cur_mon" inside, instead
>> of let caller pass in a explicit montior* "mon", I guess that is the
>> question.
>
> I'd have to see the code to tell, but yes, what Stefan described is the
> best practice for the Monitor.
>
I think this would not be a problem until qemu wants more than one
human monitor console, and then we may require a data structure to tell
where to output the string: stdout, *mon, or even stderr, and
error_printf() also need to be changed.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-03 2:51 ` Wenchao Xia
@ 2013-05-06 2:09 ` Wenchao Xia
2013-05-06 13:22 ` Luiz Capitulino
0 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-05-06 2:09 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
于 2013-5-3 10:51, Wenchao Xia 写道:
> 于 2013-5-2 20:02, Luiz Capitulino 写道:
>> On Thu, 02 May 2013 10:05:08 +0800
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>
>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
>>>> On Fri, 26 Apr 2013 16:46:57 +0200
>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>>
>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const
>>>>>> QDict *qdict)
>>>>>> }
>>>>>>
>>>>>> if (total > 0) {
>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
>>>>>> sizeof(buf), NULL));
>>>>>> + bdrv_snapshot_dump(NULL);
>>>>>> + monitor_printf(mon, "\n");
>>>>>
>>>>> Luiz: any issue with mixing monitor_printf(mon) and
>>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
>>>>> explicitly passing mon instead of relying on cur_mon.
>>>>
>>>> where are they being mixed?
>>>>
>>> bdrv_snapshot_dump() used a global variable "cur_mon" inside,
>>> instead
>>> of let caller pass in a explicit montior* "mon", I guess that is the
>>> question.
>>
>> I'd have to see the code to tell, but yes, what Stefan described is the
>> best practice for the Monitor.
>>
> I think this would not be a problem until qemu wants more than one
> human monitor console, and then we may require a data structure to tell
> where to output the string: stdout, *mon, or even stderr, and
> error_printf() also need to be changed.
>
Luiz, what is your idea? I'd like to respin v2 if no issues for it.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-06 2:09 ` Wenchao Xia
@ 2013-05-06 13:22 ` Luiz Capitulino
2013-05-15 2:10 ` Wenchao Xia
0 siblings, 1 reply; 34+ messages in thread
From: Luiz Capitulino @ 2013-05-06 13:22 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
On Mon, 06 May 2013 10:09:43 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2013-5-3 10:51, Wenchao Xia 写道:
> > 于 2013-5-2 20:02, Luiz Capitulino 写道:
> >> On Thu, 02 May 2013 10:05:08 +0800
> >> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>
> >>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
> >>>> On Fri, 26 Apr 2013 16:46:57 +0200
> >>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >>>>
> >>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
> >>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const
> >>>>>> QDict *qdict)
> >>>>>> }
> >>>>>>
> >>>>>> if (total > 0) {
> >>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
> >>>>>> sizeof(buf), NULL));
> >>>>>> + bdrv_snapshot_dump(NULL);
> >>>>>> + monitor_printf(mon, "\n");
> >>>>>
> >>>>> Luiz: any issue with mixing monitor_printf(mon) and
> >>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
> >>>>> explicitly passing mon instead of relying on cur_mon.
> >>>>
> >>>> where are they being mixed?
> >>>>
> >>> bdrv_snapshot_dump() used a global variable "cur_mon" inside,
> >>> instead
> >>> of let caller pass in a explicit montior* "mon", I guess that is the
> >>> question.
> >>
> >> I'd have to see the code to tell, but yes, what Stefan described is the
> >> best practice for the Monitor.
> >>
> > I think this would not be a problem until qemu wants more than one
> > human monitor console, and then we may require a data structure to tell
> > where to output the string: stdout, *mon, or even stderr, and
> > error_printf() also need to be changed.
> >
> Luiz, what is your idea? I'd like to respin v2 if no issues for it.
As I said before, I'd have to see the code to tell. But answering your comment,
the code does support multiple monitors.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-06 13:22 ` Luiz Capitulino
@ 2013-05-15 2:10 ` Wenchao Xia
2013-05-15 12:28 ` Luiz Capitulino
0 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-05-15 2:10 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
于 2013-5-6 21:22, Luiz Capitulino 写道:
> On Mon, 06 May 2013 10:09:43 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-5-3 10:51, Wenchao Xia 写道:
>>> 于 2013-5-2 20:02, Luiz Capitulino 写道:
>>>> On Thu, 02 May 2013 10:05:08 +0800
>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>
>>>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
>>>>>> On Fri, 26 Apr 2013 16:46:57 +0200
>>>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>>>>
>>>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
>>>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const
>>>>>>>> QDict *qdict)
>>>>>>>> }
>>>>>>>>
>>>>>>>> if (total > 0) {
>>>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
>>>>>>>> sizeof(buf), NULL));
>>>>>>>> + bdrv_snapshot_dump(NULL);
>>>>>>>> + monitor_printf(mon, "\n");
>>>>>>>
>>>>>>> Luiz: any issue with mixing monitor_printf(mon) and
>>>>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
>>>>>>> explicitly passing mon instead of relying on cur_mon.
>>>>>>
>>>>>> where are they being mixed?
>>>>>>
>>>>> bdrv_snapshot_dump() used a global variable "cur_mon" inside,
>>>>> instead
>>>>> of let caller pass in a explicit montior* "mon", I guess that is the
>>>>> question.
>>>>
>>>> I'd have to see the code to tell, but yes, what Stefan described is the
>>>> best practice for the Monitor.
>>>>
>>> I think this would not be a problem until qemu wants more than one
>>> human monitor console, and then we may require a data structure to tell
>>> where to output the string: stdout, *mon, or even stderr, and
>>> error_printf() also need to be changed.
>>>
>> Luiz, what is your idea? I'd like to respin v2 if no issues for it.
>
> As I said before, I'd have to see the code to tell. But answering your comment,
> the code does support multiple monitors.
>
Hi Luiz,
Sorry to ask again, do you think method above is OK now, waiting for
your confirm.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-15 2:10 ` Wenchao Xia
@ 2013-05-15 12:28 ` Luiz Capitulino
2013-05-16 2:22 ` Wenchao Xia
0 siblings, 1 reply; 34+ messages in thread
From: Luiz Capitulino @ 2013-05-15 12:28 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
On Wed, 15 May 2013 10:10:37 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2013-5-6 21:22, Luiz Capitulino 写道:
> > On Mon, 06 May 2013 10:09:43 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >> 于 2013-5-3 10:51, Wenchao Xia 写道:
> >>> 于 2013-5-2 20:02, Luiz Capitulino 写道:
> >>>> On Thu, 02 May 2013 10:05:08 +0800
> >>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>
> >>>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
> >>>>>> On Fri, 26 Apr 2013 16:46:57 +0200
> >>>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >>>>>>
> >>>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
> >>>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const
> >>>>>>>> QDict *qdict)
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> if (total > 0) {
> >>>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
> >>>>>>>> sizeof(buf), NULL));
> >>>>>>>> + bdrv_snapshot_dump(NULL);
> >>>>>>>> + monitor_printf(mon, "\n");
> >>>>>>>
> >>>>>>> Luiz: any issue with mixing monitor_printf(mon) and
> >>>>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
> >>>>>>> explicitly passing mon instead of relying on cur_mon.
> >>>>>>
> >>>>>> where are they being mixed?
> >>>>>>
> >>>>> bdrv_snapshot_dump() used a global variable "cur_mon" inside,
> >>>>> instead
> >>>>> of let caller pass in a explicit montior* "mon", I guess that is the
> >>>>> question.
> >>>>
> >>>> I'd have to see the code to tell, but yes, what Stefan described is the
> >>>> best practice for the Monitor.
> >>>>
> >>> I think this would not be a problem until qemu wants more than one
> >>> human monitor console, and then we may require a data structure to tell
> >>> where to output the string: stdout, *mon, or even stderr, and
> >>> error_printf() also need to be changed.
> >>>
> >> Luiz, what is your idea? I'd like to respin v2 if no issues for it.
> >
> > As I said before, I'd have to see the code to tell. But answering your comment,
> > the code does support multiple monitors.
> >
> Hi Luiz,
> Sorry to ask again, do you think method above is OK now, waiting for
> your confirm.
Can you point me to the code in question?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-15 12:28 ` Luiz Capitulino
@ 2013-05-16 2:22 ` Wenchao Xia
2013-05-16 12:17 ` Luiz Capitulino
0 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-05-16 2:22 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
于 2013-5-15 20:28, Luiz Capitulino 写道:
> On Wed, 15 May 2013 10:10:37 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-5-6 21:22, Luiz Capitulino 写道:
>>> On Mon, 06 May 2013 10:09:43 +0800
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>
>>>> 于 2013-5-3 10:51, Wenchao Xia 写道:
>>>>> 于 2013-5-2 20:02, Luiz Capitulino 写道:
>>>>>> On Thu, 02 May 2013 10:05:08 +0800
>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>>
>>>>>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
>>>>>>>> On Fri, 26 Apr 2013 16:46:57 +0200
>>>>>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>>>>>>
>>>>>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
>>>>>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const
>>>>>>>>>> QDict *qdict)
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> if (total > 0) {
>>>>>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
>>>>>>>>>> sizeof(buf), NULL));
>>>>>>>>>> + bdrv_snapshot_dump(NULL);
>>>>>>>>>> + monitor_printf(mon, "\n");
>>>>>>>>>
>>>>>>>>> Luiz: any issue with mixing monitor_printf(mon) and
>>>>>>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
>>>>>>>>> explicitly passing mon instead of relying on cur_mon.
>>>>>>>>
>>>>>>>> where are they being mixed?
>>>>>>>>
>>>>>>> bdrv_snapshot_dump() used a global variable "cur_mon" inside,
>>>>>>> instead
>>>>>>> of let caller pass in a explicit montior* "mon", I guess that is the
>>>>>>> question.
>>>>>>
>>>>>> I'd have to see the code to tell, but yes, what Stefan described is the
>>>>>> best practice for the Monitor.
>>>>>>
>>>>> I think this would not be a problem until qemu wants more than one
>>>>> human monitor console, and then we may require a data structure to tell
>>>>> where to output the string: stdout, *mon, or even stderr, and
>>>>> error_printf() also need to be changed.
>>>>>
>>>> Luiz, what is your idea? I'd like to respin v2 if no issues for it.
>>>
>>> As I said before, I'd have to see the code to tell. But answering your comment,
>>> the code does support multiple monitors.
>>>
>> Hi Luiz,
>> Sorry to ask again, do you think method above is OK now, waiting for
>> your confirm.
>
> Can you point me to the code in question?
>
Sure, it is
+
+/*
+ * Print to current monitor if we have one, else to stdout. It is
similar with
+ * error_printf().
+ * TODO just like error_vprintf()
+ */
+void message_printf(const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ if (cur_mon) {
+ monitor_vprintf(cur_mon, fmt, ap);
+ } else {
+ vfprintf(stdout, fmt, ap);
+ }
+ va_end(ap);
+}
This function used global variable cur_mon instead of input parameter,
similar to error_printf().
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-16 2:22 ` Wenchao Xia
@ 2013-05-16 12:17 ` Luiz Capitulino
2013-05-17 3:30 ` Wenchao Xia
0 siblings, 1 reply; 34+ messages in thread
From: Luiz Capitulino @ 2013-05-16 12:17 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
On Thu, 16 May 2013 10:22:09 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2013-5-15 20:28, Luiz Capitulino 写道:
> > On Wed, 15 May 2013 10:10:37 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >> 于 2013-5-6 21:22, Luiz Capitulino 写道:
> >>> On Mon, 06 May 2013 10:09:43 +0800
> >>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>
> >>>> 于 2013-5-3 10:51, Wenchao Xia 写道:
> >>>>> 于 2013-5-2 20:02, Luiz Capitulino 写道:
> >>>>>> On Thu, 02 May 2013 10:05:08 +0800
> >>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>>>
> >>>>>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
> >>>>>>>> On Fri, 26 Apr 2013 16:46:57 +0200
> >>>>>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
> >>>>>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const
> >>>>>>>>>> QDict *qdict)
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> if (total > 0) {
> >>>>>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
> >>>>>>>>>> sizeof(buf), NULL));
> >>>>>>>>>> + bdrv_snapshot_dump(NULL);
> >>>>>>>>>> + monitor_printf(mon, "\n");
> >>>>>>>>>
> >>>>>>>>> Luiz: any issue with mixing monitor_printf(mon) and
> >>>>>>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
> >>>>>>>>> explicitly passing mon instead of relying on cur_mon.
> >>>>>>>>
> >>>>>>>> where are they being mixed?
> >>>>>>>>
> >>>>>>> bdrv_snapshot_dump() used a global variable "cur_mon" inside,
> >>>>>>> instead
> >>>>>>> of let caller pass in a explicit montior* "mon", I guess that is the
> >>>>>>> question.
> >>>>>>
> >>>>>> I'd have to see the code to tell, but yes, what Stefan described is the
> >>>>>> best practice for the Monitor.
> >>>>>>
> >>>>> I think this would not be a problem until qemu wants more than one
> >>>>> human monitor console, and then we may require a data structure to tell
> >>>>> where to output the string: stdout, *mon, or even stderr, and
> >>>>> error_printf() also need to be changed.
> >>>>>
> >>>> Luiz, what is your idea? I'd like to respin v2 if no issues for it.
> >>>
> >>> As I said before, I'd have to see the code to tell. But answering your comment,
> >>> the code does support multiple monitors.
> >>>
> >> Hi Luiz,
> >> Sorry to ask again, do you think method above is OK now, waiting for
> >> your confirm.
> >
> > Can you point me to the code in question?
> >
> Sure, it is
>
> +
> +/*
> + * Print to current monitor if we have one, else to stdout. It is
> similar with
> + * error_printf().
> + * TODO just like error_vprintf()
> + */
> +void message_printf(const char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + if (cur_mon) {
> + monitor_vprintf(cur_mon, fmt, ap);
> + } else {
> + vfprintf(stdout, fmt, ap);
> + }
> + va_end(ap);
> +}
>
> This function used global variable cur_mon instead of input parameter,
> similar to error_printf().
Why do you need it? Why can't you you use error_printf() for example?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-16 12:17 ` Luiz Capitulino
@ 2013-05-17 3:30 ` Wenchao Xia
2013-05-17 12:30 ` Luiz Capitulino
0 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-05-17 3:30 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
于 2013-5-16 20:17, Luiz Capitulino 写道:
> On Thu, 16 May 2013 10:22:09 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-5-15 20:28, Luiz Capitulino 写道:
>>> On Wed, 15 May 2013 10:10:37 +0800
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>
>>>> 于 2013-5-6 21:22, Luiz Capitulino 写道:
>>>>> On Mon, 06 May 2013 10:09:43 +0800
>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>> 于 2013-5-3 10:51, Wenchao Xia 写道:
>>>>>>> 于 2013-5-2 20:02, Luiz Capitulino 写道:
>>>>>>>> On Thu, 02 May 2013 10:05:08 +0800
>>>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>>>>
>>>>>>>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
>>>>>>>>>> On Fri, 26 Apr 2013 16:46:57 +0200
>>>>>>>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
>>>>>>>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const
>>>>>>>>>>>> QDict *qdict)
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> if (total > 0) {
>>>>>>>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
>>>>>>>>>>>> sizeof(buf), NULL));
>>>>>>>>>>>> + bdrv_snapshot_dump(NULL);
>>>>>>>>>>>> + monitor_printf(mon, "\n");
>>>>>>>>>>>
>>>>>>>>>>> Luiz: any issue with mixing monitor_printf(mon) and
>>>>>>>>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
>>>>>>>>>>> explicitly passing mon instead of relying on cur_mon.
>>>>>>>>>>
>>>>>>>>>> where are they being mixed?
>>>>>>>>>>
>>>>>>>>> bdrv_snapshot_dump() used a global variable "cur_mon" inside,
>>>>>>>>> instead
>>>>>>>>> of let caller pass in a explicit montior* "mon", I guess that is the
>>>>>>>>> question.
>>>>>>>>
>>>>>>>> I'd have to see the code to tell, but yes, what Stefan described is the
>>>>>>>> best practice for the Monitor.
>>>>>>>>
>>>>>>> I think this would not be a problem until qemu wants more than one
>>>>>>> human monitor console, and then we may require a data structure to tell
>>>>>>> where to output the string: stdout, *mon, or even stderr, and
>>>>>>> error_printf() also need to be changed.
>>>>>>>
>>>>>> Luiz, what is your idea? I'd like to respin v2 if no issues for it.
>>>>>
>>>>> As I said before, I'd have to see the code to tell. But answering your comment,
>>>>> the code does support multiple monitors.
>>>>>
>>>> Hi Luiz,
>>>> Sorry to ask again, do you think method above is OK now, waiting for
>>>> your confirm.
>>>
>>> Can you point me to the code in question?
>>>
>> Sure, it is
>>
>> +
>> +/*
>> + * Print to current monitor if we have one, else to stdout. It is
>> similar with
>> + * error_printf().
>> + * TODO just like error_vprintf()
>> + */
>> +void message_printf(const char *fmt, ...)
>> +{
>> + va_list ap;
>> +
>> + va_start(ap, fmt);
>> + if (cur_mon) {
>> + monitor_vprintf(cur_mon, fmt, ap);
>> + } else {
>> + vfprintf(stdout, fmt, ap);
>> + }
>> + va_end(ap);
>> +}
>>
>> This function used global variable cur_mon instead of input parameter,
>> similar to error_printf().
>
> Why do you need it? Why can't you you use error_printf() for example?
>
error_printf() will print out to stderr in qemu-img, but stdout is
wanted for those dump info function.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-17 3:30 ` Wenchao Xia
@ 2013-05-17 12:30 ` Luiz Capitulino
2013-05-20 2:39 ` Wenchao Xia
0 siblings, 1 reply; 34+ messages in thread
From: Luiz Capitulino @ 2013-05-17 12:30 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
On Fri, 17 May 2013 11:30:31 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2013-5-16 20:17, Luiz Capitulino 写道:
> > On Thu, 16 May 2013 10:22:09 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >> 于 2013-5-15 20:28, Luiz Capitulino 写道:
> >>> On Wed, 15 May 2013 10:10:37 +0800
> >>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>
> >>>> 于 2013-5-6 21:22, Luiz Capitulino 写道:
> >>>>> On Mon, 06 May 2013 10:09:43 +0800
> >>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>>
> >>>>>> 于 2013-5-3 10:51, Wenchao Xia 写道:
> >>>>>>> 于 2013-5-2 20:02, Luiz Capitulino 写道:
> >>>>>>>> On Thu, 02 May 2013 10:05:08 +0800
> >>>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>>>>>
> >>>>>>>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
> >>>>>>>>>> On Fri, 26 Apr 2013 16:46:57 +0200
> >>>>>>>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
> >>>>>>>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const
> >>>>>>>>>>>> QDict *qdict)
> >>>>>>>>>>>> }
> >>>>>>>>>>>>
> >>>>>>>>>>>> if (total > 0) {
> >>>>>>>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
> >>>>>>>>>>>> sizeof(buf), NULL));
> >>>>>>>>>>>> + bdrv_snapshot_dump(NULL);
> >>>>>>>>>>>> + monitor_printf(mon, "\n");
> >>>>>>>>>>>
> >>>>>>>>>>> Luiz: any issue with mixing monitor_printf(mon) and
> >>>>>>>>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
> >>>>>>>>>>> explicitly passing mon instead of relying on cur_mon.
> >>>>>>>>>>
> >>>>>>>>>> where are they being mixed?
> >>>>>>>>>>
> >>>>>>>>> bdrv_snapshot_dump() used a global variable "cur_mon" inside,
> >>>>>>>>> instead
> >>>>>>>>> of let caller pass in a explicit montior* "mon", I guess that is the
> >>>>>>>>> question.
> >>>>>>>>
> >>>>>>>> I'd have to see the code to tell, but yes, what Stefan described is the
> >>>>>>>> best practice for the Monitor.
> >>>>>>>>
> >>>>>>> I think this would not be a problem until qemu wants more than one
> >>>>>>> human monitor console, and then we may require a data structure to tell
> >>>>>>> where to output the string: stdout, *mon, or even stderr, and
> >>>>>>> error_printf() also need to be changed.
> >>>>>>>
> >>>>>> Luiz, what is your idea? I'd like to respin v2 if no issues for it.
> >>>>>
> >>>>> As I said before, I'd have to see the code to tell. But answering your comment,
> >>>>> the code does support multiple monitors.
> >>>>>
> >>>> Hi Luiz,
> >>>> Sorry to ask again, do you think method above is OK now, waiting for
> >>>> your confirm.
> >>>
> >>> Can you point me to the code in question?
> >>>
> >> Sure, it is
> >>
> >> +
> >> +/*
> >> + * Print to current monitor if we have one, else to stdout. It is
> >> similar with
> >> + * error_printf().
> >> + * TODO just like error_vprintf()
> >> + */
> >> +void message_printf(const char *fmt, ...)
> >> +{
> >> + va_list ap;
> >> +
> >> + va_start(ap, fmt);
> >> + if (cur_mon) {
> >> + monitor_vprintf(cur_mon, fmt, ap);
> >> + } else {
> >> + vfprintf(stdout, fmt, ap);
> >> + }
> >> + va_end(ap);
> >> +}
> >>
> >> This function used global variable cur_mon instead of input parameter,
> >> similar to error_printf().
> >
> > Why do you need it? Why can't you you use error_printf() for example?
> >
> error_printf() will print out to stderr in qemu-img, but stdout is
> wanted for those dump info function.
You can refactor the code so that you can pass a FILE *stream argument to
error_vprintf() and maybe add error_printf_stream()?
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-17 12:30 ` Luiz Capitulino
@ 2013-05-20 2:39 ` Wenchao Xia
2013-05-22 2:09 ` Wenchao Xia
0 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-05-20 2:39 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
于 2013-5-17 20:30, Luiz Capitulino 写道:
> On Fri, 17 May 2013 11:30:31 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-5-16 20:17, Luiz Capitulino 写道:
>>> On Thu, 16 May 2013 10:22:09 +0800
>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>
>>>> 于 2013-5-15 20:28, Luiz Capitulino 写道:
>>>>> On Wed, 15 May 2013 10:10:37 +0800
>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>
>>>>>> 于 2013-5-6 21:22, Luiz Capitulino 写道:
>>>>>>> On Mon, 06 May 2013 10:09:43 +0800
>>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>>>
>>>>>>>> 于 2013-5-3 10:51, Wenchao Xia 写道:
>>>>>>>>> 于 2013-5-2 20:02, Luiz Capitulino 写道:
>>>>>>>>>> On Thu, 02 May 2013 10:05:08 +0800
>>>>>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
>>>>>>>>>>>> On Fri, 26 Apr 2013 16:46:57 +0200
>>>>>>>>>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
>>>>>>>>>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor *mon, const
>>>>>>>>>>>>>> QDict *qdict)
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> if (total > 0) {
>>>>>>>>>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
>>>>>>>>>>>>>> sizeof(buf), NULL));
>>>>>>>>>>>>>> + bdrv_snapshot_dump(NULL);
>>>>>>>>>>>>>> + monitor_printf(mon, "\n");
>>>>>>>>>>>>>
>>>>>>>>>>>>> Luiz: any issue with mixing monitor_printf(mon) and
>>>>>>>>>>>>> monitor_vprintf(cur_mon) calls? I guess there was a reason for
>>>>>>>>>>>>> explicitly passing mon instead of relying on cur_mon.
>>>>>>>>>>>>
>>>>>>>>>>>> where are they being mixed?
>>>>>>>>>>>>
>>>>>>>>>>> bdrv_snapshot_dump() used a global variable "cur_mon" inside,
>>>>>>>>>>> instead
>>>>>>>>>>> of let caller pass in a explicit montior* "mon", I guess that is the
>>>>>>>>>>> question.
>>>>>>>>>>
>>>>>>>>>> I'd have to see the code to tell, but yes, what Stefan described is the
>>>>>>>>>> best practice for the Monitor.
>>>>>>>>>>
>>>>>>>>> I think this would not be a problem until qemu wants more than one
>>>>>>>>> human monitor console, and then we may require a data structure to tell
>>>>>>>>> where to output the string: stdout, *mon, or even stderr, and
>>>>>>>>> error_printf() also need to be changed.
>>>>>>>>>
>>>>>>>> Luiz, what is your idea? I'd like to respin v2 if no issues for it.
>>>>>>>
>>>>>>> As I said before, I'd have to see the code to tell. But answering your comment,
>>>>>>> the code does support multiple monitors.
>>>>>>>
>>>>>> Hi Luiz,
>>>>>> Sorry to ask again, do you think method above is OK now, waiting for
>>>>>> your confirm.
>>>>>
>>>>> Can you point me to the code in question?
>>>>>
>>>> Sure, it is
>>>>
>>>> +
>>>> +/*
>>>> + * Print to current monitor if we have one, else to stdout. It is
>>>> similar with
>>>> + * error_printf().
>>>> + * TODO just like error_vprintf()
>>>> + */
>>>> +void message_printf(const char *fmt, ...)
>>>> +{
>>>> + va_list ap;
>>>> +
>>>> + va_start(ap, fmt);
>>>> + if (cur_mon) {
>>>> + monitor_vprintf(cur_mon, fmt, ap);
>>>> + } else {
>>>> + vfprintf(stdout, fmt, ap);
>>>> + }
>>>> + va_end(ap);
>>>> +}
>>>>
>>>> This function used global variable cur_mon instead of input parameter,
>>>> similar to error_printf().
>>>
>>> Why do you need it? Why can't you you use error_printf() for example?
>>>
>> error_printf() will print out to stderr in qemu-img, but stdout is
>> wanted for those dump info function.
>
> You can refactor the code so that you can pass a FILE *stream argument to
> error_vprintf() and maybe add error_printf_stream()?
>
The name is a bit confusing, maybe qemu_printf()? Another problem is,
monitor have a buf[] instead of a FILE*, I think it need a structure
include those:
typdef enum QemuOutputType {
QEMU_OUTPUT_TYPE_STREAM,
QEMU_OUTPUT_TYPE_MONITOR,
} QemuOutputType;
typedef struct QemuOutput {
QemuOutputType type;
union {
FILE *file;
Monitor *mon;
};
}
It may brings some inconvienience to caller, but this is what I can
think out now.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-20 2:39 ` Wenchao Xia
@ 2013-05-22 2:09 ` Wenchao Xia
2013-05-22 12:23 ` Luiz Capitulino
0 siblings, 1 reply; 34+ messages in thread
From: Wenchao Xia @ 2013-05-22 2:09 UTC (permalink / raw)
To: Luiz Capitulino
Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
于 2013-5-20 10:39, Wenchao Xia 写道:
> 于 2013-5-17 20:30, Luiz Capitulino 写道:
>> On Fri, 17 May 2013 11:30:31 +0800
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>
>>> 于 2013-5-16 20:17, Luiz Capitulino 写道:
>>>> On Thu, 16 May 2013 10:22:09 +0800
>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>
>>>>> 于 2013-5-15 20:28, Luiz Capitulino 写道:
>>>>>> On Wed, 15 May 2013 10:10:37 +0800
>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>>
>>>>>>> 于 2013-5-6 21:22, Luiz Capitulino 写道:
>>>>>>>> On Mon, 06 May 2013 10:09:43 +0800
>>>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>>>>
>>>>>>>>> 于 2013-5-3 10:51, Wenchao Xia 写道:
>>>>>>>>>> 于 2013-5-2 20:02, Luiz Capitulino 写道:
>>>>>>>>>>> On Thu, 02 May 2013 10:05:08 +0800
>>>>>>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
>>>>>>>>>>>>> On Fri, 26 Apr 2013 16:46:57 +0200
>>>>>>>>>>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
>>>>>>>>>>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor
>>>>>>>>>>>>>>> *mon, const
>>>>>>>>>>>>>>> QDict *qdict)
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> if (total > 0) {
>>>>>>>>>>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
>>>>>>>>>>>>>>> sizeof(buf), NULL));
>>>>>>>>>>>>>>> + bdrv_snapshot_dump(NULL);
>>>>>>>>>>>>>>> + monitor_printf(mon, "\n");
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Luiz: any issue with mixing monitor_printf(mon) and
>>>>>>>>>>>>>> monitor_vprintf(cur_mon) calls? I guess there was a
>>>>>>>>>>>>>> reason for
>>>>>>>>>>>>>> explicitly passing mon instead of relying on cur_mon.
>>>>>>>>>>>>>
>>>>>>>>>>>>> where are they being mixed?
>>>>>>>>>>>>>
>>>>>>>>>>>> bdrv_snapshot_dump() used a global variable
>>>>>>>>>>>> "cur_mon" inside,
>>>>>>>>>>>> instead
>>>>>>>>>>>> of let caller pass in a explicit montior* "mon", I guess
>>>>>>>>>>>> that is the
>>>>>>>>>>>> question.
>>>>>>>>>>>
>>>>>>>>>>> I'd have to see the code to tell, but yes, what Stefan
>>>>>>>>>>> described is the
>>>>>>>>>>> best practice for the Monitor.
>>>>>>>>>>>
>>>>>>>>>> I think this would not be a problem until qemu wants
>>>>>>>>>> more than one
>>>>>>>>>> human monitor console, and then we may require a data
>>>>>>>>>> structure to tell
>>>>>>>>>> where to output the string: stdout, *mon, or even stderr, and
>>>>>>>>>> error_printf() also need to be changed.
>>>>>>>>>>
>>>>>>>>> Luiz, what is your idea? I'd like to respin v2 if no
>>>>>>>>> issues for it.
>>>>>>>>
>>>>>>>> As I said before, I'd have to see the code to tell. But
>>>>>>>> answering your comment,
>>>>>>>> the code does support multiple monitors.
>>>>>>>>
>>>>>>> Hi Luiz,
>>>>>>> Sorry to ask again, do you think method above is OK now,
>>>>>>> waiting for
>>>>>>> your confirm.
>>>>>>
>>>>>> Can you point me to the code in question?
>>>>>>
>>>>> Sure, it is
>>>>>
>>>>> +
>>>>> +/*
>>>>> + * Print to current monitor if we have one, else to stdout. It is
>>>>> similar with
>>>>> + * error_printf().
>>>>> + * TODO just like error_vprintf()
>>>>> + */
>>>>> +void message_printf(const char *fmt, ...)
>>>>> +{
>>>>> + va_list ap;
>>>>> +
>>>>> + va_start(ap, fmt);
>>>>> + if (cur_mon) {
>>>>> + monitor_vprintf(cur_mon, fmt, ap);
>>>>> + } else {
>>>>> + vfprintf(stdout, fmt, ap);
>>>>> + }
>>>>> + va_end(ap);
>>>>> +}
>>>>>
>>>>> This function used global variable cur_mon instead of input
>>>>> parameter,
>>>>> similar to error_printf().
>>>>
>>>> Why do you need it? Why can't you you use error_printf() for example?
>>>>
>>> error_printf() will print out to stderr in qemu-img, but stdout is
>>> wanted for those dump info function.
>>
>> You can refactor the code so that you can pass a FILE *stream argument to
>> error_vprintf() and maybe add error_printf_stream()?
>>
> The name is a bit confusing, maybe qemu_printf()? Another problem is,
> monitor have a buf[] instead of a FILE*, I think it need a structure
> include those:
>
> typdef enum QemuOutputType {
> QEMU_OUTPUT_TYPE_STREAM,
> QEMU_OUTPUT_TYPE_MONITOR,
> } QemuOutputType;
>
> typedef struct QemuOutput {
> QemuOutputType type;
> union {
> FILE *file;
> Monitor *mon;
> };
> }
>
> It may brings some inconvienience to caller, but this is what I can
> think out now.
>
Luiz, I am going to respin V2 as above, can I have you confirm on it?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-22 2:09 ` Wenchao Xia
@ 2013-05-22 12:23 ` Luiz Capitulino
0 siblings, 0 replies; 34+ messages in thread
From: Luiz Capitulino @ 2013-05-22 12:23 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, Stefan Hajnoczi, qemu-devel, armbru, pbonzini
On Wed, 22 May 2013 10:09:19 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2013-5-20 10:39, Wenchao Xia 写道:
> > 于 2013-5-17 20:30, Luiz Capitulino 写道:
> >> On Fri, 17 May 2013 11:30:31 +0800
> >> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>
> >>> 于 2013-5-16 20:17, Luiz Capitulino 写道:
> >>>> On Thu, 16 May 2013 10:22:09 +0800
> >>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>
> >>>>> 于 2013-5-15 20:28, Luiz Capitulino 写道:
> >>>>>> On Wed, 15 May 2013 10:10:37 +0800
> >>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>>>
> >>>>>>> 于 2013-5-6 21:22, Luiz Capitulino 写道:
> >>>>>>>> On Mon, 06 May 2013 10:09:43 +0800
> >>>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>>>>>
> >>>>>>>>> 于 2013-5-3 10:51, Wenchao Xia 写道:
> >>>>>>>>>> 于 2013-5-2 20:02, Luiz Capitulino 写道:
> >>>>>>>>>>> On Thu, 02 May 2013 10:05:08 +0800
> >>>>>>>>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> 于 2013-4-30 3:05, Luiz Capitulino 写道:
> >>>>>>>>>>>>> On Fri, 26 Apr 2013 16:46:57 +0200
> >>>>>>>>>>>>> Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> On Fri, Apr 26, 2013 at 05:31:15PM +0800, Wenchao Xia wrote:
> >>>>>>>>>>>>>>> @@ -2586,10 +2585,12 @@ void do_info_snapshots(Monitor
> >>>>>>>>>>>>>>> *mon, const
> >>>>>>>>>>>>>>> QDict *qdict)
> >>>>>>>>>>>>>>> }
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> if (total > 0) {
> >>>>>>>>>>>>>>> - monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf,
> >>>>>>>>>>>>>>> sizeof(buf), NULL));
> >>>>>>>>>>>>>>> + bdrv_snapshot_dump(NULL);
> >>>>>>>>>>>>>>> + monitor_printf(mon, "\n");
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Luiz: any issue with mixing monitor_printf(mon) and
> >>>>>>>>>>>>>> monitor_vprintf(cur_mon) calls? I guess there was a
> >>>>>>>>>>>>>> reason for
> >>>>>>>>>>>>>> explicitly passing mon instead of relying on cur_mon.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> where are they being mixed?
> >>>>>>>>>>>>>
> >>>>>>>>>>>> bdrv_snapshot_dump() used a global variable
> >>>>>>>>>>>> "cur_mon" inside,
> >>>>>>>>>>>> instead
> >>>>>>>>>>>> of let caller pass in a explicit montior* "mon", I guess
> >>>>>>>>>>>> that is the
> >>>>>>>>>>>> question.
> >>>>>>>>>>>
> >>>>>>>>>>> I'd have to see the code to tell, but yes, what Stefan
> >>>>>>>>>>> described is the
> >>>>>>>>>>> best practice for the Monitor.
> >>>>>>>>>>>
> >>>>>>>>>> I think this would not be a problem until qemu wants
> >>>>>>>>>> more than one
> >>>>>>>>>> human monitor console, and then we may require a data
> >>>>>>>>>> structure to tell
> >>>>>>>>>> where to output the string: stdout, *mon, or even stderr, and
> >>>>>>>>>> error_printf() also need to be changed.
> >>>>>>>>>>
> >>>>>>>>> Luiz, what is your idea? I'd like to respin v2 if no
> >>>>>>>>> issues for it.
> >>>>>>>>
> >>>>>>>> As I said before, I'd have to see the code to tell. But
> >>>>>>>> answering your comment,
> >>>>>>>> the code does support multiple monitors.
> >>>>>>>>
> >>>>>>> Hi Luiz,
> >>>>>>> Sorry to ask again, do you think method above is OK now,
> >>>>>>> waiting for
> >>>>>>> your confirm.
> >>>>>>
> >>>>>> Can you point me to the code in question?
> >>>>>>
> >>>>> Sure, it is
> >>>>>
> >>>>> +
> >>>>> +/*
> >>>>> + * Print to current monitor if we have one, else to stdout. It is
> >>>>> similar with
> >>>>> + * error_printf().
> >>>>> + * TODO just like error_vprintf()
> >>>>> + */
> >>>>> +void message_printf(const char *fmt, ...)
> >>>>> +{
> >>>>> + va_list ap;
> >>>>> +
> >>>>> + va_start(ap, fmt);
> >>>>> + if (cur_mon) {
> >>>>> + monitor_vprintf(cur_mon, fmt, ap);
> >>>>> + } else {
> >>>>> + vfprintf(stdout, fmt, ap);
> >>>>> + }
> >>>>> + va_end(ap);
> >>>>> +}
> >>>>>
> >>>>> This function used global variable cur_mon instead of input
> >>>>> parameter,
> >>>>> similar to error_printf().
> >>>>
> >>>> Why do you need it? Why can't you you use error_printf() for example?
> >>>>
> >>> error_printf() will print out to stderr in qemu-img, but stdout is
> >>> wanted for those dump info function.
> >>
> >> You can refactor the code so that you can pass a FILE *stream argument to
> >> error_vprintf() and maybe add error_printf_stream()?
> >>
> > The name is a bit confusing, maybe qemu_printf()? Another problem is,
> > monitor have a buf[] instead of a FILE*, I think it need a structure
> > include those:
> >
> > typdef enum QemuOutputType {
> > QEMU_OUTPUT_TYPE_STREAM,
> > QEMU_OUTPUT_TYPE_MONITOR,
> > } QemuOutputType;
> >
> > typedef struct QemuOutput {
> > QemuOutputType type;
> > union {
> > FILE *file;
> > Monitor *mon;
> > };
> > }
> >
> > It may brings some inconvienience to caller, but this is what I can
> > think out now.
> >
> Luiz, I am going to respin V2 as above, can I have you confirm on it?
I'm honestly a bit confused with what you're suggesting. I'd just respin the
patches and restart the discussion.
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2013-05-22 12:23 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-04-26 9:31 [Qemu-devel] [PATCH 0/7] qapi and snapshot code clean up in block layer Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 1/7] block: drop bs_snapshots global variable Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 2/7] block: move bdrv_snapshot_find() to block/snapshot.c Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 3/7] block: move snapshot code in block.c " Wenchao Xia
2013-04-26 19:05 ` Eric Blake
2013-04-26 9:31 ` [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot() Wenchao Xia
2013-04-26 14:34 ` Stefan Hajnoczi
2013-04-26 14:47 ` Eric Blake
2013-04-27 3:34 ` Wenchao Xia
2013-04-30 17:52 ` Eric Blake
2013-04-30 18:16 ` Eric Blake
2013-05-02 2:02 ` Wenchao Xia
2013-05-02 21:12 ` Pavel Hrdina
2013-04-26 9:31 ` [Qemu-devel] [PATCH 5/7] block: move collect_snapshots() and collect_image_info() to block/qapi.c Wenchao Xia
2013-04-26 9:31 ` [Qemu-devel] [PATCH 6/7] block: move qmp and info dump related code " Wenchao Xia
2013-04-30 17:50 ` Eric Blake
2013-04-26 9:31 ` [Qemu-devel] [PATCH 7/7] block: dump to monitor for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
2013-04-26 14:46 ` Stefan Hajnoczi
2013-04-27 3:37 ` Wenchao Xia
2013-04-29 19:05 ` Luiz Capitulino
2013-05-02 2:05 ` Wenchao Xia
2013-05-02 12:02 ` Luiz Capitulino
2013-05-03 2:51 ` Wenchao Xia
2013-05-06 2:09 ` Wenchao Xia
2013-05-06 13:22 ` Luiz Capitulino
2013-05-15 2:10 ` Wenchao Xia
2013-05-15 12:28 ` Luiz Capitulino
2013-05-16 2:22 ` Wenchao Xia
2013-05-16 12:17 ` Luiz Capitulino
2013-05-17 3:30 ` Wenchao Xia
2013-05-17 12:30 ` Luiz Capitulino
2013-05-20 2:39 ` Wenchao Xia
2013-05-22 2:09 ` Wenchao Xia
2013-05-22 12:23 ` Luiz Capitulino
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.