From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34208) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TrPEn-0003f7-8T for qemu-devel@nongnu.org; Sat, 05 Jan 2013 03:36:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TrPEl-0004W2-96 for qemu-devel@nongnu.org; Sat, 05 Jan 2013 03:36:17 -0500 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:53351) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TrPEk-0004Vp-BS for qemu-devel@nongnu.org; Sat, 05 Jan 2013 03:36:15 -0500 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sat, 5 Jan 2013 18:28:58 +1000 Received: from d23relay04.au.ibm.com (d23relay04.au.ibm.com [9.190.234.120]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 02FA92BB004B for ; Sat, 5 Jan 2013 19:36:09 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r058OhoW59113612 for ; Sat, 5 Jan 2013 19:24:44 +1100 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r058a8lb026799 for ; Sat, 5 Jan 2013 19:36:08 +1100 Message-ID: <50E7E5F3.8030209@linux.vnet.ibm.com> Date: Sat, 05 Jan 2013 16:36:03 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1355725509-5429-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1355725509-5429-7-git-send-email-xiawenc@linux.vnet.ibm.com> <20130104154447.GC6310@stefanha-thinkpad.hitronhub.home> In-Reply-To: <20130104154447.GC6310@stefanha-thinkpad.hitronhub.home> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 6/6] snapshot: human monitor interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, pbonzini@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, blauwirbel@gmail.com > 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