Re: [Qemu-devel] [PATCH v2 07/12] qapi: Convert savevm

[Pavel Hrdina]

On 26.3.2013 20:56, Eric Blake wrote:
> On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <address@hidden>
> > ---
> >   hmp-commands.hx         |  4 ++--
> >   hmp.c                   |  9 +++++++++
> >   hmp.h                   |  1 +
> >   include/sysemu/sysemu.h |  1 -
> >   qapi-schema.json        | 18 ++++++++++++++++++
> >   qmp-commands.hx         | 29 +++++++++++++++++++++++++++++
> >   savevm.c                | 27 ++++++++++++---------------
> >   7 files changed, 71 insertions(+), 18 deletions(-)
>
> > +++ b/qapi-schema.json
> > @@ -3442,3 +3442,21 @@
> >   # Since: 1.5
> >   ##
> >   { 'command': 'query-tpm', 'returns': ['TPMInfo'] }
> > +
> > +##
> > +# @vm-snapshot-save:
> > +#
> > +# Create a snapshot of the whole virtual machine. If tag is provided as @name,
> > +# it is used as human readable identifier. If there is already a snapshot
> > +# with the same tag or ID, it is replaced.
>
> Any reason we have to fix this up later in the series, instead of
> getting the interface right to begin with (in regards to having an
> optional force argument)?

It was supposed to introduce new feature for both, but I can make it as 
you suggesting.

> > +#
> > +# The VM is automatically stopped and resumed and saving a snapshot can take
> > +# a long time.
>
> Transactionally, are we exposing the right building blocks?  While the
> existing HMP command pauses the domain up front, I think the QMP
> interface should be job-oriented.  That is, it should be possible to
> start a long-running job and have QMP return immediately, so that QMP
> remains responsive, and lets us do a live migrate, live bandwidth
> tuning, the ability to gracefully abort, and the ability to pause the
> domain if live migrate isn't converging fast enough.  HMP would then
> preserve its existing interface by calling multiple QMP commands,
> instead of trying to make QMP an exact analogue to the sub-par HMP
> interface.
>
> Libvirt _really_ wants to be able to cancel an in-progress snapshot
> creation action, but can't if all we expose is the same interface as HMP
> has always had.

I'm working on live snapshots which will introduce this functionality, 
but I couldn't give you any deadline when I'll have it done. So if you 
(libvirt) really want to have savevm and vm-snapshot-save non-blocking 
before I'll finish live snapshots, then I could rewrite this patch.

I know that current approach isn't good enough because of that blocking 
behavior.

> > +# @name: #optional tag of new snapshot or tag|id of existing snapshot
> > +#
> > +# Returns: Nothing on success
>
> Bad.  If @name is optional, and one is generated on your behalf, then
> you need to return the 'name' that was generated.  Also, even if 'name'
> is provided, knowing which 'id' was allocated is useful, since later
> APIs can operate on 'name' or 'id'.

That's a good point. I'll fix this to return all information about the 
created snapshot.

> > +#
> > +# Since: 1.5
> > +##
> > +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
>
> In other words, this needs a return value.
>
>
> >           if (!bdrv_can_snapshot(bs)) {
> > -            monitor_printf(mon, "Device '%s' is writable but does not support 
> > snapshots.\n",
> > -                               bdrv_get_device_name(bs));
> > +            error_setg(errp, "Device '%s' is writable but does not support "
> > +                       "snapshots.", bdrv_get_device_name(bs));
> >               return;
> >           }
> >       }
> >
> >       bs = bdrv_snapshots();
> >       if (!bs) {
> > -        monitor_printf(mon, "No block device can accept snapshots\n");
> > +        error_setg(errp, "No block device can accept snapshots.");
>
> Odd that we weren't consistent on whether errors ended with '.' when
> using monitor_printf; your patch at least tried to be self-consistent,
> even if opposite the normal usage of error_setg() :)

Thanks for review. I've fixed error messages and other issues you 
mentioned in other patches.

Pavel