Re: [Qemu-devel] [PATCH v2 1/1] Add QMP bits for blockdev-snapshot-sync.

[Anthony Liguori]

On 04/27/2011 10:05 AM, Luiz Capitulino wrote:
> On Mon, 18 Apr 2011 16:27:01 +0200
> address@hidden wrote:
>
> > From: Jes Sorensen<address@hidden>
> >
> > 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<address@hidden>
> > ---
> >   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",