From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:53098) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QF6J7-0002D0-60 for qemu-devel@nongnu.org; Wed, 27 Apr 2011 11:05:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QF6J1-00045B-Cx for qemu-devel@nongnu.org; Wed, 27 Apr 2011 11:05:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43972) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QF6J1-000456-38 for qemu-devel@nongnu.org; Wed, 27 Apr 2011 11:05:31 -0400 Date: Wed, 27 Apr 2011 12:05:20 -0300 From: Luiz Capitulino Message-ID: <20110427120520.74e348d9@doriath> In-Reply-To: <1303136821-13333-2-git-send-email-Jes.Sorensen@redhat.com> References: <1303136821-13333-1-git-send-email-Jes.Sorensen@redhat.com> <1303136821-13333-2-git-send-email-Jes.Sorensen@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Jes.Sorensen@redhat.com Cc: kwolf@redhat.com, qemu-devel@nongnu.org, Markus Armbruster 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. 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",