From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=43858 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pxc1U-0006zm-Fb for qemu-devel@nongnu.org; Thu, 10 Mar 2011 04:19:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pxc1T-0005bl-0p for qemu-devel@nongnu.org; Thu, 10 Mar 2011 04:19:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45143) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pxc1S-0005bT-Mx for qemu-devel@nongnu.org; Thu, 10 Mar 2011 04:19:06 -0500 Message-ID: <4D789803.40206@redhat.com> Date: Thu, 10 Mar 2011 10:21:07 +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> <4D77A44C.5090904@redhat.com> <4D77B2FD.3050304@codemonkey.ws> In-Reply-To: <4D77B2FD.3050304@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, Markus Armbruster , lcapitulino@redhat.com Am 09.03.2011 18:03, schrieb Anthony Liguori: > On 03/09/2011 10:01 AM, Kevin Wolf wrote: >> 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... > > 'device' is a device name though, right? Or is it a name associated > with a BlockDriverState that currently happens to be a qdev name? > > Would I be able to eventually pass a qdev path here? > > If the answer is that this is a bdrv_name, then should we at least use > blockdev instead of blkdev? I think it's a drive name, like 'ide-hd0' or what they were called. I'm not completely sure if this is the same namespace as blockdev_add would use, but at first sight it would make some sense. Markus? Kevin