* [Qemu-devel] [PATCH V2 1/5] block: drop bs_snapshots global variable
2013-05-23 8:47 [Qemu-devel] [PATCH V2 0/5] qapi and snapshot code clean up in block layer Wenchao Xia
@ 2013-05-23 8:47 ` Wenchao Xia
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 2/5] block: move snapshot code in block.c to block/snapshot.c Wenchao Xia
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Wenchao Xia @ 2013-05-23 8:47 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] 14+ messages in thread* [Qemu-devel] [PATCH V2 2/5] block: move snapshot code in block.c to block/snapshot.c
2013-05-23 8:47 [Qemu-devel] [PATCH V2 0/5] qapi and snapshot code clean up in block layer Wenchao Xia
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 1/5] block: drop bs_snapshots global variable Wenchao Xia
@ 2013-05-23 8:47 ` Wenchao Xia
2013-05-24 11:35 ` Stefan Hajnoczi
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 3/5] block: move qmp and info dump related code to block/qapi.c Wenchao Xia
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Wenchao Xia @ 2013-05-23 8:47 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini,
Wenchao Xia
All snapshot related code, except bdrv_snapshot_dump(), is moved to
block/snapshot.c. bdrv_snapshot_dump() will be moved to another file later.
It also fixes small code style errors reported by check script.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block.c | 105 ------------------------------
block/Makefile.objs | 1 +
block/snapshot.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++
include/block/block.h | 28 ++------
include/block/snapshot.h | 54 +++++++++++++++
savevm.c | 23 +------
6 files changed, 224 insertions(+), 149 deletions(-)
create mode 100644 block/snapshot.c
create mode 100644 include/block/snapshot.h
diff --git a/block.c b/block.c
index 478a3b2..2561800 100644
--- a/block.c
+++ b/block.c
@@ -3346,111 +3346,6 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
return false;
}
-/**************************************************************/
-/* handling of snapshots */
-
-int bdrv_can_snapshot(BlockDriverState *bs)
-{
- BlockDriver *drv = bs->drv;
- if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
- return 0;
- }
-
- if (!drv->bdrv_snapshot_create) {
- if (bs->file != NULL) {
- return bdrv_can_snapshot(bs->file);
- }
- return 0;
- }
-
- return 1;
-}
-
-int bdrv_is_snapshot(BlockDriverState *bs)
-{
- return !!(bs->open_flags & BDRV_O_SNAPSHOT);
-}
-
-int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info)
-{
- BlockDriver *drv = bs->drv;
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_create)
- return drv->bdrv_snapshot_create(bs, sn_info);
- if (bs->file)
- return bdrv_snapshot_create(bs->file, sn_info);
- return -ENOTSUP;
-}
-
-int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id)
-{
- BlockDriver *drv = bs->drv;
- int ret, open_ret;
-
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_goto)
- return drv->bdrv_snapshot_goto(bs, snapshot_id);
-
- if (bs->file) {
- drv->bdrv_close(bs);
- ret = bdrv_snapshot_goto(bs->file, snapshot_id);
- open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
- if (open_ret < 0) {
- bdrv_delete(bs->file);
- bs->drv = NULL;
- return open_ret;
- }
- return ret;
- }
-
- return -ENOTSUP;
-}
-
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
-{
- BlockDriver *drv = bs->drv;
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_delete)
- return drv->bdrv_snapshot_delete(bs, snapshot_id);
- if (bs->file)
- return bdrv_snapshot_delete(bs->file, snapshot_id);
- return -ENOTSUP;
-}
-
-int bdrv_snapshot_list(BlockDriverState *bs,
- QEMUSnapshotInfo **psn_info)
-{
- BlockDriver *drv = bs->drv;
- if (!drv)
- return -ENOMEDIUM;
- if (drv->bdrv_snapshot_list)
- return drv->bdrv_snapshot_list(bs, psn_info);
- if (bs->file)
- return bdrv_snapshot_list(bs->file, psn_info);
- return -ENOTSUP;
-}
-
-int bdrv_snapshot_load_tmp(BlockDriverState *bs,
- const char *snapshot_name)
-{
- BlockDriver *drv = bs->drv;
- if (!drv) {
- return -ENOMEDIUM;
- }
- if (!bs->read_only) {
- return -EINVAL;
- }
- if (drv->bdrv_snapshot_load_tmp) {
- return drv->bdrv_snapshot_load_tmp(bs, snapshot_name);
- }
- return -ENOTSUP;
-}
-
/* backing_file can either be relative, or absolute, or a protocol. If it is
* relative, it must be relative to the chain. So, passing in bs->filename
* from a BDS as backing_file should not be done, as that may be relative to
diff --git a/block/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..cdc2a76
--- /dev/null
+++ b/block/snapshot.c
@@ -0,0 +1,162 @@
+/*
+ * 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_is_snapshot(BlockDriverState *bs)
+{
+ return !!(bs->open_flags & BDRV_O_SNAPSHOT);
+}
+
+int bdrv_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (drv->bdrv_snapshot_create) {
+ return drv->bdrv_snapshot_create(bs, sn_info);
+ }
+ if (bs->file) {
+ return bdrv_snapshot_create(bs->file, sn_info);
+ }
+ return -ENOTSUP;
+}
+
+int bdrv_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id)
+{
+ BlockDriver *drv = bs->drv;
+ int ret, open_ret;
+
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (drv->bdrv_snapshot_goto) {
+ return drv->bdrv_snapshot_goto(bs, snapshot_id);
+ }
+
+ if (bs->file) {
+ drv->bdrv_close(bs);
+ ret = bdrv_snapshot_goto(bs->file, snapshot_id);
+ open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
+ if (open_ret < 0) {
+ bdrv_delete(bs->file);
+ bs->drv = NULL;
+ return open_ret;
+ }
+ return ret;
+ }
+
+ return -ENOTSUP;
+}
+
+int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (drv->bdrv_snapshot_delete) {
+ return drv->bdrv_snapshot_delete(bs, snapshot_id);
+ }
+ if (bs->file) {
+ return bdrv_snapshot_delete(bs->file, snapshot_id);
+ }
+ return -ENOTSUP;
+}
+
+int bdrv_snapshot_list(BlockDriverState *bs,
+ QEMUSnapshotInfo **psn_info)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (drv->bdrv_snapshot_list) {
+ return drv->bdrv_snapshot_list(bs, psn_info);
+ }
+ if (bs->file) {
+ return bdrv_snapshot_list(bs->file, psn_info);
+ }
+ return -ENOTSUP;
+}
+
+int bdrv_snapshot_load_tmp(BlockDriverState *bs,
+ const char *snapshot_name)
+{
+ BlockDriver *drv = bs->drv;
+ if (!drv) {
+ return -ENOMEDIUM;
+ }
+ if (!bs->read_only) {
+ return -EINVAL;
+ }
+ if (drv->bdrv_snapshot_load_tmp) {
+ return drv->bdrv_snapshot_load_tmp(bs, snapshot_name);
+ }
+ return -ENOTSUP;
+}
diff --git a/include/block/block.h b/include/block/block.h
index 38263b9..fde6fb8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -7,6 +7,11 @@
#include "block/coroutine.h"
#include "qapi/qmp/qobject.h"
#include "qapi-types.h"
+/*
+ * snapshot.h is needed since bdrv_snapshot_dump(), it can be removed when the
+ * function is moved to other file.
+ */
+#include "block/snapshot.h"
/* block.c */
typedef struct BlockDriver BlockDriver;
@@ -27,17 +32,6 @@ typedef struct BlockFragInfo {
uint64_t compressed_clusters;
} BlockFragInfo;
-typedef struct QEMUSnapshotInfo {
- char id_str[128]; /* unique snapshot id */
- /* the following fields are informative. They are not needed for
- the consistency of the snapshot */
- char name[256]; /* user chosen name */
- uint64_t vm_state_size; /* VM state info size */
- uint32_t date_sec; /* UTC date of the snapshot */
- uint32_t date_nsec;
- uint64_t vm_clock_nsec; /* VM clock relative to boot */
-} QEMUSnapshotInfo;
-
/* Callbacks for block device models */
typedef struct BlockDevOps {
/*
@@ -330,17 +324,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
char *dest, size_t sz);
BlockInfo *bdrv_query_info(BlockDriverState *s);
BlockStats *bdrv_query_stats(const BlockDriverState *bs);
-int bdrv_can_snapshot(BlockDriverState *bs);
-int bdrv_is_snapshot(BlockDriverState *bs);
-int bdrv_snapshot_create(BlockDriverState *bs,
- QEMUSnapshotInfo *sn_info);
-int bdrv_snapshot_goto(BlockDriverState *bs,
- const char *snapshot_id);
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
-int bdrv_snapshot_list(BlockDriverState *bs,
- QEMUSnapshotInfo **psn_info);
-int bdrv_snapshot_load_tmp(BlockDriverState *bs,
- const char *snapshot_name);
+
char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
char *get_human_readable_size(char *buf, int buf_size, int64_t size);
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
new file mode 100644
index 0000000..96d4a50
--- /dev/null
+++ b/include/block/snapshot.h
@@ -0,0 +1,54 @@
+/*
+ * 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_is_snapshot(BlockDriverState *bs);
+int bdrv_snapshot_create(BlockDriverState *bs,
+ QEMUSnapshotInfo *sn_info);
+int bdrv_snapshot_goto(BlockDriverState *bs,
+ const char *snapshot_id);
+int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int bdrv_snapshot_list(BlockDriverState *bs,
+ QEMUSnapshotInfo **psn_info);
+int bdrv_snapshot_load_tmp(BlockDriverState *bs,
+ const char *snapshot_name);
+#endif
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] 14+ messages in thread* Re: [Qemu-devel] [PATCH V2 2/5] block: move snapshot code in block.c to block/snapshot.c
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 2/5] block: move snapshot code in block.c to block/snapshot.c Wenchao Xia
@ 2013-05-24 11:35 ` Stefan Hajnoczi
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2013-05-24 11:35 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, armbru, qemu-devel, pbonzini, lcapitulino
On Thu, May 23, 2013 at 04:47:13PM +0800, Wenchao Xia wrote:
> -int bdrv_is_snapshot(BlockDriverState *bs)
> -{
> - return !!(bs->open_flags & BDRV_O_SNAPSHOT);
> -}
No need to respin, but this function has nothing to do with the other
functions. This function is about -drive snapshot=on, the others are
about internal snapshots.
It should not go into block/snapshot.c. At least it should be
documented as having nothing to do with internal snapshots.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH V2 3/5] block: move qmp and info dump related code to block/qapi.c
2013-05-23 8:47 [Qemu-devel] [PATCH V2 0/5] qapi and snapshot code clean up in block layer Wenchao Xia
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 1/5] block: drop bs_snapshots global variable Wenchao Xia
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 2/5] block: move snapshot code in block.c to block/snapshot.c Wenchao Xia
@ 2013-05-23 8:47 ` Wenchao Xia
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf() Wenchao Xia
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
4 siblings, 0 replies; 14+ messages in thread
From: Wenchao Xia @ 2013-05-23 8:47 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini,
Wenchao Xia
This patch is a pure code move patch, except following modification:
1 get_human_readable_size() is changed to static function.
2 dump_human_image_info() is renamed to bdrv_image_info_dump().
3 in qmp_query_block() and qmp_query_blockstats, use bdrv_next(bs)
instead of direct traverse of global array 'bdrv_states'.
4 code style fix.
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 | 10 --
include/block/block_int.h | 1 +
include/block/qapi.h | 41 +++++
qemu-img.c | 156 +-------------------
savevm.c | 1 +
8 files changed, 408 insertions(+), 348 deletions(-)
create mode 100644 block/qapi.c
create mode 100644 include/block/qapi.h
diff --git a/block.c b/block.c
index 2561800..ca28c0d 100644
--- a/block.c
+++ b/block.c
@@ -3089,128 +3089,6 @@ int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
return data.ret;
}
-BlockInfo *bdrv_query_info(BlockDriverState *bs)
-{
- BlockInfo *info = g_malloc0(sizeof(*info));
- info->device = g_strdup(bs->device_name);
- info->type = g_strdup("unknown");
- info->locked = bdrv_dev_is_medium_locked(bs);
- info->removable = bdrv_dev_has_removable_media(bs);
-
- if (bdrv_dev_has_removable_media(bs)) {
- info->has_tray_open = true;
- info->tray_open = bdrv_dev_is_tray_open(bs);
- }
-
- if (bdrv_iostatus_is_enabled(bs)) {
- info->has_io_status = true;
- info->io_status = bs->iostatus;
- }
-
- if (bs->dirty_bitmap) {
- info->has_dirty = true;
- info->dirty = g_malloc0(sizeof(*info->dirty));
- info->dirty->count = bdrv_get_dirty_count(bs) * BDRV_SECTOR_SIZE;
- info->dirty->granularity =
- ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bs->dirty_bitmap));
- }
-
- if (bs->drv) {
- info->has_inserted = true;
- info->inserted = g_malloc0(sizeof(*info->inserted));
- info->inserted->file = g_strdup(bs->filename);
- info->inserted->ro = bs->read_only;
- info->inserted->drv = g_strdup(bs->drv->format_name);
- info->inserted->encrypted = bs->encrypted;
- info->inserted->encryption_key_missing = bdrv_key_required(bs);
-
- if (bs->backing_file[0]) {
- info->inserted->has_backing_file = true;
- info->inserted->backing_file = g_strdup(bs->backing_file);
- }
-
- info->inserted->backing_file_depth = bdrv_get_backing_file_depth(bs);
-
- if (bs->io_limits_enabled) {
- info->inserted->bps =
- bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL];
- info->inserted->bps_rd =
- bs->io_limits.bps[BLOCK_IO_LIMIT_READ];
- info->inserted->bps_wr =
- bs->io_limits.bps[BLOCK_IO_LIMIT_WRITE];
- info->inserted->iops =
- bs->io_limits.iops[BLOCK_IO_LIMIT_TOTAL];
- info->inserted->iops_rd =
- bs->io_limits.iops[BLOCK_IO_LIMIT_READ];
- info->inserted->iops_wr =
- bs->io_limits.iops[BLOCK_IO_LIMIT_WRITE];
- }
- }
- return info;
-}
-
-BlockInfoList *qmp_query_block(Error **errp)
-{
- BlockInfoList *head = NULL, **p_next = &head;
- BlockDriverState *bs;
-
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
- BlockInfoList *info = g_malloc0(sizeof(*info));
- info->value = bdrv_query_info(bs);
-
- *p_next = info;
- p_next = &info->next;
- }
-
- return head;
-}
-
-BlockStats *bdrv_query_stats(const BlockDriverState *bs)
-{
- BlockStats *s;
-
- s = g_malloc0(sizeof(*s));
-
- if (bs->device_name[0]) {
- s->has_device = true;
- s->device = g_strdup(bs->device_name);
- }
-
- s->stats = g_malloc0(sizeof(*s->stats));
- s->stats->rd_bytes = bs->nr_bytes[BDRV_ACCT_READ];
- s->stats->wr_bytes = bs->nr_bytes[BDRV_ACCT_WRITE];
- s->stats->rd_operations = bs->nr_ops[BDRV_ACCT_READ];
- s->stats->wr_operations = bs->nr_ops[BDRV_ACCT_WRITE];
- s->stats->wr_highest_offset = bs->wr_highest_sector * BDRV_SECTOR_SIZE;
- s->stats->flush_operations = bs->nr_ops[BDRV_ACCT_FLUSH];
- s->stats->wr_total_time_ns = bs->total_time_ns[BDRV_ACCT_WRITE];
- s->stats->rd_total_time_ns = bs->total_time_ns[BDRV_ACCT_READ];
- s->stats->flush_total_time_ns = bs->total_time_ns[BDRV_ACCT_FLUSH];
-
- if (bs->file) {
- s->has_parent = true;
- s->parent = bdrv_query_stats(bs->file);
- }
-
- return s;
-}
-
-BlockStatsList *qmp_query_blockstats(Error **errp)
-{
- BlockStatsList *head = NULL, **p_next = &head;
- BlockDriverState *bs;
-
- QTAILQ_FOREACH(bs, &bdrv_states, list) {
- BlockStatsList *info = g_malloc0(sizeof(*info));
- info->value = bdrv_query_stats(bs);
-
- *p_next = info;
- p_next = &info->next;
- }
-
- return head;
-}
-
const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
{
if (bs->backing_hd && bs->backing_hd->encrypted)
@@ -3441,69 +3319,6 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
return curr_bs;
}
-#define NB_SUFFIXES 4
-
-char *get_human_readable_size(char *buf, int buf_size, int64_t size)
-{
- static const char suffixes[NB_SUFFIXES] = "KMGT";
- int64_t base;
- int i;
-
- if (size <= 999) {
- snprintf(buf, buf_size, "%" PRId64, size);
- } else {
- base = 1024;
- for(i = 0; i < NB_SUFFIXES; i++) {
- if (size < (10 * base)) {
- snprintf(buf, buf_size, "%0.1f%c",
- (double)size / base,
- suffixes[i]);
- break;
- } else if (size < (1000 * base) || i == (NB_SUFFIXES - 1)) {
- snprintf(buf, buf_size, "%" PRId64 "%c",
- ((size + (base >> 1)) / base),
- suffixes[i]);
- break;
- }
- base = base * 1024;
- }
- }
- return buf;
-}
-
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
-{
- char buf1[128], date_buf[128], clock_buf[128];
- struct tm tm;
- time_t ti;
- int64_t secs;
-
- if (!sn) {
- snprintf(buf, buf_size,
- "%-10s%-20s%7s%20s%15s",
- "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
- } else {
- ti = sn->date_sec;
- localtime_r(&ti, &tm);
- strftime(date_buf, sizeof(date_buf),
- "%Y-%m-%d %H:%M:%S", &tm);
- secs = sn->vm_clock_nsec / 1000000000;
- snprintf(clock_buf, sizeof(clock_buf),
- "%02d:%02d:%02d.%03d",
- (int)(secs / 3600),
- (int)((secs / 60) % 60),
- (int)(secs % 60),
- (int)((sn->vm_clock_nsec / 1000000) % 1000));
- snprintf(buf, buf_size,
- "%-10s%-20s%7s%20s%15s",
- sn->id_str, sn->name,
- get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
- date_buf,
- clock_buf);
- }
- return buf;
-}
-
/**************************************************************/
/* async I/Os */
diff --git a/block/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 fde6fb8..35f7958 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -7,11 +7,6 @@
#include "block/coroutine.h"
#include "qapi/qmp/qobject.h"
#include "qapi-types.h"
-/*
- * snapshot.h is needed since bdrv_snapshot_dump(), it can be removed when the
- * function is moved to other file.
- */
-#include "block/snapshot.h"
/* block.c */
typedef struct BlockDriver BlockDriver;
@@ -322,12 +317,7 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
char *filename, int filename_size);
void bdrv_get_full_backing_filename(BlockDriverState *bs,
char *dest, size_t sz);
-BlockInfo *bdrv_query_info(BlockDriverState *s);
-BlockStats *bdrv_query_stats(const BlockDriverState *bs);
-
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
-char *get_human_readable_size(char *buf, int buf_size, int64_t size);
int path_is_absolute(const char *path);
void path_combine(char *dest, int dest_size,
const char *base_path,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6078dd3..ba52247 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -33,6 +33,7 @@
#include "qapi/qmp/qerror.h"
#include "monitor/monitor.h"
#include "qemu/hbitmap.h"
+#include "block/snapshot.h"
#define BLOCK_FLAG_ENCRYPT 1
#define BLOCK_FLAG_COMPAT6 4
diff --git a/include/block/qapi.h b/include/block/qapi.h
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] 14+ messages in thread* [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf()
2013-05-23 8:47 [Qemu-devel] [PATCH V2 0/5] qapi and snapshot code clean up in block layer Wenchao Xia
` (2 preceding siblings ...)
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 3/5] block: move qmp and info dump related code to block/qapi.c Wenchao Xia
@ 2013-05-23 8:47 ` Wenchao Xia
2013-05-23 15:05 ` Eric Blake
` (2 more replies)
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
4 siblings, 3 replies; 14+ messages in thread
From: Wenchao Xia @ 2013-05-23 8:47 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini,
Wenchao Xia
This function takes an input parameter *output, which can be specified by
caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(),
which is a static function added in this patch.
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
include/qemu/error-report.h | 13 +++++++++++++
util/qemu-error.c | 28 ++++++++++++++++++++++++++--
2 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index c902cc1..cdde78b 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -14,6 +14,8 @@
#define QEMU_ERROR_H
#include <stdarg.h>
+#include <stdio.h>
+#include "qemu/typedefs.h"
typedef struct Location {
/* all members are private to qemu-error.c */
@@ -32,6 +34,17 @@ void loc_set_none(void);
void loc_set_cmdline(char **argv, int idx, int cnt);
void loc_set_file(const char *fname, int lno);
+typedef struct QemuOutput {
+ enum { OUTPUT_STREAM, OUTPUT_MONITOR } kind;
+ union {
+ FILE *stream;
+ Monitor *monitor;
+ };
+} QemuOutput;
+
+void message_printf(const QemuOutput *output, const char *fmt, ...)
+ GCC_FMT_ATTR(2, 3);
+
void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 08a36f4..c7ff0a8 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -13,6 +13,25 @@
#include <stdio.h>
#include "monitor/monitor.h"
+static GCC_FMT_ATTR(2, 0)
+void message_vprintf(const QemuOutput *output, const char *fmt, va_list ap)
+{
+ if (output->kind == OUTPUT_STREAM) {
+ vfprintf(output->stream, fmt, ap);
+ } else if (output->kind == OUTPUT_MONITOR) {
+ monitor_vprintf(output->monitor, fmt, ap);
+ }
+}
+
+void message_printf(const QemuOutput *output, const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ message_vprintf(output, fmt, ap);
+ va_end(ap);
+}
+
/*
* Print to current monitor if we have one, else to stderr.
* TODO should return int, so callers can calculate width, but that
@@ -20,11 +39,16 @@
*/
void error_vprintf(const char *fmt, va_list ap)
{
+ QemuOutput output;
+
if (cur_mon) {
- monitor_vprintf(cur_mon, fmt, ap);
+ output.kind = OUTPUT_MONITOR;
+ output.monitor = cur_mon;
} else {
- vfprintf(stderr, fmt, ap);
+ output.kind = OUTPUT_STREAM;
+ output.stream = stderr;
}
+ message_vprintf(&output, fmt, ap);
}
/*
--
1.7.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf()
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf() Wenchao Xia
@ 2013-05-23 15:05 ` Eric Blake
2013-05-24 1:41 ` Wenchao Xia
2013-05-23 17:14 ` Luiz Capitulino
2013-05-24 11:45 ` Stefan Hajnoczi
2 siblings, 1 reply; 14+ messages in thread
From: Eric Blake @ 2013-05-23 15:05 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini,
armbru
[-- Attachment #1: Type: text/plain, Size: 2136 bytes --]
On 05/23/2013 02:47 AM, Wenchao Xia wrote:
> This function takes an input parameter *output, which can be specified by
> caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(),
> which is a static function added in this patch.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> include/qemu/error-report.h | 13 +++++++++++++
> util/qemu-error.c | 28 ++++++++++++++++++++++++++--
> 2 files changed, 39 insertions(+), 2 deletions(-)
>
> +++ b/util/qemu-error.c
> @@ -13,6 +13,25 @@
> #include <stdio.h>
> #include "monitor/monitor.h"
>
> +static GCC_FMT_ATTR(2, 0)
> +void message_vprintf(const QemuOutput *output, const char *fmt, va_list ap)
> +{
> + if (output->kind == OUTPUT_STREAM) {
> + vfprintf(output->stream, fmt, ap);
> + } else if (output->kind == OUTPUT_MONITOR) {
> + monitor_vprintf(output->monitor, fmt, ap);
> + }
Should you use a switch statement here, instead of open coding all
possible enum values? But that's cosmetic.
More importantly, I think this function should return an int, whose
value is the value of vfprintf. On the monitor_vfprintf arm, it could
return 0 for now (or, you could unravel THAT problem and fix
monitor_vfprintf to return an output count, but that sounds like a
bigger task).
> +}
> +
> +void message_printf(const QemuOutput *output, const char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + message_vprintf(output, fmt, ap);
> + va_end(ap);
This function should also return int.
> +}
> +
> /*
> * Print to current monitor if we have one, else to stderr.
> * TODO should return int, so callers can calculate width, but that
And by fixing the underlying function to return int, you could finally
get rid of this TODO.
Given that the int return problem is pre-existing, and probably deserves
its own series, I'm fine with taking this patch as-is.
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] 14+ messages in thread* Re: [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf()
2013-05-23 15:05 ` Eric Blake
@ 2013-05-24 1:41 ` Wenchao Xia
0 siblings, 0 replies; 14+ messages in thread
From: Wenchao Xia @ 2013-05-24 1:41 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini,
armbru
于 2013-5-23 23:05, Eric Blake 写道:
> On 05/23/2013 02:47 AM, Wenchao Xia wrote:
>> This function takes an input parameter *output, which can be specified by
>> caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(),
>> which is a static function added in this patch.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>> include/qemu/error-report.h | 13 +++++++++++++
>> util/qemu-error.c | 28 ++++++++++++++++++++++++++--
>> 2 files changed, 39 insertions(+), 2 deletions(-)
>>
>> +++ b/util/qemu-error.c
>> @@ -13,6 +13,25 @@
>> #include <stdio.h>
>> #include "monitor/monitor.h"
>>
>> +static GCC_FMT_ATTR(2, 0)
>> +void message_vprintf(const QemuOutput *output, const char *fmt, va_list ap)
>> +{
>> + if (output->kind == OUTPUT_STREAM) {
>> + vfprintf(output->stream, fmt, ap);
>> + } else if (output->kind == OUTPUT_MONITOR) {
>> + monitor_vprintf(output->monitor, fmt, ap);
>> + }
>
> Should you use a switch statement here, instead of open coding all
> possible enum values? But that's cosmetic.
>
> More importantly, I think this function should return an int, whose
> value is the value of vfprintf. On the monitor_vfprintf arm, it could
> return 0 for now (or, you could unravel THAT problem and fix
> monitor_vfprintf to return an output count, but that sounds like a
> bigger task).
>
>> +}
>> +
>> +void message_printf(const QemuOutput *output, const char *fmt, ...)
>> +{
>> + va_list ap;
>> +
>> + va_start(ap, fmt);
>> + message_vprintf(output, fmt, ap);
>> + va_end(ap);
>
> This function should also return int.
>
>> +}
>> +
>> /*
>> * Print to current monitor if we have one, else to stderr.
>> * TODO should return int, so callers can calculate width, but that
>
> And by fixing the underlying function to return int, you could finally
> get rid of this TODO.
>
> Given that the int return problem is pre-existing, and probably deserves
> its own series, I'm fine with taking this patch as-is.
>
It may not have much meaning of returning int now, since underlining
function do not support it so no caller can benefit from it. I think a
series later for that is better, thanks for your reviewing.
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf()
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf() Wenchao Xia
2013-05-23 15:05 ` Eric Blake
@ 2013-05-23 17:14 ` Luiz Capitulino
2013-05-24 11:45 ` Stefan Hajnoczi
2 siblings, 0 replies; 14+ messages in thread
From: Luiz Capitulino @ 2013-05-23 17:14 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, stefanha, qemu-devel, armbru, pbonzini
On Thu, 23 May 2013 16:47:15 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> This function takes an input parameter *output, which can be specified by
> caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(),
> which is a static function added in this patch.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
This solution looks good to me:
Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
> include/qemu/error-report.h | 13 +++++++++++++
> util/qemu-error.c | 28 ++++++++++++++++++++++++++--
> 2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index c902cc1..cdde78b 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -14,6 +14,8 @@
> #define QEMU_ERROR_H
>
> #include <stdarg.h>
> +#include <stdio.h>
> +#include "qemu/typedefs.h"
>
> typedef struct Location {
> /* all members are private to qemu-error.c */
> @@ -32,6 +34,17 @@ void loc_set_none(void);
> void loc_set_cmdline(char **argv, int idx, int cnt);
> void loc_set_file(const char *fname, int lno);
>
> +typedef struct QemuOutput {
> + enum { OUTPUT_STREAM, OUTPUT_MONITOR } kind;
> + union {
> + FILE *stream;
> + Monitor *monitor;
> + };
> +} QemuOutput;
> +
> +void message_printf(const QemuOutput *output, const char *fmt, ...)
> + GCC_FMT_ATTR(2, 3);
> +
> void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
> void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index 08a36f4..c7ff0a8 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -13,6 +13,25 @@
> #include <stdio.h>
> #include "monitor/monitor.h"
>
> +static GCC_FMT_ATTR(2, 0)
> +void message_vprintf(const QemuOutput *output, const char *fmt, va_list ap)
> +{
> + if (output->kind == OUTPUT_STREAM) {
> + vfprintf(output->stream, fmt, ap);
> + } else if (output->kind == OUTPUT_MONITOR) {
> + monitor_vprintf(output->monitor, fmt, ap);
> + }
> +}
> +
> +void message_printf(const QemuOutput *output, const char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + message_vprintf(output, fmt, ap);
> + va_end(ap);
> +}
> +
> /*
> * Print to current monitor if we have one, else to stderr.
> * TODO should return int, so callers can calculate width, but that
> @@ -20,11 +39,16 @@
> */
> void error_vprintf(const char *fmt, va_list ap)
> {
> + QemuOutput output;
> +
> if (cur_mon) {
> - monitor_vprintf(cur_mon, fmt, ap);
> + output.kind = OUTPUT_MONITOR;
> + output.monitor = cur_mon;
> } else {
> - vfprintf(stderr, fmt, ap);
> + output.kind = OUTPUT_STREAM;
> + output.stream = stderr;
> }
> + message_vprintf(&output, fmt, ap);
> }
>
> /*
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf()
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf() Wenchao Xia
2013-05-23 15:05 ` Eric Blake
2013-05-23 17:14 ` Luiz Capitulino
@ 2013-05-24 11:45 ` Stefan Hajnoczi
2 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2013-05-24 11:45 UTC (permalink / raw)
To: Wenchao Xia; +Cc: kwolf, phrdina, armbru, qemu-devel, pbonzini, lcapitulino
On Thu, May 23, 2013 at 04:47:15PM +0800, Wenchao Xia wrote:
> This function takes an input parameter *output, which can be specified by
> caller as stderr, stdout or a monitor. error_vprintf() now calls message_vprintf(),
> which is a static function added in this patch.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> include/qemu/error-report.h | 13 +++++++++++++
> util/qemu-error.c | 28 ++++++++++++++++++++++++++--
> 2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index c902cc1..cdde78b 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -14,6 +14,8 @@
> #define QEMU_ERROR_H
>
> #include <stdarg.h>
> +#include <stdio.h>
> +#include "qemu/typedefs.h"
>
> typedef struct Location {
> /* all members are private to qemu-error.c */
> @@ -32,6 +34,17 @@ void loc_set_none(void);
> void loc_set_cmdline(char **argv, int idx, int cnt);
> void loc_set_file(const char *fname, int lno);
>
> +typedef struct QemuOutput {
> + enum { OUTPUT_STREAM, OUTPUT_MONITOR } kind;
> + union {
> + FILE *stream;
> + Monitor *monitor;
> + };
> +} QemuOutput;
> +
> +void message_printf(const QemuOutput *output, const char *fmt, ...)
> + GCC_FMT_ATTR(2, 3);
This is introducing a slightly different solution for fprintf_function,
which is already widely used:
$ git grep fprintf_function | wc -l
101
Please reuse fprintf_function.
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-23 8:47 [Qemu-devel] [PATCH V2 0/5] qapi and snapshot code clean up in block layer Wenchao Xia
` (3 preceding siblings ...)
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 4/5] util: add new function message_printf() Wenchao Xia
@ 2013-05-23 8:47 ` Wenchao Xia
2013-05-23 15:31 ` Eric Blake
4 siblings, 1 reply; 14+ messages in thread
From: Wenchao Xia @ 2013-05-23 8:47 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, phrdina, stefanha, armbru, lcapitulino, pbonzini,
Wenchao Xia
Buffer is not used now so the string would not be truncated any more. 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 | 65 +++++++++++++++++++++++++++-----------------------
include/block/qapi.h | 5 ++-
qemu-img.c | 15 +++++++----
savevm.c | 11 ++++++--
4 files changed, 55 insertions(+), 41 deletions(-)
diff --git a/block/qapi.c b/block/qapi.c
index 155e77e..2e083f8 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -259,7 +259,7 @@ 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(const QemuOutput *output, QEMUSnapshotInfo *sn)
{
char buf1[128], date_buf[128], clock_buf[128];
struct tm tm;
@@ -267,9 +267,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");
+ message_printf(output,
+ "%-10s%-20s%7s%20s%15s",
+ "ID", "TAG", "VM SIZE", "DATE", "VM CLOCK");
} else {
ti = sn->date_sec;
localtime_r(&ti, &tm);
@@ -282,17 +282,17 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
(int)((secs / 60) % 60),
(int)(secs % 60),
(int)((sn->vm_clock_nsec / 1000000) % 1000));
- snprintf(buf, buf_size,
- "%-10s%-20s%7s%20s%15s",
- sn->id_str, sn->name,
- get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
- date_buf,
- clock_buf);
+ message_printf(output,
+ "%-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(const QemuOutput *output, ImageInfo *info)
{
char size_buf[128], dsize_buf[128];
if (!info->has_actual_size) {
@@ -302,43 +302,47 @@ void bdrv_image_info_dump(ImageInfo *info)
info->actual_size);
}
get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
- printf("image: %s\n"
- "file format: %s\n"
- "virtual size: %s (%" PRId64 " bytes)\n"
- "disk size: %s\n",
- info->filename, info->format, size_buf,
- info->virtual_size,
- dsize_buf);
+ message_printf(output,
+ "image: %s\n"
+ "file format: %s\n"
+ "virtual size: %s (%" PRId64 " bytes)\n"
+ "disk size: %s\n",
+ info->filename, info->format, size_buf,
+ info->virtual_size,
+ dsize_buf);
if (info->has_encrypted && info->encrypted) {
- printf("encrypted: yes\n");
+ message_printf(output, "encrypted: yes\n");
}
if (info->has_cluster_size) {
- printf("cluster_size: %" PRId64 "\n", info->cluster_size);
+ message_printf(output, "cluster_size: %" PRId64 "\n",
+ info->cluster_size);
}
if (info->has_dirty_flag && info->dirty_flag) {
- printf("cleanly shut down: no\n");
+ message_printf(output, "cleanly shut down: no\n");
}
if (info->has_backing_filename) {
- printf("backing file: %s", info->backing_filename);
+ message_printf(output, "backing file: %s", info->backing_filename);
if (info->has_full_backing_filename) {
- printf(" (actual path: %s)", info->full_backing_filename);
+ message_printf(output, " (actual path: %s)",
+ info->full_backing_filename);
}
- putchar('\n');
+ message_printf(output, "\n");
if (info->has_backing_filename_format) {
- printf("backing file format: %s\n", info->backing_filename_format);
+ message_printf(output, "backing file format: %s\n",
+ info->backing_filename_format);
}
}
if (info->has_snapshots) {
SnapshotInfoList *elem;
- char buf[256];
- printf("Snapshot list:\n");
- printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ message_printf(output, "Snapshot list:\n");
+ bdrv_snapshot_dump(output, NULL);
+ message_printf(output, "\n");
/* Ideally bdrv_snapshot_dump() would operate on SnapshotInfoList but
* we convert to the block layer's native QEMUSnapshotInfo for now.
@@ -354,7 +358,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(output, &sn);
+ message_printf(output, "\n");
}
}
}
diff --git a/include/block/qapi.h b/include/block/qapi.h
index 55d1848..9c80118 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -28,6 +28,7 @@
#include "qapi-types.h"
#include "block/block.h"
#include "block/snapshot.h"
+#include "qemu/error-report.h"
void bdrv_collect_snapshots(BlockDriverState *bs , ImageInfo *info);
void bdrv_collect_image_info(BlockDriverState *bs,
@@ -36,6 +37,6 @@ void bdrv_collect_image_info(BlockDriverState *bs,
BlockInfo *bdrv_query_info(BlockDriverState *s);
BlockStats *bdrv_query_stats(const BlockDriverState *bs);
-char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
-void bdrv_image_info_dump(ImageInfo *info);
+void bdrv_snapshot_dump(const QemuOutput *output, QEMUSnapshotInfo *sn);
+void bdrv_image_info_dump(const QemuOutput *output, ImageInfo *info);
#endif
diff --git a/qemu-img.c b/qemu-img.c
index 5d1e480..14901cf 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1554,16 +1554,18 @@ static void dump_snapshots(BlockDriverState *bs)
{
QEMUSnapshotInfo *sn_tab, *sn;
int nb_sns, i;
- char buf[256];
+ QemuOutput output = {OUTPUT_STREAM, {stdout,} };
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));
+ message_printf(&output, "Snapshot list:\n");
+ bdrv_snapshot_dump(&output, NULL);
+ message_printf(&output, "\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(&output, sn);
+ message_printf(&output, "\n");
}
g_free(sn_tab);
}
@@ -1606,14 +1608,15 @@ static void dump_human_image_info_list(ImageInfoList *list)
{
ImageInfoList *elem;
bool delim = false;
+ QemuOutput output = {OUTPUT_STREAM, {stdout,} };
for (elem = list; elem; elem = elem->next) {
if (delim) {
- printf("\n");
+ message_printf(&output, "\n");
}
delim = true;
- bdrv_image_info_dump(elem->value);
+ bdrv_image_info_dump(&output, elem->value);
}
}
diff --git a/savevm.c b/savevm.c
index f988e89..93414e1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2540,7 +2540,10 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
int nb_sns, i, ret, available;
int total;
int *available_snapshots;
- char buf[256];
+ QemuOutput output;
+
+ output.kind = OUTPUT_MONITOR;
+ output.monitor = mon;
bs = find_vmstate_bs();
if (!bs) {
@@ -2583,10 +2586,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(&output, 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(&output, 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] 14+ messages in thread* Re: [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-23 8:47 ` [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump() Wenchao Xia
@ 2013-05-23 15:31 ` Eric Blake
2013-05-24 1:48 ` Wenchao Xia
0 siblings, 1 reply; 14+ messages in thread
From: Eric Blake @ 2013-05-23 15:31 UTC (permalink / raw)
To: Wenchao Xia
Cc: kwolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini,
armbru
[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]
On 05/23/2013 02:47 AM, Wenchao Xia wrote:
> Buffer is not used now so the string would not be truncated any more. 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 | 65 +++++++++++++++++++++++++++-----------------------
> include/block/qapi.h | 5 ++-
> qemu-img.c | 15 +++++++----
> savevm.c | 11 ++++++--
> 4 files changed, 55 insertions(+), 41 deletions(-)
>
> @@ -282,17 +282,17 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
> (int)((secs / 60) % 60),
> (int)(secs % 60),
> (int)((sn->vm_clock_nsec / 1000000) % 1000));
> - snprintf(buf, buf_size,
> - "%-10s%-20s%7s%20s%15s",
> - sn->id_str, sn->name,
> - get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
> - date_buf,
> - clock_buf);
> + message_printf(output,
You got rid of ONE buffer...
> + "%-10s%-20s%7s%20s%15s",
> + sn->id_str, sn->name,
> + get_human_readable_size(buf1, sizeof(buf1),
...but what is this other buffer still doing? get_human_readable_size
needs to be converted to use QemuOutput.
> +void bdrv_image_info_dump(const QemuOutput *output, ImageInfo *info)
> {
> char size_buf[128], dsize_buf[128];
Why do we still need size_buf and dsize_buf?
> if (!info->has_actual_size) {
> @@ -302,43 +302,47 @@ void bdrv_image_info_dump(ImageInfo *info)
> info->actual_size);
> }
> get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
Again, get_human_readable_size should be converted to use QemuOutput.
> +++ b/qemu-img.c
> @@ -1554,16 +1554,18 @@ static void dump_snapshots(BlockDriverState *bs)
> {
> QEMUSnapshotInfo *sn_tab, *sn;
> int nb_sns, i;
> - char buf[256];
> + QemuOutput output = {OUTPUT_STREAM, {stdout,} };
This is relying on C99's rule that a union is initialized by its first
named member. But I think it might be more readable as:
output = { .kind = OUTPUT_STREAM, .stream = stdout };
not to mention that you will HAVE to use a designator to ever initialize
the monitor element of the union in any parallel code that favors the
monitor.
Hmm, does C99 even allow anonymous unions, or is that a gcc extension?
Overall, I like the direction this is headed. The conversion looks
reasonable, although it didn't quite go far enough for getting rid of
buffers.
--
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] 14+ messages in thread* Re: [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-23 15:31 ` Eric Blake
@ 2013-05-24 1:48 ` Wenchao Xia
2013-05-24 2:31 ` Wenchao Xia
0 siblings, 1 reply; 14+ messages in thread
From: Wenchao Xia @ 2013-05-24 1:48 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini,
armbru
于 2013-5-23 23:31, Eric Blake 写道:
> On 05/23/2013 02:47 AM, Wenchao Xia wrote:
>> Buffer is not used now so the string would not be truncated any more. 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 | 65 +++++++++++++++++++++++++++-----------------------
>> include/block/qapi.h | 5 ++-
>> qemu-img.c | 15 +++++++----
>> savevm.c | 11 ++++++--
>> 4 files changed, 55 insertions(+), 41 deletions(-)
>>
>
>> @@ -282,17 +282,17 @@ char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn)
>> (int)((secs / 60) % 60),
>> (int)(secs % 60),
>> (int)((sn->vm_clock_nsec / 1000000) % 1000));
>> - snprintf(buf, buf_size,
>> - "%-10s%-20s%7s%20s%15s",
>> - sn->id_str, sn->name,
>> - get_human_readable_size(buf1, sizeof(buf1), sn->vm_state_size),
>> - date_buf,
>> - clock_buf);
>> + message_printf(output,
>
> You got rid of ONE buffer...
>
>> + "%-10s%-20s%7s%20s%15s",
>> + sn->id_str, sn->name,
>> + get_human_readable_size(buf1, sizeof(buf1),
>
> ...but what is this other buffer still doing? get_human_readable_size
> needs to be converted to use QemuOutput.
>
>> +void bdrv_image_info_dump(const QemuOutput *output, ImageInfo *info)
>> {
>> char size_buf[128], dsize_buf[128];
>
> Why do we still need size_buf and dsize_buf?
>
>> if (!info->has_actual_size) {
>> @@ -302,43 +302,47 @@ void bdrv_image_info_dump(ImageInfo *info)
>> info->actual_size);
>> }
>> get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
>
> Again, get_human_readable_size should be converted to use QemuOutput.
>
They do not likely have a chance to be truncated, so have not change
them. I will convert them also in next version.
>> +++ b/qemu-img.c
>> @@ -1554,16 +1554,18 @@ static void dump_snapshots(BlockDriverState *bs)
>> {
>> QEMUSnapshotInfo *sn_tab, *sn;
>> int nb_sns, i;
>> - char buf[256];
>> + QemuOutput output = {OUTPUT_STREAM, {stdout,} };
>
> This is relying on C99's rule that a union is initialized by its first
> named member. But I think it might be more readable as:
>
> output = { .kind = OUTPUT_STREAM, .stream = stdout };
>
> not to mention that you will HAVE to use a designator to ever initialize
> the monitor element of the union in any parallel code that favors the
> monitor.
This solve the initialization issue, will use it, thank u for the tip.
>
> Hmm, does C99 even allow anonymous unions, or is that a gcc extension?
>
> Overall, I like the direction this is headed. The conversion looks
> reasonable, although it didn't quite go far enough for getting rid of
> buffers.
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] [PATCH V2 5/5] block: dump to specified output for bdrv_snapshot_dump() and bdrv_image_info_dump()
2013-05-24 1:48 ` Wenchao Xia
@ 2013-05-24 2:31 ` Wenchao Xia
0 siblings, 0 replies; 14+ messages in thread
From: Wenchao Xia @ 2013-05-24 2:31 UTC (permalink / raw)
To: Eric Blake
Cc: kwolf, phrdina, stefanha, qemu-devel, lcapitulino, pbonzini,
armbru
于 2013-5-24 9:48, Wenchao Xia 写道:
> 于 2013-5-23 23:31, Eric Blake 写道:
>> On 05/23/2013 02:47 AM, Wenchao Xia wrote:
>>> Buffer is not used now so the string would not be truncated any more.
>>> 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 | 65
>>> +++++++++++++++++++++++++++-----------------------
>>> include/block/qapi.h | 5 ++-
>>> qemu-img.c | 15 +++++++----
>>> savevm.c | 11 ++++++--
>>> 4 files changed, 55 insertions(+), 41 deletions(-)
>>>
>>
>>> @@ -282,17 +282,17 @@ char *bdrv_snapshot_dump(char *buf, int
>>> buf_size, QEMUSnapshotInfo *sn)
>>> (int)((secs / 60) % 60),
>>> (int)(secs % 60),
>>> (int)((sn->vm_clock_nsec / 1000000) % 1000));
>>> - snprintf(buf, buf_size,
>>> - "%-10s%-20s%7s%20s%15s",
>>> - sn->id_str, sn->name,
>>> - get_human_readable_size(buf1, sizeof(buf1),
>>> sn->vm_state_size),
>>> - date_buf,
>>> - clock_buf);
>>> + message_printf(output,
>>
>> You got rid of ONE buffer...
>>
>>> + "%-10s%-20s%7s%20s%15s",
>>> + sn->id_str, sn->name,
>>> + get_human_readable_size(buf1, sizeof(buf1),
>>
>> ...but what is this other buffer still doing? get_human_readable_size
>> needs to be converted to use QemuOutput.
>>
>>> +void bdrv_image_info_dump(const QemuOutput *output, ImageInfo *info)
>>> {
>>> char size_buf[128], dsize_buf[128];
>>
>> Why do we still need size_buf and dsize_buf?
>>
>>> if (!info->has_actual_size) {
>>> @@ -302,43 +302,47 @@ void bdrv_image_info_dump(ImageInfo *info)
>>> info->actual_size);
>>> }
>>> get_human_readable_size(size_buf, sizeof(size_buf),
>>> info->virtual_size);
>>
>> Again, get_human_readable_size should be converted to use QemuOutput.
>>
> They do not likely have a chance to be truncated, so have not change
> them. I will convert them also in next version.
>
Just found this buffer is used in format control:
message_printf(output,
"%-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);
if get_human_readable_size() is converted, a manual control is
needed. Same thing happens to clock_buf. It seems better to keep what
it is.
>>> +++ b/qemu-img.c
>>> @@ -1554,16 +1554,18 @@ static void dump_snapshots(BlockDriverState *bs)
>>> {
>>> QEMUSnapshotInfo *sn_tab, *sn;
>>> int nb_sns, i;
>>> - char buf[256];
>>> + QemuOutput output = {OUTPUT_STREAM, {stdout,} };
>>
>> This is relying on C99's rule that a union is initialized by its first
>> named member. But I think it might be more readable as:
>>
>> output = { .kind = OUTPUT_STREAM, .stream = stdout };
>>
>> not to mention that you will HAVE to use a designator to ever initialize
>> the monitor element of the union in any parallel code that favors the
>> monitor.
> This solve the initialization issue, will use it, thank u for the tip.
>
>>
>> Hmm, does C99 even allow anonymous unions, or is that a gcc extension?
>>
>> Overall, I like the direction this is headed. The conversion looks
>> reasonable, although it didn't quite go far enough for getting rid of
>> buffers.
>>
>
>
--
Best Regards
Wenchao Xia
^ permalink raw reply [flat|nested] 14+ messages in thread