On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  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)?

> +#
> +# 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.

> +# @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'.

> +#
> +# 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() :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org