From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:57454) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QFSKE-0004Yy-CD for qemu-devel@nongnu.org; Thu, 28 Apr 2011 10:36:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QFSK8-0008OZ-LU for qemu-devel@nongnu.org; Thu, 28 Apr 2011 10:36:14 -0400 Received: from mail-yx0-f173.google.com ([209.85.213.173]:45815) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QFSK8-0008OV-HU for qemu-devel@nongnu.org; Thu, 28 Apr 2011 10:36:08 -0400 Received: by yxk8 with SMTP id 8so983401yxk.4 for ; Thu, 28 Apr 2011 07:36:07 -0700 (PDT) Message-ID: <4DB97B55.7030202@codemonkey.ws> Date: Thu, 28 Apr 2011 09:36:05 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1303136821-13333-1-git-send-email-Jes.Sorensen@redhat.com> <1303136821-13333-2-git-send-email-Jes.Sorensen@redhat.com> <20110427120520.74e348d9@doriath> In-Reply-To: <20110427120520.74e348d9@doriath> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/1] Add QMP bits for blockdev-snapshot-sync. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, Jes.Sorensen@redhat.com, qemu-devel@nongnu.org, Markus Armbruster On 04/27/2011 10:05 AM, Luiz Capitulino wrote: > On Mon, 18 Apr 2011 16:27:01 +0200 > Jes.Sorensen@redhat.com wrote: > >> From: Jes Sorensen >> >> This is quivalent to snapshot_blkdev in the human monitor, with _sync >> added to the command name to make it explicit that the command is >> synchronous and leave space for a future async version. > > I'm not sure appending "_sync" is such a good convention, most commands > are sync today and they don't have it. I'd prefer to call it snapshot_blkdev > and note in the documentation how it works. It probably should be called snapshot_blkdev_broken because that's what it really is. The '_sync' is there to indicate that this command doesn't properly implement asynchronous logic and can break a guest. I'd actually prefer that we not expose this command through QMP at all and instead implement a proper snapshot command. Regards, Anthony Liguori > > On the other hand, I'm not sure how Anthony is going to model async > commands, so maybe he has a better suggestion. > >> Signed-off-by: Jes Sorensen >> --- >> qmp-commands.hx | 26 ++++++++++++++++++++++++++ >> 1 files changed, 26 insertions(+), 0 deletions(-) >> >> diff --git a/qmp-commands.hx b/qmp-commands.hx >> index fbd98ee..b8f537c 100644 >> --- a/qmp-commands.hx >> +++ b/qmp-commands.hx >> @@ -667,6 +667,32 @@ Example: >> EQMP >> >> { >> + .name = "blockdev-snapshot-sync", >> + .args_type = "device:B,snapshot_file:s?,format:s?", >> + .params = "device [new-image-file] [format]", >> + .user_print = monitor_user_noop, >> + .mhandler.cmd_new = do_snapshot_blkdev, >> + }, >> + >> +SQMP > > This doesn't follow QMP doc convention, which is: > > command name > ------------ > > (Explain how the command works, like you do below) > > Arguments > > Example > >> +Synchronous snapshot of block device, using snapshot file as target >> +if provided. > > It's not optional in HMP: > > (qemu) snapshot_blkdev ide0-hd0 > Parameter 'snapshot_file' is missing > (qemu) > > And the command argument is called "snapshot_file" not "new-image-file" > as written in the HMP help text. > >> + >> +If a new image file is specified, the new image file will become the >> +new root image. If format is specified, the snapshot file will be >> +created in that format. Otherwise the snapshot will be internal! >> +(currently unsupported). > > Sorry for the stupid question, but what's a "new root image"? Also, all > these assumptions seem human features to me, as it can save some typing > and I can poke around to see where the snapshots are stored. > > All arguments should be mandatory in QMP, IMO. > > Finally, what's the expect behavior when -snapshot is used? I'm getting > this: > > (qemu) snapshot_blkdev ide0-hd0 snap-test > Could not open '/tmp/vl.6w8YXA' > (qemu) > > At first, I don't see why we shouldn't generate the live snapshot, but anyway, > any special behavior like this should be noted in the section called Notes > in the command's documentation. > >> + >> +Errors: >> +If creating the new snapshot image fails, QEMU will continue running >> +on the original image. If switching to the newly created image fails, >> +it will be attempted to fall back to the original image and return >> +QERR_OPEN_FILE_FAILED with the snapshot filename. If re-opening >> +the original image fails, QERR_OPEN_FILE_FAILED will be returned with >> +the original image filename. >> +EQMP >> + >> + { >> .name = "balloon", >> .args_type = "value:M", >> .params = "target", > >