From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=42199 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PxLnA-0000mi-Do for qemu-devel@nongnu.org; Wed, 09 Mar 2011 10:59:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PxLn9-0003AZ-6d for qemu-devel@nongnu.org; Wed, 09 Mar 2011 10:59:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:12491) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PxLn8-0003AU-O0 for qemu-devel@nongnu.org; Wed, 09 Mar 2011 10:59:15 -0500 Message-ID: <4D77A44C.5090904@redhat.com> Date: Wed, 09 Mar 2011 17:01:16 +0100 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] QMP: add snapshot_blkdev_sync command References: <1299685061-24234-1-git-send-email-Jes.Sorensen@redhat.com> <4D77A0EA.4060507@codemonkey.ws> In-Reply-To: <4D77A0EA.4060507@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Jes.Sorensen@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com Am 09.03.2011 16:46, schrieb Anthony Liguori: > On 03/09/2011 09:37 AM, Jes.Sorensen@redhat.com wrote: >> From: Jes Sorensen >> >> Add QMP bits for snapshot_blkdev_sync command. This is the same as >> snapshot_blkdev in the human monitor, but added _sync to the name to >> make it explicit that the command is synchronous and leave space for a >> future async version. >> >> Signed-off-by: Jes Sorensen >> --- >> qmp-commands.hx | 19 +++++++++++++++++++ >> 1 files changed, 19 insertions(+), 0 deletions(-) >> >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index 9d3cc31..e32187e 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -667,6 +667,25 @@ Example: >> EQMP >> >> { >> + .name = "snapshot_blkdev_sync", >> + .args_type = "device:B,snapshot_file:s?,format:s?", >> + .params = "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)", >> + .user_print = monitor_user_noop, >> + .mhandler.cmd_new = do_snapshot_blkdev, >> + }, >> + >> +SQMP >> +Synchronous snapshot of block device, using snapshot file as target >> +if provided. > > Please document the error semantics. > > The documentation in .help is discarded for QMP. You should put the > docs in the SQMP section. > > Also, QMP should use '-' instead of '_'. We should also try to follow > the form 'noun'-'verb' so the name would be better as 'blkdev-snapshot-sync' > > I'm not sure blkdev is the right prefix. Kevin, what are your thoughts > here? Does 'blkdev' make sense for any command operating on a block > device (that is, a qdev device that happens to have a block drive, not > the same thing as -blockdev that we've discussed in the past). Doesn't this command work on a -blockdev style thing, i.e. BlockDriverState or DriveInfo? I don't think we have any commands that refer to qdev devices that happen to be block devices. You could probably argue that some of them should... Kevin