[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 6/6] snapshot: human monitor interface
|
From: |
Wenchao Xia |
|
Subject: |
Re: [Qemu-devel] [PATCH 6/6] snapshot: human monitor interface |
|
Date: |
Sat, 05 Jan 2013 16:36:03 +0800 |
|
User-agent: |
Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/20121026 Thunderbird/16.0.2 |
> On Mon, Dec 17, 2012 at 02:25:09PM +0800, Wenchao Xia wrote:
@@ -983,17 +983,22 @@ ETEXI
{
.name = "snapshot_blkdev",
- .args_type = "reuse:-n,device:B,snapshot-file:s?,format:s?",
- .params = "[-n] device [new-image-file] [format]",
- .help = "initiates a live snapshot\n\t\t\t"
- "of device. If a new image file is specified,
the\n\t\t\t"
- "new image file will become the new root image.\n\t\t\t"
- "If format is specified, the snapshot file will\n\t\t\t"
- "be created in that format. Otherwise the\n\t\t\t"
- "snapshot will be internal! (currently
unsupported).\n\t\t\t"
- "The default format is qcow2. The -n flag requests
QEMU\n\t\t\t"
- "to reuse the image found in new-image-file, instead
of\n\t\t\t"
- "recreating it from scratch.",
+ .args_type =
"internal:-i,reuse:-n,device:B,snapshot-file:s?,format:s?",
+ .params = "[-i] [-n] device [new-image-file] [format]",
Please rename snapshot-file and new-image-file because it is now either
the external snapshot filename or the internal snapshot name - it's not
always a file!
OK.
+ .help = "initiates a live snapshot of device.\n\t\t\t"
+ " The -i flag requests QEMU to create internal
snapshot\n\t\t\t"
+ "instead of external one.\n\t\t\t"
+ " The -n flag requests QEMU to use existing
snapshot\n\t\t\t"
+ "instead of creating new snapshot, which would fails
if\n\t\t\t"
+ "snapshot does not exist ahead.\n\t\t\t"
"which fails if the snapshot does not exist already"
Will use this, thanks.
+ " new-image-file is the snapshot's name, in external
case\n\t\t\t"
+ "it is the new image's name which will become the new
root\n\t\t\t"
+ "image and must be specified, in internal case it is
the\n\t\t\t"
+ "record's name and if not specified QEMU will
create\n\t\t\t"
+ "internal snapshot with name generated according to
time.\n\t\t\t"
+ " format is only valid in external case, which is the
new\n\t\t\t"
+ "snapshot image's format. If not sepcified default
format\n\t\t\t"
+ "qcow2 will be used.",
"If not specified, the default format is qcow2."
.mhandler.cmd = hmp_snapshot_blkdev,
},
@@ -1004,6 +1009,25 @@ Snapshot device, using snapshot file as target if
provided
ETEXI
{
+ .name = "snapshot_delete_blkdev",
Internal snapshots can already be deleted with delvm but there is no
existing command for external snapshots.
+ .args_type = "internal:-i,device:B,snapshot-file:s",
+ .params = "[-i] device new-image-file",
+ .help = "delete a snapshot synchronous.\n\t\t\t"
"synchronous" is implied for HMP commands, they are all like this so I
don't think it's necessary to mention it.
+ " The -i flag requests QEMU to delete internal
snapshot\n\t\t\t"
+ "instead of external one.\n\t\t\t"
+ " new-image-file is the snapshot's name, in external
case\n\t\t\t"
+ "it is the image's name which is not supported
now.\n\t\t\t"
I'm not sure how useful this interface is. If we have no implementation
for deleting external snapshots and libvirt already uses delvm, then I'd
prefer you drop this command from the patch series.
Later on, when there is code to implement external snapshot deletion it
can be added. Then there is no risk that this command design doesn't
work and we have to change it again. (Remember libvirt already uses
delvm so adding snapshot_delete_blkdev without external snapshot
deletion just adds code churn.)
OK, this interface will be dropped to make things simpler.
@@ -1486,8 +1510,8 @@ ETEXI
{
.name = "info",
- .args_type = "item:s?",
- .params = "[subcommand]",
+ .args_type = "item:s?,params:s?",
+ .params = "[subcommand] [params]",
.help = "show various information about the system state",
.mhandler.cmd = do_info,
},
What does this change do?
Pls ignore this, another serial was sent which extend hmp info
infra first.
diff --git a/hmp.c b/hmp.c
index 180ba2b..f247f51 100644
--- a/hmp.c
+++ b/hmp.c
@@ -806,20 +806,40 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
const char *filename = qdict_get_try_str(qdict, "snapshot-file");
const char *format = qdict_get_try_str(qdict, "format");
int reuse = qdict_get_try_bool(qdict, "reuse", 0);
+ int internal = qdict_get_try_bool(qdict, "internal", 0);
enum NewImageMode mode;
+ enum SnapshotType type;
Error *errp = NULL;
- if (!filename) {
- /* In the future, if 'snapshot-file' is not specified, the snapshot
- will be taken internally. Today it's actually required. */
+ if ((!internal) && (!filename)) {
+ /* in external case filename must be set, should we generate
+ it automatically? */
Picking a filename would mainly be useful to humans. libvirt and other
management tools will want full control anyway. So the question is: is
there a naming policy that will be useful to most human users?
If yes, please implement it. If no, please drop this comment.
It will be dropped.
diff --git a/monitor.c b/monitor.c
index c0e32d6..81de470 100644
--- a/monitor.c
+++ b/monitor.c
@@ -124,11 +124,14 @@ typedef struct mon_cmd_t {
void (*user_print)(Monitor *mon, const QObject *data);
union {
void (*info)(Monitor *mon);
+ void (*info_qdict)(Monitor *mon, const QDict *qdict);
void (*cmd)(Monitor *mon, const QDict *qdict);
- int (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
+ int (*cmd_new)(Monitor *mon, const QDict *params,
+ QObject **ret_data);
int (*cmd_async)(Monitor *mon, const QDict *params,
MonitorCompletion *cb, void *opaque);
} mhandler;
+ int info_cmd_need_qdict;
int flags;
The union discriminator is the flags field (e.g. MONITOR_CMD_ASYNC).
Please follow that style.
(If you use a boolean variable, please use the bool type instead of
int.)
There would be another way to extend hmp info command.
} mon_cmd_t;
@@ -824,7 +827,11 @@ static void do_info(Monitor *mon, const QDict *qdict)
goto help;
}
- cmd->mhandler.info(mon);
+ if (cmd->info_cmd_need_qdict) {
+ cmd->mhandler.info_qdict(mon, qdict);
+ } else {
+ cmd->mhandler.info(mon);
+ }
return;
help:
The generic monitor changes need to go in a separate commit.
@@ -2605,10 +2612,12 @@ static mon_cmd_t info_cmds[] = {
},
{
.name = "snapshots",
- .args_type = "",
- .params = "",
- .help = "show the currently saved VM snapshots",
- .mhandler.info = do_info_snapshots,
+ .args_type = "device:B?",
+ .params = "[device]",
+ .help = "show the currently saved VM snapshots or snapshots on "
+ "a single device.",
+ .mhandler.info_qdict = do_info_snapshots,
+ .info_cmd_need_qdict = 1,
},
{
.name = "status",
This change to the info snapshots command needs to go in a separate
commit.
It will go to hmp serial.
diff --git a/savevm.c b/savevm.c
index c027529..5982aa9 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2336,7 +2336,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
}
}
-void do_info_snapshots(Monitor *mon)
+static void do_info_snapshots_vm(Monitor *mon)
{
BlockDriverState *bs, *bs1;
QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
@@ -2400,6 +2400,59 @@ void do_info_snapshots(Monitor *mon)
}
+static void do_info_snapshots_blk(Monitor *mon, const char *device)
+{
+ BlockDriverState *bs;
+ QEMUSnapshotInfo *sn_tab, *sn;
+ int nb_sns, i;
+ char buf[256];
+
+ /* find the target bs */
+ bs = bdrv_find(device);
+ if (!bs) {
+ monitor_printf(mon, "Device '%s' not found.\n", device);
+ return ;
+ }
+
+ if (!bdrv_can_snapshot(bs)) {
+ monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
+ return ;
+ }
+
+ nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+ if (nb_sns < 0) {
+ monitor_printf(mon, "Device %s bdrv_snapshot_list: error %d\n",
+ device, nb_sns);
+ return;
+ }
+
+ if (nb_sns == 0) {
+ monitor_printf(mon, "There is no snapshot available.\n");
+ return;
+ }
+
+ monitor_printf(mon, "Device %s:\n", device);
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+ for (i = 0; i < nb_sns; i++) {
+ sn = &sn_tab[i];
+ monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+ }
+ g_free(sn_tab);
+ return;
+}
Return at the end of a void function is not necessary and QEMU code
doesn't use it. Please use QEMU style.
OK.
Stefan
--
Best Regards
Wenchao Xia