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

[Pavel Hrdina <phrdina@redhat.com>]

On 26.3.2013 20:56, Eric Blake wrote:
> 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)?

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