* [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer
@ 2013-05-25 3:09 Wenchao Xia
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable Wenchao Xia
` (5 more replies)
0 siblings, 6 replies; 25+ messages in thread
From: Wenchao Xia @ 2013-05-25 3:09 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 info dumping function to get rid of buffer, avoid string truncation.
v2:
Squash code moving patches since they are reviewed in v1.
Drop bdrv_snapshot_find() function change patch, since it related to snapshot
logic which should be changed together with Pavel's serial.
Use a parameter in message_printf() to tell where to print, instead of use
global variable "cur_mon" inside.
v3:
Address Stefan's comments:
2/4: do not move bdrv_is_snapshot().
4/4: reuse fprintf_function function pointer instead of a new function.
Stefan Hajnoczi (1):
1 block: drop bs_snapshots global variable
Wenchao Xia (3):
2 block: move snapshot code in block.c to block/snapshot.c
3 block: move qmp and info dump related code to block/qapi.c
4 block: dump snapshot and image info to specified output
block.c | 313 --------------------------------------
block/Makefile.objs | 1 +
block/qapi.c | 366 +++++++++++++++++++++++++++++++++++++++++++++
block/snapshot.c | 157 +++++++++++++++++++
include/block/block.h | 28 +----
include/block/block_int.h | 1 +
include/block/qapi.h | 43 ++++++
include/block/snapshot.h | 53 +++++++
qemu-img.c | 163 +-------------------
savevm.c | 40 ++---
10 files changed, 646 insertions(+), 519 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] 25+ messages in thread
* [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
2013-05-25 3:09 [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Wenchao Xia
@ 2013-05-25 3:09 ` Wenchao Xia
2013-05-27 15:18 ` Kevin Wolf
2013-05-27 15:25 ` Kevin Wolf
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 2/4] block: move snapshot code in block.c to block/snapshot.c Wenchao Xia
` (4 subsequent siblings)
5 siblings, 2 replies; 25+ messages in thread
From: Wenchao Xia @ 2013-05-25 3:09 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 3f87489..478a3b2 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] 25+ messages in thread
* [Qemu-devel] [PATCH V3 2/4] block: move snapshot code in block.c to block/snapshot.c
2013-05-25 3:09 [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Wenchao Xia
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable Wenchao Xia
@ 2013-05-25 3:09 ` Wenchao Xia
2013-05-25 12:17 ` Eric Blake
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 3/4] block: move qmp and info dump related code to block/qapi.c Wenchao Xia
` (3 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: Wenchao Xia @ 2013-05-25 3:09 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
All snapshot related code, except bdrv_snapshot_dump() and
bdrv_is_snapshot(), is moved to block/snapshot.c. bdrv_snapshot_dump()
will be moved to another file later. bdrv_is_snapshot() is not related
with internal snapshot. It also fixes small code style errors reported
by check script.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block.c | 100 -----------------------------
block/Makefile.objs | 1 +
block/snapshot.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 26 ++------
include/block/snapshot.h | 53 ++++++++++++++++
savevm.c | 23 +-------
6 files changed, 217 insertions(+), 143 deletions(-)
create mode 100644 block/snapshot.c
create mode 100644 include/block/snapshot.h
diff --git a/block.c b/block.c
index 478a3b2..67cafb7 100644
--- a/block.c
+++ b/block.c
@@ -3346,111 +3346,11 @@ 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/Makefile.objs b/block/Makefile.objs
index 5f0358a..8670999 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -4,6 +4,7 @@ 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 += vhdx.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..6c6d9de
--- /dev/null
+++ b/block/snapshot.c
@@ -0,0 +1,157 @@
+/*
+ * 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"
+#include "block/block_int.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;
+}
+
+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_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..725c7fb 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
new file mode 100644
index 0000000..eaf61f0
--- /dev/null
+++ b/include/block/snapshot.h
@@ -0,0 +1,53 @@
+/*
+ * 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"
+
+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_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
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] 25+ messages in thread
* [Qemu-devel] [PATCH V3 3/4] block: move qmp and info dump related code to block/qapi.c
2013-05-25 3:09 [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Wenchao Xia
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable Wenchao Xia
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 2/4] block: move snapshot code in block.c to block/snapshot.c Wenchao Xia
@ 2013-05-25 3:09 ` Wenchao Xia
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output Wenchao Xia
` (2 subsequent siblings)
5 siblings, 0 replies; 25+ messages in thread
From: Wenchao Xia @ 2013-05-25 3:09 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 collect_snapshots() and collect_image_info() are renamed, unused parameter
*fmt in collect_image_info() is removed.
5 code style fix.
To avoid conflict and tip better, macro in header file is BLOCK_QAPI_H
instead of QAPI_H. 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>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block.c | 185 -----------------------
block/Makefile.objs | 2 +-
block/qapi.c | 360 +++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 9 -
include/block/block_int.h | 1 +
include/block/qapi.h | 41 +++++
qemu-img.c | 156 +-------------------
savevm.c | 1 +
8 files changed, 408 insertions(+), 347 deletions(-)
create mode 100644 block/qapi.c
create mode 100644 include/block/qapi.h
diff --git a/block.c b/block.c
index 67cafb7..1fd2944 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)
@@ -3446,69 +3324,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/Makefile.objs b/block/Makefile.objs
index 8670999..2981654 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -4,7 +4,7 @@ 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 += vhdx.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..155e77e
--- /dev/null
+++ b/block/qapi.c
@@ -0,0 +1,360 @@
+/*
+ * 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"
+#include "qmp-commands.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;
+ }
+ }
+}
+
+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 725c7fb..6b5f2b7 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,8 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);
void bdrv_get_full_backing_filename(BlockDriverState *bs,
char *dest, size_t sz);
-BlockInfo *bdrv_query_info(BlockDriverState *s);
-BlockStats *bdrv_query_stats(const BlockDriverState *bs);
int bdrv_is_snapshot(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
new file mode 100644
index 0000000..55d1848
--- /dev/null
+++ b/include/block/qapi.h
@@ -0,0 +1,41 @@
+/*
+ * 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"
+#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 cd096a1..5d1e480 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,122 +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];
- 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;
@@ -1761,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);
}
}
@@ -1811,8 +1663,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;
diff --git a/savevm.c b/savevm.c
index ab53a02..f988e89 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] 25+ messages in thread
* [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output
2013-05-25 3:09 [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Wenchao Xia
` (2 preceding siblings ...)
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 3/4] block: move qmp and info dump related code to block/qapi.c Wenchao Xia
@ 2013-05-25 3:09 ` Wenchao Xia
2013-05-25 12:15 ` Eric Blake
2013-05-27 15:02 ` Luiz Capitulino
2013-05-27 15:41 ` [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Kevin Wolf
2013-06-04 12:46 ` Kevin Wolf
5 siblings, 2 replies; 25+ messages in thread
From: Wenchao Xia @ 2013-05-25 3:09 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini, Wenchao Xia
bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now,
some internal buffers are still used for format control, which have no
chance to be truncated. As a result, these two functions have no more issue
of truncation, and they can be used by both qemu and qemu-img with correct
parameter specified.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
block/qapi.c | 66 +++++++++++++++++++++++++++----------------------
include/block/qapi.h | 6 +++-
qemu-img.c | 9 ++++---
savevm.c | 7 +++--
4 files changed, 49 insertions(+), 39 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 155e77e..794dbf8 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)
+void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
+ QEMUSnapshotInfo *sn)
{
char buf1[128], date_buf[128], clock_buf[128];
struct tm tm;
@@ -267,9 +268,9 @@ 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");
+ func_fprintf(f,
+ "%-10s%-20s%7s%20s%15s",
+ "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
} else {
ti = sn->date_sec;
localtime_r(&ti, &tm);
@@ -282,17 +283,18 @@ 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);
+ func_fprintf(f,
+ "%-10s%-20s%7s%20s%15s",
+ sn->id_str, sn->name,
+ get_human_readable_size(buf1, sizeof(buf1),
+ sn->vm_state_size),
+ date_buf,
+ clock_buf);
}
- return buf;
}
-void bdrv_image_info_dump(ImageInfo *info)
+void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
+ ImageInfo *info)
{
char size_buf[128], dsize_buf[128];
if (!info->has_actual_size) {
@@ -302,43 +304,46 @@ 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);
+ func_fprintf(f,
+ "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");
+ func_fprintf(f, "encrypted: yes\n");
}
if (info->has_cluster_size) {
- printf("cluster_size: %" PRId64 "\n", info->cluster_size);
+ func_fprintf(f, "cluster_size: %" PRId64 "\n",
+ info->cluster_size);
}
if (info->has_dirty_flag && info->dirty_flag) {
- printf("cleanly shut down: no\n");
+ func_fprintf(f, "cleanly shut down: no\n");
}
if (info->has_backing_filename) {
- printf("backing file: %s", info->backing_filename);
+ func_fprintf(f, "backing file: %s", info->backing_filename);
if (info->has_full_backing_filename) {
- printf(" (actual path: %s)", info->full_backing_filename);
+ func_fprintf(f, " (actual path: %s)", info->full_backing_filename);
}
- putchar('\n');
+ func_fprintf(f, "\n");
if (info->has_backing_filename_format) {
- printf("backing file format: %s\n", info->backing_filename_format);
+ func_fprintf(f, "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));
+ func_fprintf(f, "Snapshot list:\n");
+ bdrv_snapshot_dump(func_fprintf, f, NULL);
+ func_fprintf(f, "\n");
/* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
* we convert to the block layer's native QEMUSnapshotInfo for now.
@@ -354,7 +359,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(func_fprintf, f, &sn);
+ func_fprintf(f, "\n");
}
}
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 55d1848..e6e568d 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -36,6 +36,8 @@ 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_image_info_dump(ImageInfo *info);
+void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
+ QEMUSnapshotInfo *sn);
+void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
+ ImageInfo *info);
#endif
diff --git a/qemu-img.c b/qemu-img.c
index 5d1e480..82c7977 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(fprintf, stdout, 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(fprintf, stdout, sn);
+ printf("\n");
}
g_free(sn_tab);
}
@@ -1613,7 +1614,7 @@ static void dump_human_image_info_list(ImageInfoList *list)
}
delim = true;
- bdrv_image_info_dump(elem->value);
+ bdrv_image_info_dump(fprintf, stdout, elem->value);
}
}
diff --git a/savevm.c b/savevm.c
index f988e89..9b9d037 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2540,7 +2540,6 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
int nb_sns, i, ret, available;
int total;
int *available_snapshots;
- char buf[256];
bs = find_vmstate_bs();
if (!bs) {
@@ -2583,10 +2582,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((fprintf_function)monitor_printf, mon, 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((fprintf_function)monitor_printf, mon, sn);
+ monitor_printf(mon, "\n");
}
} else {
monitor_printf(mon, "There is no suitable snapshot available\n");
--
1.7.1
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output Wenchao Xia
@ 2013-05-25 12:15 ` Eric Blake
2013-05-27 15:02 ` Luiz Capitulino
1 sibling, 0 replies; 25+ messages in thread
From: Eric Blake @ 2013-05-25 12:15 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]
On 05/24/2013 09:09 PM, Wenchao Xia wrote:
> bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now,
> some internal buffers are still used for format control, which have no
> chance to be truncated. As a result, these two functions have no more issue
> of truncation, and they can be used by both qemu and qemu-img with correct
> parameter specified.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block/qapi.c | 66 +++++++++++++++++++++++++++----------------------
> include/block/qapi.h | 6 +++-
> qemu-img.c | 9 ++++---
> savevm.c | 7 +++--
> 4 files changed, 49 insertions(+), 39 deletions(-)
> -char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
> +void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
> + QEMUSnapshotInfo *sn)
> {
> char buf1[128], date_buf[128], clock_buf[128];
> + func_fprintf(f,
> + "%-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);
Now that 'buf' is no longer in scope, it might be nice to rename 'buf1'
to something more meaningful; maybe size_buf to go along with the other
two named buffers. But the choice of naming doesn't impact the
correctness, so
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] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 2/4] block: move snapshot code in block.c to block/snapshot.c
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 2/4] block: move snapshot code in block.c to block/snapshot.c Wenchao Xia
@ 2013-05-25 12:17 ` Eric Blake
0 siblings, 0 replies; 25+ messages in thread
From: Eric Blake @ 2013-05-25 12:17 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
[-- Attachment #1: Type: text/plain, Size: 1051 bytes --]
On 05/24/2013 09:09 PM, Wenchao Xia wrote:
> All snapshot related code, except bdrv_snapshot_dump() and
> bdrv_is_snapshot(), is moved to block/snapshot.c. bdrv_snapshot_dump()
> will be moved to another file later. bdrv_is_snapshot() is not related
> with internal snapshot. It also fixes small code style errors reported
> by check script.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> block.c | 100 -----------------------------
> block/Makefile.objs | 1 +
> block/snapshot.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++
> include/block/block.h | 26 ++------
> include/block/snapshot.h | 53 ++++++++++++++++
> savevm.c | 23 +-------
> 6 files changed, 217 insertions(+), 143 deletions(-)
> create mode 100644 block/snapshot.c
> create mode 100644 include/block/snapshot.h
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] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output Wenchao Xia
2013-05-25 12:15 ` Eric Blake
@ 2013-05-27 15:02 ` Luiz Capitulino
2013-05-27 15:40 ` Kevin Wolf
1 sibling, 1 reply; 25+ messages in thread
From: Luiz Capitulino @ 2013-05-27 15:02 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, stefanha, qemu-devel, armbru, pbonzini
On Sat, 25 May 2013 11:09:45 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now,
> some internal buffers are still used for format control, which have no
> chance to be truncated. As a result, these two functions have no more issue
> of truncation, and they can be used by both qemu and qemu-img with correct
> parameter specified.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
I don't like the casting and the void pointers very much, but I can't
suggest anything better:
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> block/qapi.c | 66 +++++++++++++++++++++++++++----------------------
> include/block/qapi.h | 6 +++-
> qemu-img.c | 9 ++++---
> savevm.c | 7 +++--
> 4 files changed, 49 insertions(+), 39 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index 155e77e..794dbf8 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)
> +void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
> + QEMUSnapshotInfo *sn)
> {
> char buf1[128], date_buf[128], clock_buf[128];
> struct tm tm;
> @@ -267,9 +268,9 @@ 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");
> + func_fprintf(f,
> + "%-10s%-20s%7s%20s%15s",
> + "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
> } else {
> ti = sn->date_sec;
> localtime_r(&ti, &tm);
> @@ -282,17 +283,18 @@ 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);
> + func_fprintf(f,
> + "%-10s%-20s%7s%20s%15s",
> + sn->id_str, sn->name,
> + get_human_readable_size(buf1, sizeof(buf1),
> + sn->vm_state_size),
> + date_buf,
> + clock_buf);
> }
> - return buf;
> }
>
> -void bdrv_image_info_dump(ImageInfo *info)
> +void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
> + ImageInfo *info)
> {
> char size_buf[128], dsize_buf[128];
> if (!info->has_actual_size) {
> @@ -302,43 +304,46 @@ 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);
> + func_fprintf(f,
> + "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");
> + func_fprintf(f, "encrypted: yes\n");
> }
>
> if (info->has_cluster_size) {
> - printf("cluster_size: %" PRId64 "\n", info->cluster_size);
> + func_fprintf(f, "cluster_size: %" PRId64 "\n",
> + info->cluster_size);
> }
>
> if (info->has_dirty_flag && info->dirty_flag) {
> - printf("cleanly shut down: no\n");
> + func_fprintf(f, "cleanly shut down: no\n");
> }
>
> if (info->has_backing_filename) {
> - printf("backing file: %s", info->backing_filename);
> + func_fprintf(f, "backing file: %s", info->backing_filename);
> if (info->has_full_backing_filename) {
> - printf(" (actual path: %s)", info->full_backing_filename);
> + func_fprintf(f, " (actual path: %s)", info->full_backing_filename);
> }
> - putchar('\n');
> + func_fprintf(f, "\n");
> if (info->has_backing_filename_format) {
> - printf("backing file format: %s\n", info->backing_filename_format);
> + func_fprintf(f, "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));
> + func_fprintf(f, "Snapshot list:\n");
> + bdrv_snapshot_dump(func_fprintf, f, NULL);
> + func_fprintf(f, "\n");
>
> /* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
> * we convert to the block layer's native QEMUSnapshotInfo for now.
> @@ -354,7 +359,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(func_fprintf, f, &sn);
> + func_fprintf(f, "\n");
> }
> }
> }
> diff --git a/include/block/qapi.h b/include/block/qapi.h
> index 55d1848..e6e568d 100644
> --- a/include/block/qapi.h
> +++ b/include/block/qapi.h
> @@ -36,6 +36,8 @@ 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_image_info_dump(ImageInfo *info);
> +void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
> + QEMUSnapshotInfo *sn);
> +void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
> + ImageInfo *info);
> #endif
> diff --git a/qemu-img.c b/qemu-img.c
> index 5d1e480..82c7977 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(fprintf, stdout, 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(fprintf, stdout, sn);
> + printf("\n");
> }
> g_free(sn_tab);
> }
> @@ -1613,7 +1614,7 @@ static void dump_human_image_info_list(ImageInfoList *list)
> }
> delim = true;
>
> - bdrv_image_info_dump(elem->value);
> + bdrv_image_info_dump(fprintf, stdout, elem->value);
> }
> }
>
> diff --git a/savevm.c b/savevm.c
> index f988e89..9b9d037 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2540,7 +2540,6 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
> int nb_sns, i, ret, available;
> int total;
> int *available_snapshots;
> - char buf[256];
>
> bs = find_vmstate_bs();
> if (!bs) {
> @@ -2583,10 +2582,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((fprintf_function)monitor_printf, mon, 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((fprintf_function)monitor_printf, mon, sn);
> + monitor_printf(mon, "\n");
> }
> } else {
> monitor_printf(mon, "There is no suitable snapshot available\n");
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable Wenchao Xia
@ 2013-05-27 15:18 ` Kevin Wolf
2013-05-28 2:20 ` Wenchao Xia
2013-05-27 15:25 ` Kevin Wolf
1 sibling, 1 reply; 25+ messages in thread
From: Kevin Wolf @ 2013-05-27 15:18 UTC (permalink / raw)
To: Wenchao Xia
Cc: phrdina, stefanha, qemu-devel, lcapitulino, Stefan Hajnoczi,
pbonzini, armbru
Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
> 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>
I am not totally convinced by this approach, especially when it's nowhere
documented that the order of BDSes in the list is important. However, I
think the current code is suboptimal as well, so I'll apply this for
now.
What we really want is this: savevm lets you choose which image to save
the VM state to, and if you don't specify one, it automatically picks
one like today. loadvm checks all images and loads the VM state from the
image that has the VM state for this snapshot. If loadvm finds that it's
not exactly one image that has a VM state, this is an error condition.
Is anyone interested in implementing this?
Kevin
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable Wenchao Xia
2013-05-27 15:18 ` Kevin Wolf
@ 2013-05-27 15:25 ` Kevin Wolf
2013-05-28 2:24 ` Wenchao Xia
2013-05-28 7:46 ` Stefan Hajnoczi
1 sibling, 2 replies; 25+ messages in thread
From: Kevin Wolf @ 2013-05-27 15:25 UTC (permalink / raw)
To: Wenchao Xia
Cc: phrdina, stefanha, qemu-devel, lcapitulino, Stefan Hajnoczi,
pbonzini, armbru
Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
> 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 3f87489..478a3b2 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;
> - }
This hunk isn't replaced by any other code. If I understand correctly
what it's doing, it prevented you from saving the VM state to a
removable device, which would be allowed after this patch.
Is this a change we want to make? Why isn't it described in the commit
message?
Kevin
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output
2013-05-27 15:02 ` Luiz Capitulino
@ 2013-05-27 15:40 ` Kevin Wolf
2013-05-27 15:55 ` Luiz Capitulino
2013-05-28 2:09 ` Wenchao Xia
0 siblings, 2 replies; 25+ messages in thread
From: Kevin Wolf @ 2013-05-27 15:40 UTC (permalink / raw)
To: Luiz Capitulino
Cc: phrdina, stefanha, armbru, qemu-devel, pbonzini, Wenchao Xia
Am 27.05.2013 um 17:02 hat Luiz Capitulino geschrieben:
> On Sat, 25 May 2013 11:09:45 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
> > bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now,
> > some internal buffers are still used for format control, which have no
> > chance to be truncated. As a result, these two functions have no more issue
> > of truncation, and they can be used by both qemu and qemu-img with correct
> > parameter specified.
> >
> > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> I don't like the casting and the void pointers very much, but I can't
> suggest anything better:
Maybe introduce and use a different function pointer type that
explicitly takes void* instead of FILE*. You'd still have to cast the
function pointers (and now actually in all instances), but then at least
nobody can accidentally misinterpret a Monitor* as FILE*.
Kevin
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer
2013-05-25 3:09 [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Wenchao Xia
` (3 preceding siblings ...)
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output Wenchao Xia
@ 2013-05-27 15:41 ` Kevin Wolf
2013-05-30 2:41 ` Wenchao Xia
2013-06-04 12:46 ` Kevin Wolf
5 siblings, 1 reply; 25+ messages in thread
From: Kevin Wolf @ 2013-05-27 15:41 UTC (permalink / raw)
To: Wenchao Xia; +Cc: phrdina, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
> 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 info dumping function to get rid of buffer, avoid string truncation.
Posted comments on patch 1 and 4.
Patches 2 and 3 are:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output
2013-05-27 15:40 ` Kevin Wolf
@ 2013-05-27 15:55 ` Luiz Capitulino
2013-05-28 2:09 ` Wenchao Xia
1 sibling, 0 replies; 25+ messages in thread
From: Luiz Capitulino @ 2013-05-27 15:55 UTC (permalink / raw)
To: Kevin Wolf; +Cc: phrdina, stefanha, armbru, qemu-devel, pbonzini, Wenchao Xia
On Mon, 27 May 2013 17:40:59 +0200
Kevin Wolf <kwolf@redhat.com> wrote:
> Am 27.05.2013 um 17:02 hat Luiz Capitulino geschrieben:
> > On Sat, 25 May 2013 11:09:45 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> > > bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now,
> > > some internal buffers are still used for format control, which have no
> > > chance to be truncated. As a result, these two functions have no more issue
> > > of truncation, and they can be used by both qemu and qemu-img with correct
> > > parameter specified.
> > >
> > > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >
> > I don't like the casting and the void pointers very much, but I can't
> > suggest anything better:
>
> Maybe introduce and use a different function pointer type that
> explicitly takes void* instead of FILE*. You'd still have to cast the
> function pointers (and now actually in all instances), but then at least
> nobody can accidentally misinterpret a Monitor* as FILE*.
I'm not sure this helps much because we'd have two functions pointers
to choose (which fails the purpose of having the function pointer IMO) and
also, there's code in QEMU doing this cast/void dance already.
If we want a good solution, we'd have to find a better way to print to
the monitor and to standard output.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output
2013-05-27 15:40 ` Kevin Wolf
2013-05-27 15:55 ` Luiz Capitulino
@ 2013-05-28 2:09 ` Wenchao Xia
1 sibling, 0 replies; 25+ messages in thread
From: Wenchao Xia @ 2013-05-28 2:09 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, stefanha, qemu-devel, armbru, pbonzini, Luiz Capitulino
于 2013-5-27 23:40, Kevin Wolf 写道:
> Am 27.05.2013 um 17:02 hat Luiz Capitulino geschrieben:
>> On Sat, 25 May 2013 11:09:45 +0800
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>
>>> bdrv_snapshot_dump() and bdrv_image_info_dump() do not dump to a buffer now,
>>> some internal buffers are still used for format control, which have no
>>> chance to be truncated. As a result, these two functions have no more issue
>>> of truncation, and they can be used by both qemu and qemu-img with correct
>>> parameter specified.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>
>> I don't like the casting and the void pointers very much, but I can't
>> suggest anything better:
>
> Maybe introduce and use a different function pointer type that
> explicitly takes void* instead of FILE*. You'd still have to cast the
> function pointers (and now actually in all instances), but then at least
> nobody can accidentally misinterpret a Monitor* as FILE*.
>
> Kevin
>
You mean change
typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
to
typedef int (*fprintf_function)(void *out, const char *fmt, ...)
GCC_FMT_ATTR(2, 3);
?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
2013-05-27 15:18 ` Kevin Wolf
@ 2013-05-28 2:20 ` Wenchao Xia
0 siblings, 0 replies; 25+ messages in thread
From: Wenchao Xia @ 2013-05-28 2:20 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, stefanha, qemu-devel, lcapitulino, Stefan Hajnoczi,
pbonzini, armbru
于 2013-5-27 23:18, Kevin Wolf 写道:
> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
>> 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>
>
> I am not totally convinced by this approach, especially when it's nowhere
> documented that the order of BDSes in the list is important. However, I
> think the current code is suboptimal as well, so I'll apply this for
> now.
>
> What we really want is this: savevm lets you choose which image to save
> the VM state to, and if you don't specify one, it automatically picks
> one like today. loadvm checks all images and loads the VM state from the
> image that has the VM state for this snapshot. If loadvm finds that it's
> not exactly one image that has a VM state, this is an error condition.
>
> Is anyone interested in implementing this?
>
> Kevin
>
I think that can came up after Pavel's savevm transaction, which
add parameter telling which image to save vmstate. This patch simply
keep what it is now not touching that part.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
2013-05-27 15:25 ` Kevin Wolf
@ 2013-05-28 2:24 ` Wenchao Xia
2013-05-28 7:46 ` Stefan Hajnoczi
1 sibling, 0 replies; 25+ messages in thread
From: Wenchao Xia @ 2013-05-28 2:24 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, stefanha, qemu-devel, lcapitulino, Stefan Hajnoczi,
pbonzini, armbru
于 2013-5-27 23:25, Kevin Wolf 写道:
> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
>> 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 3f87489..478a3b2 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;
>> - }
>
> This hunk isn't replaced by any other code. If I understand correctly
> what it's doing, it prevented you from saving the VM state to a
> removable device, which would be allowed after this patch.
>
> Is this a change we want to make? Why isn't it described in the commit
> message?
How about adding it back in find_vmstate_bs()?
>
> Kevin
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
2013-05-27 15:25 ` Kevin Wolf
2013-05-28 2:24 ` Wenchao Xia
@ 2013-05-28 7:46 ` Stefan Hajnoczi
2013-05-29 7:54 ` Wenchao Xia
1 sibling, 1 reply; 25+ messages in thread
From: Stefan Hajnoczi @ 2013-05-28 7:46 UTC (permalink / raw)
To: Kevin Wolf
Cc: phrdina, stefanha, qemu-devel, lcapitulino, pbonzini,
Wenchao Xia, armbru
On Mon, May 27, 2013 at 05:25:16PM +0200, Kevin Wolf wrote:
> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
> > 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 3f87489..478a3b2 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;
> > - }
>
> This hunk isn't replaced by any other code. If I understand correctly
> what it's doing, it prevented you from saving the VM state to a
> removable device, which would be allowed after this patch.
>
> Is this a change we want to make? Why isn't it described in the commit
> message?
My understanding of this change is different. Markus is on CC so maybe
he can confirm.
The point of bs_snapshots = NULL is not to prevent you from saving
snapshots. It's simply to reset the pointer to the next snapshottable
device (used by bdrv_snapshots()).
See the bdrv_close() hunk above which does the same thing, as well as
bdrv_snapshots() which iterates bdrv_states and updates bs_snapshots.
So what this hunk does is to reset the bdrv_snapshots() iterator when a
removable device is hooked up to an emulated storage controller. It's
no longer necessary since this patch drops the global state
(bs_snapshots) and users will always iterate from scratch.
The whole stateful approach was not necessary.
Stefan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
2013-05-28 7:46 ` Stefan Hajnoczi
@ 2013-05-29 7:54 ` Wenchao Xia
2013-05-29 9:09 ` Stefan Hajnoczi
0 siblings, 1 reply; 25+ messages in thread
From: Wenchao Xia @ 2013-05-29 7:54 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
于 2013-5-28 15:46, Stefan Hajnoczi 写道:
> On Mon, May 27, 2013 at 05:25:16PM +0200, Kevin Wolf wrote:
>> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
>>> 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 3f87489..478a3b2 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;
>>> - }
>>
>> This hunk isn't replaced by any other code. If I understand correctly
>> what it's doing, it prevented you from saving the VM state to a
>> removable device, which would be allowed after this patch.
>>
>> Is this a change we want to make? Why isn't it described in the commit
>> message?
>
> My understanding of this change is different. Markus is on CC so maybe
> he can confirm.
>
> The point of bs_snapshots = NULL is not to prevent you from saving
> snapshots. It's simply to reset the pointer to the next snapshottable
> device (used by bdrv_snapshots()).
>
> See the bdrv_close() hunk above which does the same thing, as well as
> bdrv_snapshots() which iterates bdrv_states and updates bs_snapshots.
>
> So what this hunk does is to reset the bdrv_snapshots() iterator when a
> removable device is hooked up to an emulated storage controller. It's
> no longer necessary since this patch drops the global state
> (bs_snapshots) and users will always iterate from scratch.
>
> The whole stateful approach was not necessary.
>
> Stefan
>
Reading the code, original logic actually forbidded saving vmstate
into a removable device, now it is possible since find_vmstate_bs()
doesn't check it. How about forbid again in find_vmstate_bs()?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
2013-05-29 7:54 ` Wenchao Xia
@ 2013-05-29 9:09 ` Stefan Hajnoczi
2013-05-29 9:45 ` Wenchao Xia
0 siblings, 1 reply; 25+ messages in thread
From: Stefan Hajnoczi @ 2013-05-29 9:09 UTC (permalink / raw)
To: Wenchao Xia
Cc: Kevin Wolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
On Wed, May 29, 2013 at 03:54:46PM +0800, Wenchao Xia wrote:
> 于 2013-5-28 15:46, Stefan Hajnoczi 写道:
> >On Mon, May 27, 2013 at 05:25:16PM +0200, Kevin Wolf wrote:
> >>Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
> >>>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 3f87489..478a3b2 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;
> >>>- }
> >>
> >>This hunk isn't replaced by any other code. If I understand correctly
> >>what it's doing, it prevented you from saving the VM state to a
> >>removable device, which would be allowed after this patch.
> >>
> >>Is this a change we want to make? Why isn't it described in the commit
> >>message?
> >
> >My understanding of this change is different. Markus is on CC so maybe
> >he can confirm.
> >
> >The point of bs_snapshots = NULL is not to prevent you from saving
> >snapshots. It's simply to reset the pointer to the next snapshottable
> >device (used by bdrv_snapshots()).
> >
> >See the bdrv_close() hunk above which does the same thing, as well as
> >bdrv_snapshots() which iterates bdrv_states and updates bs_snapshots.
> >
> >So what this hunk does is to reset the bdrv_snapshots() iterator when a
> >removable device is hooked up to an emulated storage controller. It's
> >no longer necessary since this patch drops the global state
> >(bs_snapshots) and users will always iterate from scratch.
> >
> >The whole stateful approach was not necessary.
> >
> >Stefan
> >
> Reading the code, original logic actually forbidded saving vmstate
> into a removable device, now it is possible since find_vmstate_bs()
> doesn't check it. How about forbid again in find_vmstate_bs()?
I don't follow.
The hunk that Kevin quoted ensures that we restart bs_snapshots
iteration - this prevents us from choosing a device that has no medium
inserted (also see bdrv_can_snapshot() which checks for an inserted
medium).
This behavior is preserved in this patch because we now always restart
iteration and it's no longer necessary to reset global iterator state.
But I don't see any code that forbids/skips inserted, read-write
removable devices in the orginal code. Please point out the specific
piece of code you think has been dropped.
Stefan
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable
2013-05-29 9:09 ` Stefan Hajnoczi
@ 2013-05-29 9:45 ` Wenchao Xia
0 siblings, 0 replies; 25+ messages in thread
From: Wenchao Xia @ 2013-05-29 9:45 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
于 2013-5-29 17:09, Stefan Hajnoczi 写道:
> On Wed, May 29, 2013 at 03:54:46PM +0800, Wenchao Xia wrote:
>> 于 2013-5-28 15:46, Stefan Hajnoczi 写道:
>>> On Mon, May 27, 2013 at 05:25:16PM +0200, Kevin Wolf wrote:
>>>> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
>>>>> 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 3f87489..478a3b2 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;
>>>>> - }
>>>>
>>>> This hunk isn't replaced by any other code. If I understand correctly
>>>> what it's doing, it prevented you from saving the VM state to a
>>>> removable device, which would be allowed after this patch.
>>>>
>>>> Is this a change we want to make? Why isn't it described in the commit
>>>> message?
>>>
>>> My understanding of this change is different. Markus is on CC so maybe
>>> he can confirm.
>>>
>>> The point of bs_snapshots = NULL is not to prevent you from saving
>>> snapshots. It's simply to reset the pointer to the next snapshottable
>>> device (used by bdrv_snapshots()).
>>>
>>> See the bdrv_close() hunk above which does the same thing, as well as
>>> bdrv_snapshots() which iterates bdrv_states and updates bs_snapshots.
>>>
>>> So what this hunk does is to reset the bdrv_snapshots() iterator when a
>>> removable device is hooked up to an emulated storage controller. It's
>>> no longer necessary since this patch drops the global state
>>> (bs_snapshots) and users will always iterate from scratch.
>>>
>>> The whole stateful approach was not necessary.
>>>
>>> Stefan
>>>
>> Reading the code, original logic actually forbidded saving vmstate
>> into a removable device, now it is possible since find_vmstate_bs()
>> doesn't check it. How about forbid again in find_vmstate_bs()?
>
> I don't follow.
>
> The hunk that Kevin quoted ensures that we restart bs_snapshots
> iteration - this prevents us from choosing a device that has no medium
> inserted (also see bdrv_can_snapshot() which checks for an inserted
> medium).
>
> This behavior is preserved in this patch because we now always restart
> iteration and it's no longer necessary to reset global iterator state.
>
> But I don't see any code that forbids/skips inserted, read-write
> removable devices in the orginal code. Please point out the specific
> piece of code you think has been dropped.
>
> Stefan
>
OK, original code make sure following code is executed before
savm_vm(), after this patch following code will be always executed
before save_vm(), nothing need to be added.
while ((bs = bdrv_next(bs))) {
if (bdrv_can_snapshot(bs)) {
bs_snapshots = bs;
return bs;
}
}
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer
2013-05-27 15:41 ` [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Kevin Wolf
@ 2013-05-30 2:41 ` Wenchao Xia
2013-05-31 13:04 ` Wenchao Xia
0 siblings, 1 reply; 25+ messages in thread
From: Wenchao Xia @ 2013-05-30 2:41 UTC (permalink / raw)
To: Kevin Wolf; +Cc: phrdina, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
于 2013-5-27 23:41, Kevin Wolf 写道:
> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
>> 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 info dumping function to get rid of buffer, avoid string truncation.
>
> Posted comments on patch 1 and 4.
>
> Patches 2 and 3 are:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
It seems nothing need change, Kevin, do you think it can be merged?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer
2013-05-30 2:41 ` Wenchao Xia
@ 2013-05-31 13:04 ` Wenchao Xia
2013-05-31 13:19 ` Luiz Capitulino
0 siblings, 1 reply; 25+ messages in thread
From: Wenchao Xia @ 2013-05-31 13:04 UTC (permalink / raw)
To: Kevin Wolf; +Cc: phrdina, stefanha, armbru, qemu-devel, pbonzini, lcapitulino
于 2013-5-30 10:41, Wenchao Xia 写道:
> 于 2013-5-27 23:41, Kevin Wolf 写道:
>> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
>>> 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 info dumping function to get rid of buffer, avoid string
>>> truncation.
>>
>> Posted comments on patch 1 and 4.
>>
>> Patches 2 and 3 are:
>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>
> It seems nothing need change, Kevin, do you think it can be merged?
>
This serial blocks mine and Pavel's work, anything need to be
improved? Respin with
-typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
+typedef int (*fprintf_function)(void *out, const char *fmt, ...)
?
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer
2013-05-31 13:04 ` Wenchao Xia
@ 2013-05-31 13:19 ` Luiz Capitulino
2013-06-03 2:22 ` Wenchao Xia
0 siblings, 1 reply; 25+ messages in thread
From: Luiz Capitulino @ 2013-05-31 13:19 UTC (permalink / raw)
To: Wenchao Xia; +Cc: Kevin Wolf, phrdina, stefanha, qemu-devel, armbru, pbonzini
On Fri, 31 May 2013 21:04:10 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> 于 2013-5-30 10:41, Wenchao Xia 写道:
> > 于 2013-5-27 23:41, Kevin Wolf 写道:
> >> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
> >>> 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 info dumping function to get rid of buffer, avoid string
> >>> truncation.
> >>
> >> Posted comments on patch 1 and 4.
> >>
> >> Patches 2 and 3 are:
> >> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> >>
> > It seems nothing need change, Kevin, do you think it can be merged?
> >
> This serial blocks mine and Pavel's work, anything need to be
> improved? Respin with
> -typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
> +typedef int (*fprintf_function)(void *out, const char *fmt, ...)
> ?
As far as my review is concerned, I'm OK with your current version.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer
2013-05-31 13:19 ` Luiz Capitulino
@ 2013-06-03 2:22 ` Wenchao Xia
0 siblings, 0 replies; 25+ messages in thread
From: Wenchao Xia @ 2013-06-03 2:22 UTC (permalink / raw)
To: Luiz Capitulino
Cc: Kevin Wolf, phrdina, stefanha, qemu-devel, armbru, pbonzini
于 2013-5-31 21:19, Luiz Capitulino 写道:
> On Fri, 31 May 2013 21:04:10 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-5-30 10:41, Wenchao Xia 写道:
>>> 于 2013-5-27 23:41, Kevin Wolf 写道:
>>>> Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
>>>>> 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 info dumping function to get rid of buffer, avoid string
>>>>> truncation.
>>>>
>>>> Posted comments on patch 1 and 4.
>>>>
>>>> Patches 2 and 3 are:
>>>> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>>>>
>>> It seems nothing need change, Kevin, do you think it can be merged?
>>>
>> This serial blocks mine and Pavel's work, anything need to be
>> improved? Respin with
>> -typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
>> +typedef int (*fprintf_function)(void *out, const char *fmt, ...)
>> ?
>
> As far as my review is concerned, I'm OK with your current version.
>
Thanks Luiz. This series does code moves, so it have big chance to get
conflict when upstream changes, hope it not hang out too long... sorry
for pushing many times.
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer
2013-05-25 3:09 [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Wenchao Xia
` (4 preceding siblings ...)
2013-05-27 15:41 ` [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Kevin Wolf
@ 2013-06-04 12:46 ` Kevin Wolf
5 siblings, 0 replies; 25+ messages in thread
From: Kevin Wolf @ 2013-06-04 12:46 UTC (permalink / raw)
To: Wenchao Xia; +Cc: phrdina, stefanha, qemu-devel, lcapitulino, pbonzini, armbru
Am 25.05.2013 um 05:09 hat Wenchao Xia geschrieben:
> 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 info dumping function to get rid of buffer, avoid string truncation.
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2013-06-04 12:46 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-25 3:09 [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Wenchao Xia
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 1/4] block: drop bs_snapshots global variable Wenchao Xia
2013-05-27 15:18 ` Kevin Wolf
2013-05-28 2:20 ` Wenchao Xia
2013-05-27 15:25 ` Kevin Wolf
2013-05-28 2:24 ` Wenchao Xia
2013-05-28 7:46 ` Stefan Hajnoczi
2013-05-29 7:54 ` Wenchao Xia
2013-05-29 9:09 ` Stefan Hajnoczi
2013-05-29 9:45 ` Wenchao Xia
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 2/4] block: move snapshot code in block.c to block/snapshot.c Wenchao Xia
2013-05-25 12:17 ` Eric Blake
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 3/4] block: move qmp and info dump related code to block/qapi.c Wenchao Xia
2013-05-25 3:09 ` [Qemu-devel] [PATCH V3 4/4] block: dump snapshot and image info to specified output Wenchao Xia
2013-05-25 12:15 ` Eric Blake
2013-05-27 15:02 ` Luiz Capitulino
2013-05-27 15:40 ` Kevin Wolf
2013-05-27 15:55 ` Luiz Capitulino
2013-05-28 2:09 ` Wenchao Xia
2013-05-27 15:41 ` [Qemu-devel] [PATCH V3 0/4] qapi and snapshot code clean up in block layer Kevin Wolf
2013-05-30 2:41 ` Wenchao Xia
2013-05-31 13:04 ` Wenchao Xia
2013-05-31 13:19 ` Luiz Capitulino
2013-06-03 2:22 ` Wenchao Xia
2013-06-04 12:46 ` Kevin Wolf
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.