[Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Denis V. Lunev]

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 migration/savevm.c |  5 +++++
 qapi-schema.json   | 13 +++++++++++++
 qmp-commands.hx    | 25 +++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index f83ffd0..565b10a 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }
 }
 
+void qmp_savevm(bool has_name, const char *name, Error **errp)
+{
+    do_savevm(has_name ? name : NULL, errp);
+}
+
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
diff --git a/qapi-schema.json b/qapi-schema.json
index b65905f..8cc8b44 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3962,3 +3962,16 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @savevm
+#
+# Save a VM snapshot. Without a name new snapshot is created",
+#
+# @name: identifier of a snapshot to be created
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'savevm', 'data': {'*name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..cd895f6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4739,3 +4739,28 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+SQMP
+savevm
+------------------
+
+Save a VM snapshot. If no tag or id are provided, a new snapshot is created
+
+Arguments:
+
+- "name": (optional) snapshot name
+
+Example:
+
+-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "savevm",
+        .args_type  = "name:s?",
+        .mhandler.cmd_new = qmp_marshal_savevm,
+    },
-- 
2.5.0

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  migration/savevm.c |  5 +++++
>  qapi-schema.json   | 13 +++++++++++++
>  qmp-commands.hx    | 25 +++++++++++++++++++++++++
>  3 files changed, 43 insertions(+)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index f83ffd0..565b10a 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +void qmp_savevm(bool has_name, const char *name, Error **errp)
> +{
> +    do_savevm(has_name ? name : NULL, errp);
> +}
> +

Please name do_savevm() qmp_savevm() and drop this wrapper.

We're working on omitting has_FOO for pointer-valued FOO.

>  void qmp_xen_save_devices_state(const char *filename, Error **errp)
>  {
>      QEMUFile *f;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index b65905f..8cc8b44 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3962,3 +3962,16 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @savevm
> +#
> +# Save a VM snapshot. Without a name new snapshot is created",
> +#
> +# @name: identifier of a snapshot to be created

Missing #optional tag.

What happens when @name is missing?

What happens when @name names an existing snapshot?

Do we want @name to be optional in QMP?  I dimly remember ambiguity
problems between names and IDs.  Perhaps it'll become clear later in the
series.

The QMP interface needs to make sense on its own, even if that means
deviating from HMP.  Juan, Amit, do you have opinions on the proper QMP
interface for internal snapshots?

> +#
> +# Returns: Nothing on success
> +#
> +# Since 2.6
> +##
> +{ 'command': 'savevm', 'data': {'*name': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9d8b42f..cd895f6 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4739,3 +4739,28 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +SQMP
> +savevm
> +------------------
> +
> +Save a VM snapshot. If no tag or id are provided, a new snapshot is created
> +
> +Arguments:
> +
> +- "name": (optional) snapshot name
> +
> +Example:
> +
> +-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
> +        .name       = "savevm",
> +        .args_type  = "name:s?",
> +        .mhandler.cmd_new = qmp_marshal_savevm,
> +    },

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Juan Quintela]

Markus Armbruster <armbru@redhat.com> wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> ---
>>  migration/savevm.c |  5 +++++
>>  qapi-schema.json   | 13 +++++++++++++
>>  qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>  3 files changed, 43 insertions(+)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index f83ffd0..565b10a 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>      }
>>  }
>>  
>> +void qmp_savevm(bool has_name, const char *name, Error **errp)
>> +{
>> +    do_savevm(has_name ? name : NULL, errp);
>> +}
>> +
>
> Please name do_savevm() qmp_savevm() and drop this wrapper.
>
> We're working on omitting has_FOO for pointer-valued FOO.

Agreed.

>
>>  void qmp_xen_save_devices_state(const char *filename, Error **errp)
>>  {
>>      QEMUFile *f;
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index b65905f..8cc8b44 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3962,3 +3962,16 @@
>>  ##
>>  { 'enum': 'ReplayMode',
>>    'data': [ 'none', 'record', 'play' ] }
>> +
>> +##
>> +# @savevm
>> +#
>> +# Save a VM snapshot. Without a name new snapshot is created",
>> +#
>> +# @name: identifier of a snapshot to be created
>
> Missing #optional tag.
>
> What happens when @name is missing?

Why are we allowing this?  QMP is going to be called by libvirt or
similar, they can choose a name, thanks very much.

> What happens when @name names an existing snapshot?

We remove them.

    /* Delete old snapshots of the same name */
    if (name && del_existing_snapshots(mon, name) < 0) {
        goto the_end;
    }

I think we should give one error.  Let the hmp code remove them.
I would preffer to change the interface to "require" a flag if we want
it to be removed.  But requiring a flag is the equivalent of  requiring

qmp_deletevm
qmp_savevm

or whatever they are called.


> Do we want @name to be optional in QMP?  I dimly remember ambiguity
> problems between names and IDs.  Perhaps it'll become clear later in the
> series.

Again, I think that requiring a name is the best thing to do.  Libvirt,
openstack and friends are good at choosen names, qemu is not.

> The QMP interface needs to make sense on its own, even if that means
> deviating from HMP.  Juan, Amit, do you have opinions on the proper QMP
> interface for internal snapshots?

I agree that the "requirement" of a name is a good idea.  Not deleting
the existing snapshots looks like a good idea also.  It is not difficult
to romeve them previously.

Thanks, Juan.

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Denis V. Lunev]

On 11/18/2015 02:36 PM, Juan Quintela wrote:
> Markus Armbruster <armbru@redhat.com> wrote:
>> "Denis V. Lunev" <den@openvz.org> writes:
>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Juan Quintela <quintela@redhat.com>
>>> CC: Amit Shah <amit.shah@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> ---
>>>   migration/savevm.c |  5 +++++
>>>   qapi-schema.json   | 13 +++++++++++++
>>>   qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>>   3 files changed, 43 insertions(+)
>>>
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index f83ffd0..565b10a 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>>       }
>>>   }
>>>   
>>> +void qmp_savevm(bool has_name, const char *name, Error **errp)
>>> +{
>>> +    do_savevm(has_name ? name : NULL, errp);
>>> +}
>>> +
>> Please name do_savevm() qmp_savevm() and drop this wrapper.
>>
>> We're working on omitting has_FOO for pointer-valued FOO.
> Agreed.
>

is there a tree with this stuff in or something I could be based on?
This series is simple enough to be written fast and be merged
early when the hard freeze will be done.

Above stuff could have a way harder way due to the amount of
changes to be performed.

Den

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Denis V. Lunev]

On 11/17/2015 01:10 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> ---
>>   migration/savevm.c |  5 +++++
>>   qapi-schema.json   | 13 +++++++++++++
>>   qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>   3 files changed, 43 insertions(+)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index f83ffd0..565b10a 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>       }
>>   }
>>   
>> +void qmp_savevm(bool has_name, const char *name, Error **errp)
>> +{
>> +    do_savevm(has_name ? name : NULL, errp);
>> +}
>> +
> Please name do_savevm() qmp_savevm() and drop this wrapper.
>
> We're working on omitting has_FOO for pointer-valued FOO.
error code is used in the qemu-img.
I can drop error code check there and replace it with checking of Error 
pointer.
Is this OK for you?

Den

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

On 12/01/2015 07:28 AM, Denis V. Lunev wrote:
>>>>   +void qmp_savevm(bool has_name, const char *name, Error **errp)
>>>> +{
>>>> +    do_savevm(has_name ? name : NULL, errp);
>>>> +}
>>>> +
>>> Please name do_savevm() qmp_savevm() and drop this wrapper.
>>>
>>> We're working on omitting has_FOO for pointer-valued FOO.
>> Agreed.
>>
> 
> is there a tree with this stuff in or something I could be based on?
> This series is simple enough to be written fast and be merged
> early when the hard freeze will be done.

I have started the work in my local tree, but have not posted it yet.
I'll try and remember to cc you when it's ready (right now, there are
more than 20 qapi patches from me and another 35 or so from Marc-Andre
that need reviewing before we'd even get to the point of my work on
dropping has_FOO for pointers).

> 
> Above stuff could have a way harder way due to the amount of
> changes to be performed.

Dropping has_FOO will be rather invasive; my plan is to add a per-struct
marker to the .json files on which structs are ready to use the leaner
layout, then do a series of patches that adjusts a couple structs at a
time and catches all fallout for those structs.  Then, if everything
converts nicely to the leaner form, I can remove the per-struct marker
in the end.  So it's not a problem to just keep the three-argument form
with has_name for now.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> On 11/18/2015 02:36 PM, Juan Quintela wrote:
>> Markus Armbruster <armbru@redhat.com> wrote:
>>> "Denis V. Lunev" <den@openvz.org> writes:
>>>
>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>> CC: Juan Quintela <quintela@redhat.com>
>>>> CC: Amit Shah <amit.shah@redhat.com>
>>>> CC: Markus Armbruster <armbru@redhat.com>
>>>> CC: Eric Blake <eblake@redhat.com>
>>>> ---
>>>>   migration/savevm.c |  5 +++++
>>>>   qapi-schema.json   | 13 +++++++++++++
>>>>   qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>>>   3 files changed, 43 insertions(+)
>>>>
>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>> index f83ffd0..565b10a 100644
>>>> --- a/migration/savevm.c
>>>> +++ b/migration/savevm.c
>>>> @@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>>>       }
>>>>   }
>>>>   +void qmp_savevm(bool has_name, const char *name, Error **errp)
>>>> +{
>>>> +    do_savevm(has_name ? name : NULL, errp);
>>>> +}
>>>> +
>>> Please name do_savevm() qmp_savevm() and drop this wrapper.
>>>
>>> We're working on omitting has_FOO for pointer-valued FOO.
>> Agreed.
>>
>
> is there a tree with this stuff in or something I could be based on?
> This series is simple enough to be written fast and be merged
> early when the hard freeze will be done.
>
> Above stuff could have a way harder way due to the amount of
> changes to be performed.

I recommend not to base on our qapi work just for the has_FOO change.  I
mentioned it only because once it lands, the wrapper I want you to drop
would become entirely pointless.  Go ahead and get your patches in, and
let us deal with cleaning up all has_FOO.

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Markus Armbruster]

"Denis V. Lunev" <den@openvz.org> writes:

> On 11/17/2015 01:10 PM, Markus Armbruster wrote:
>> "Denis V. Lunev" <den@openvz.org> writes:
>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Juan Quintela <quintela@redhat.com>
>>> CC: Amit Shah <amit.shah@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> ---
>>>   migration/savevm.c |  5 +++++
>>>   qapi-schema.json   | 13 +++++++++++++
>>>   qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>>   3 files changed, 43 insertions(+)
>>>
>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>> index f83ffd0..565b10a 100644
>>> --- a/migration/savevm.c
>>> +++ b/migration/savevm.c
>>> @@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>>       }
>>>   }
>>>   +void qmp_savevm(bool has_name, const char *name, Error **errp)
>>> +{
>>> +    do_savevm(has_name ? name : NULL, errp);
>>> +}
>>> +
>> Please name do_savevm() qmp_savevm() and drop this wrapper.
>>
>> We're working on omitting has_FOO for pointer-valued FOO.
> error code is used in the qemu-img.
> I can drop error code check there and replace it with checking of
> Error pointer.
> Is this OK for you?

Let me rephrase my request:

1. Instead of wrapping do_savevm() in qmp_savevm(), use it directly.

2. Rename do_savevm() to qmp_savevm(), and change its arguments to make
it work as QMP command handler.

Does that make sense to you?

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Denis V. Lunev]

On 12/01/2015 06:05 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> On 11/17/2015 01:10 PM, Markus Armbruster wrote:
>>> "Denis V. Lunev" <den@openvz.org> writes:
>>>
>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>> CC: Juan Quintela <quintela@redhat.com>
>>>> CC: Amit Shah <amit.shah@redhat.com>
>>>> CC: Markus Armbruster <armbru@redhat.com>
>>>> CC: Eric Blake <eblake@redhat.com>
>>>> ---
>>>>    migration/savevm.c |  5 +++++
>>>>    qapi-schema.json   | 13 +++++++++++++
>>>>    qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>>>    3 files changed, 43 insertions(+)
>>>>
>>>> diff --git a/migration/savevm.c b/migration/savevm.c
>>>> index f83ffd0..565b10a 100644
>>>> --- a/migration/savevm.c
>>>> +++ b/migration/savevm.c
>>>> @@ -2010,6 +2010,11 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>>>        }
>>>>    }
>>>>    +void qmp_savevm(bool has_name, const char *name, Error **errp)
>>>> +{
>>>> +    do_savevm(has_name ? name : NULL, errp);
>>>> +}
>>>> +
>>> Please name do_savevm() qmp_savevm() and drop this wrapper.
>>>
>>> We're working on omitting has_FOO for pointer-valued FOO.
>> error code is used in the qemu-img.
>> I can drop error code check there and replace it with checking of
>> Error pointer.
>> Is this OK for you?
> Let me rephrase my request:
>
> 1. Instead of wrapping do_savevm() in qmp_savevm(), use it directly.
>
> 2. Rename do_savevm() to qmp_savevm(), and change its arguments to make
> it work as QMP command handler.
>
> Does that make sense to you?
sure!

I have mismatched with load_vmstate which has return code
and the error code was really checked in qemu_img.c

I'll address that there, sorry for confusion.

Den

[Qemu-devel] [PATCH v2 for 2.6 0/5] QMP wrappers for VM snapshot operations

[Denis V. Lunev]

EFI based VM with pflash storage for NVRAM could not be snapshoted as
libvirt configures storage as 'raw' and writable. OK, this is a libvirt
problem.

Another problem is that libvirt can not detect this failure at all
as it uses HMP for this operation. This create snapshot/delete snapshot
sequence passes silently.

The patchset adds QMP wrappers for the purpose.

Signed-off-by: "Denis V. Lunev" <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>

Changes from v1:
- cosmetic fixes suggested by Markus. I pray I have added all of them :)
- patch 5 is rewritten completely. Original one was deadbeaf

Denis V. Lunev (5):
  migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
  qmp: create qmp_savevm command
  qmp: create qmp_delvm command
  migration: improve error reporting for hmp_loadvm
  qmp: create QMP implementation of loadvm command

 include/sysemu/sysemu.h |   2 +-
 migration/savevm.c      | 100 +++++++++++++++++++++++++++++++-----------------
 monitor.c               |   7 +++-
 qapi-schema.json        |  39 +++++++++++++++++++
 qmp-commands.hx         |  71 ++++++++++++++++++++++++++++++++++
 vl.c                    |   5 ++-
 6 files changed, 185 insertions(+), 39 deletions(-)

[Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Denis V. Lunev]

This would be useful in the next step when QMP version of this call will
be introduced.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 migration/savevm.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 0ad1b93..b7278ac 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }
 
-void hmp_savevm(Monitor *mon, const QDict *qdict)
+static void do_savevm(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     uint64_t vm_state_size;
     qemu_timeval tv;
     struct tm tm;
-    const char *name = qdict_get_try_str(qdict, "name");
     Error *local_err = NULL;
     AioContext *aio_context;
 
     if (!bdrv_all_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;
     }
 
     /* Delete old snapshots of the same name */
     if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
-        monitor_printf(mon,
-                       "Error while deleting snapshot on device '%s': %s\n",
-                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
+        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
+                   bdrv_get_device_name(bs1), error_get_pretty(local_err));
         error_free(local_err);
         return;
     }
 
     bs = bdrv_all_find_vmstate_bs();
     if (bs == NULL) {
-        monitor_printf(mon, "No block device can accept snapshots\n");
+        error_setg(errp, "No block device can accept snapshots");
         return;
     }
     aio_context = bdrv_get_aio_context(bs);
@@ -1945,7 +1944,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
 
     ret = global_state_store();
     if (ret) {
-        monitor_printf(mon, "Error saving global state\n");
+        error_setg(errp, "Error saving global state");
         return;
     }
     vm_stop(RUN_STATE_SAVE_VM);
@@ -1977,22 +1976,20 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     /* save the VM state */
     f = qemu_fopen_bdrv(bs, 1);
     if (!f) {
-        monitor_printf(mon, "Could not open VM state file\n");
+        error_setg(errp, "Could not open VM state file");
         goto the_end;
     }
-    ret = qemu_savevm_state(f, &local_err);
+    ret = qemu_savevm_state(f, errp);
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
     if (ret < 0) {
-        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
-        error_free(local_err);
         goto the_end;
     }
 
     ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
     if (ret < 0) {
-        monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-                       bdrv_get_device_name(bs));
+        error_setg(errp, "Error while creating snapshot on '%s'",
+                   bdrv_get_device_name(bs));
     }
 
  the_end:
@@ -2002,6 +1999,17 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     }
 }
 
+void hmp_savevm(Monitor *mon, const QDict *qdict)
+{
+    Error *local_err = NULL;
+
+    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
+
+    if (local_err != NULL) {
+        error_report_err(local_err);
+    }
+}
+
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
-- 
2.5.0

[Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Denis V. Lunev]

'name' attribute is made mandatory in distinction with HMP command.

The patch also moves hmp_savevm implementation into hmp.c. This function
is just a simple wrapper now and does not have knowledge about
migration internals.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 hmp.c              | 12 ++++++++++++
 migration/savevm.c | 13 +------------
 qapi-schema.json   | 13 +++++++++++++
 qmp-commands.hx    | 25 +++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/hmp.c b/hmp.c
index 2140605..c9c7100 100644
--- a/hmp.c
+++ b/hmp.c
@@ -32,6 +32,7 @@
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
+#include "sysemu/sysemu.h"
 
 #ifdef CONFIG_SPICE
 #include <spice/enums.h>
@@ -2378,3 +2379,14 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
 
     qapi_free_RockerOfDpaGroupList(list);
 }
+
+void hmp_savevm(Monitor *mon, const QDict *qdict)
+{
+    Error *local_err = NULL;
+
+    qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err);
+
+    if (local_err != NULL) {
+        error_report_err(local_err);
+    }
+}
diff --git a/migration/savevm.c b/migration/savevm.c
index b7278ac..65e0081 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }
 
-static void do_savevm(const char *name, Error **errp)
+void qmp_savevm(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp)
     }
 }
 
-void hmp_savevm(Monitor *mon, const QDict *qdict)
-{
-    Error *local_err = NULL;
-
-    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
-
-    if (local_err != NULL) {
-        error_report_err(local_err);
-    }
-}
-
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
diff --git a/qapi-schema.json b/qapi-schema.json
index 8b1a423..0114132 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3971,3 +3971,16 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @savevm
+#
+# Save a VM snapshot. Without a name new snapshot is created",
+#
+# @name: identifier of a snapshot to be created
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'savevm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 9d8b42f..f216c5e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4739,3 +4739,28 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+SQMP
+savevm
+------------------
+
+Save a VM snapshot. If no tag or id are provided, a new snapshot is created
+
+Arguments:
+
+- "name": snapshot name
+
+Example:
+
+-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "savevm",
+        .args_type  = "name:s?",
+        .mhandler.cmd_new = qmp_marshal_savevm,
+    },
-- 
2.5.0

[Qemu-devel] [PATCH 3/5] qmp: create qmp_delvm command

[Denis V. Lunev]

The patch also moves hmp_delvm implementation into hmp.c. This function
is just a simple wrapper now and does not have knowledge about
migration internals.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 hmp.c              | 11 +++++++++++
 migration/savevm.c | 16 +++++++---------
 qapi-schema.json   | 13 +++++++++++++
 qmp-commands.hx    | 23 +++++++++++++++++++++++
 4 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/hmp.c b/hmp.c
index c9c7100..a342c8c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2390,3 +2390,14 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         error_report_err(local_err);
     }
 }
+
+void hmp_delvm(Monitor *mon, const QDict *qdict)
+{
+    Error *local_err = NULL;
+
+    qmp_delvm(qdict_get_str(qdict, "name"), &local_err);
+
+    if (local_err != NULL) {
+        error_report_err(local_err);
+    }
+}
diff --git a/migration/savevm.c b/migration/savevm.c
index 65e0081..f87a061 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2099,17 +2099,15 @@ int load_vmstate(const char *name)
     return 0;
 }
 
-void hmp_delvm(Monitor *mon, const QDict *qdict)
+void qmp_delvm(const char *name, Error **errp)
 {
     BlockDriverState *bs;
-    Error *err;
-    const char *name = qdict_get_str(qdict, "name");
-
-    if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
-        monitor_printf(mon,
-                       "Error while deleting snapshot on device '%s': %s\n",
-                       bdrv_get_device_name(bs), error_get_pretty(err));
-        error_free(err);
+    Error *local_err = NULL;
+
+    if (bdrv_all_delete_snapshot(name, &bs, &local_err) < 0) {
+        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
+                   bdrv_get_device_name(bs), error_get_pretty(local_err));
+        error_free(local_err);
     }
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 0114132..18c9a6c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3984,3 +3984,16 @@
 # Since 2.6
 ##
 { 'command': 'savevm', 'data': {'name': 'str'} }
+
+##
+# @delvm
+#
+# Delete a VM snapshot
+#
+# @name: identifier of a snapshot to be deleted
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'delvm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index f216c5e..a630e8a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4764,3 +4764,26 @@ EQMP
         .args_type  = "name:s?",
         .mhandler.cmd_new = qmp_marshal_savevm,
     },
+
+SQMP
+delvm
+------------------
+
+Delete a VM snapshot
+
+Arguments:
+
+- "name": snapshot name
+
+Example:
+
+-> { "execute": "delvm", "arguments": { "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "delvm",
+        .args_type  = "name:s",
+        .mhandler.cmd_new = qmp_marshal_delvm,
+    },
-- 
2.5.0

[Qemu-devel] [PATCH 4/5] migration: improve error reporting for load_vmstate

[Denis V. Lunev]

The patch adds Error ** parameter to load_vmstate call and fills error
inside. The caller after that properly reports error either through
monitor or via local stderr facility during VM start.

This helper will be useful too for qmp_loadvm implementation.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 include/sysemu/sysemu.h |  2 +-
 migration/savevm.c      | 28 ++++++++++++++++------------
 monitor.c               |  6 +++++-
 vl.c                    |  4 +++-
 4 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 3bb8897..d9ccf45 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -78,7 +78,7 @@ void qemu_remove_exit_notifier(Notifier *notify);
 void qemu_add_machine_init_done_notifier(Notifier *notify);
 
 void hmp_savevm(Monitor *mon, const QDict *qdict);
-int load_vmstate(const char *name);
+int load_vmstate(const char *name, Error **errp);
 void hmp_delvm(Monitor *mon, const QDict *qdict);
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict);
 
diff --git a/migration/savevm.c b/migration/savevm.c
index f87a061..7846437 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2026,7 +2026,7 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
     }
 }
 
-int load_vmstate(const char *name)
+int load_vmstate(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs_vm_state;
     QEMUSnapshotInfo sn;
@@ -2035,20 +2035,22 @@ int load_vmstate(const char *name)
     AioContext *aio_context;
 
     if (!bdrv_all_can_snapshot(&bs)) {
-        error_report("Device '%s' is writable but does not support snapshots.",
-                     bdrv_get_device_name(bs));
+        error_setg(errp,
+                   "Device '%s' is writable but does not support snapshots.",
+                   bdrv_get_device_name(bs));
         return -ENOTSUP;
     }
     ret = bdrv_all_find_snapshot(name, &bs);
     if (ret < 0) {
-        error_report("Device '%s' does not have the requested snapshot '%s'",
-                     bdrv_get_device_name(bs), name);
+        error_setg(errp,
+                   "Device '%s' does not have the requested snapshot '%s'",
+                   bdrv_get_device_name(bs), name);
         return ret;
     }
 
     bs_vm_state = bdrv_all_find_vmstate_bs();
     if (!bs_vm_state) {
-        error_report("No block device supports snapshots");
+        error_setg(errp, "No block device supports snapshots");
         return -ENOTSUP;
     }
     aio_context = bdrv_get_aio_context(bs_vm_state);
@@ -2058,10 +2060,11 @@ int load_vmstate(const char *name)
     ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
     aio_context_release(aio_context);
     if (ret < 0) {
+        error_setg_errno(errp, ret, "Snapshot '%s' not found", name);
         return ret;
     } else if (sn.vm_state_size == 0) {
-        error_report("This is a disk-only snapshot. Revert to it offline "
-            "using qemu-img.");
+        error_setg(errp, "This is a disk-only snapshot. Revert to it offline "
+                   "using qemu-img.");
         return -EINVAL;
     }
 
@@ -2070,15 +2073,16 @@ int load_vmstate(const char *name)
 
     ret = bdrv_all_goto_snapshot(name, &bs);
     if (ret < 0) {
-        error_report("Error %d while activating snapshot '%s' on '%s'",
-                     ret, name, bdrv_get_device_name(bs));
+        error_setg_errno(errp, ret,
+                         "Error while activating snapshot '%s' on '%s'",
+                         name, bdrv_get_device_name(bs));
         return ret;
     }
 
     /* restore the VM state */
     f = qemu_fopen_bdrv(bs_vm_state, 0);
     if (!f) {
-        error_report("Could not open VM state file");
+        error_setg(errp, "Could not open VM state file");
         return -EINVAL;
     }
 
@@ -2092,7 +2096,7 @@ int load_vmstate(const char *name)
 
     migration_incoming_state_destroy();
     if (ret < 0) {
-        error_report("Error %d while loading VM state", ret);
+        error_setg_errno(errp, ret, "Error while loading VM state");
         return ret;
     }
 
diff --git a/monitor.c b/monitor.c
index 9a35d72..3857724 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1739,10 +1739,14 @@ static void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
     int saved_vm_running  = runstate_is_running();
     const char *name = qdict_get_str(qdict, "name");
+    Error *local_err = NULL;
 
     vm_stop(RUN_STATE_RESTORE_VM);
 
-    if (load_vmstate(name) == 0 && saved_vm_running) {
+    if (load_vmstate(name, &local_err) < 0) {
+        error_report_err(local_err);
+    }
+    if (saved_vm_running) {
         vm_start();
     }
 }
diff --git a/vl.c b/vl.c
index 4211ff1..ee34edb 100644
--- a/vl.c
+++ b/vl.c
@@ -4648,8 +4648,10 @@ int main(int argc, char **argv, char **envp)
     qemu_system_reset(VMRESET_SILENT);
     register_global_state();
     if (loadvm) {
-        if (load_vmstate(loadvm) < 0) {
+        Error *local_err = NULL;
+        if (load_vmstate(loadvm, &local_err) < 0) {
             autostart = 0;
+            error_report_err(local_err);
         }
     }
 
-- 
2.5.0

[Qemu-devel] [PATCH 5/5] qmp: create QMP implementation of loadvm command

[Denis V. Lunev]

Unfortunately load_vmstate has a return code (int) and this code is checked
in the other places. Thus we could not just rename it to qmp_loadvm as
returns void.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 migration/savevm.c | 12 ++++++++++++
 monitor.c          | 12 ++++++------
 qapi-schema.json   | 13 +++++++++++++
 qmp-commands.hx    | 23 +++++++++++++++++++++++
 4 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 7846437..07b0bf4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2103,6 +2103,18 @@ int load_vmstate(const char *name, Error **errp)
     return 0;
 }
 
+void qmp_loadvm(const char *name, Error **errp)
+{
+    int saved_vm_running  = runstate_is_running();
+    vm_stop(RUN_STATE_RESTORE_VM);
+
+    load_vmstate(name, errp);
+
+    if (saved_vm_running) {
+        vm_start();
+    }
+}
+
 void qmp_delvm(const char *name, Error **errp)
 {
     BlockDriverState *bs;
diff --git a/monitor.c b/monitor.c
index 3857724..aba2c62 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1737,18 +1737,18 @@ void qmp_closefd(const char *fdname, Error **errp)
 
 static void hmp_loadvm(Monitor *mon, const QDict *qdict)
 {
-    int saved_vm_running  = runstate_is_running();
     const char *name = qdict_get_str(qdict, "name");
     Error *local_err = NULL;
 
-    vm_stop(RUN_STATE_RESTORE_VM);
+    if (name == NULL) {
+        monitor_printf(mon, "Snapshot name is not set for hmp_loadvm");
+        return;
+    }
 
-    if (load_vmstate(name, &local_err) < 0) {
+    qmp_loadvm(name, &local_err);
+    if (local_err != NULL) {
         error_report_err(local_err);
     }
-    if (saved_vm_running) {
-        vm_start();
-    }
 }
 
 int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
diff --git a/qapi-schema.json b/qapi-schema.json
index 18c9a6c..9747df8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3997,3 +3997,16 @@
 # Since 2.6
 ##
 { 'command': 'delvm', 'data': {'name': 'str'} }
+
+##
+# @loadvm
+#
+# Load a VM snapshot
+#
+# @name: identifier of a snapshot to be loaded
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'loadvm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a630e8a..4dedb62 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4787,3 +4787,26 @@ EQMP
         .args_type  = "name:s",
         .mhandler.cmd_new = qmp_marshal_delvm,
     },
+
+SQMP
+loadvm
+------------------
+
+Load a VM snapshot
+
+Arguments:
+
+- "name": snapshot name
+
+Example:
+
+-> { "execute": "loadvm", "arguments": { "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "loadvm",
+        .args_type  = "name:s",
+        .mhandler.cmd_new = qmp_marshal_loadvm,
+    },
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 for 2.6 0/5] QMP wrappers for VM snapshot operations

[Denis V. Lunev]

On 12/04/2015 05:44 PM, Denis V. Lunev wrote:
> EFI based VM with pflash storage for NVRAM could not be snapshoted as
> libvirt configures storage as 'raw' and writable. OK, this is a libvirt
> problem.
>
> Another problem is that libvirt can not detect this failure at all
> as it uses HMP for this operation. This create snapshot/delete snapshot
> sequence passes silently.
>
> The patchset adds QMP wrappers for the purpose.
>
> Signed-off-by: "Denis V. Lunev" <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
>
> Changes from v1:
> - cosmetic fixes suggested by Markus. I pray I have added all of them :)
> - patch 5 is rewritten completely. Original one was deadbeaf
>
> Denis V. Lunev (5):
>    migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
>    qmp: create qmp_savevm command
>    qmp: create qmp_delvm command
>    migration: improve error reporting for hmp_loadvm
>    qmp: create QMP implementation of loadvm command
>
>   include/sysemu/sysemu.h |   2 +-
>   migration/savevm.c      | 100 +++++++++++++++++++++++++++++++-----------------
>   monitor.c               |   7 +++-
>   qapi-schema.json        |  39 +++++++++++++++++++
>   qmp-commands.hx         |  71 ++++++++++++++++++++++++++++++++++
>   vl.c                    |   5 ++-
>   6 files changed, 185 insertions(+), 39 deletions(-)
>
ping

Re: [Qemu-devel] [PATCH v2 for 2.6 0/5] QMP wrappers for VM snapshot operations

[Denis V. Lunev]

On 12/11/2015 12:33 PM, Denis V. Lunev wrote:
> On 12/04/2015 05:44 PM, Denis V. Lunev wrote:
>> EFI based VM with pflash storage for NVRAM could not be snapshoted as
>> libvirt configures storage as 'raw' and writable. OK, this is a libvirt
>> problem.
>>
>> Another problem is that libvirt can not detect this failure at all
>> as it uses HMP for this operation. This create snapshot/delete snapshot
>> sequence passes silently.
>>
>> The patchset adds QMP wrappers for the purpose.
>>
>> Signed-off-by: "Denis V. Lunev" <den@openvz.org>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>>
>> Changes from v1:
>> - cosmetic fixes suggested by Markus. I pray I have added all of them :)
>> - patch 5 is rewritten completely. Original one was deadbeaf
>>
>> Denis V. Lunev (5):
>>    migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
>>    qmp: create qmp_savevm command
>>    qmp: create qmp_delvm command
>>    migration: improve error reporting for hmp_loadvm
>>    qmp: create QMP implementation of loadvm command
>>
>>   include/sysemu/sysemu.h |   2 +-
>>   migration/savevm.c      | 100 
>> +++++++++++++++++++++++++++++++-----------------
>>   monitor.c               |   7 +++-
>>   qapi-schema.json        |  39 +++++++++++++++++++
>>   qmp-commands.hx         |  71 ++++++++++++++++++++++++++++++++++
>>   vl.c                    |   5 ++-
>>   6 files changed, 185 insertions(+), 39 deletions(-)
>>
> ping
ping v2

Re: [Qemu-devel] [PATCH v2 for 2.6 0/5] QMP wrappers for VM snapshot operations

[Denis V. Lunev]

On 12/18/2015 09:10 AM, Denis V. Lunev wrote:
> On 12/11/2015 12:33 PM, Denis V. Lunev wrote:
>> On 12/04/2015 05:44 PM, Denis V. Lunev wrote:
>>> EFI based VM with pflash storage for NVRAM could not be snapshoted as
>>> libvirt configures storage as 'raw' and writable. OK, this is a libvirt
>>> problem.
>>>
>>> Another problem is that libvirt can not detect this failure at all
>>> as it uses HMP for this operation. This create snapshot/delete snapshot
>>> sequence passes silently.
>>>
>>> The patchset adds QMP wrappers for the purpose.
>>>
>>> Signed-off-by: "Denis V. Lunev" <den@openvz.org>
>>> CC: Juan Quintela <quintela@redhat.com>
>>> CC: Amit Shah <amit.shah@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>>
>>> Changes from v1:
>>> - cosmetic fixes suggested by Markus. I pray I have added all of 
>>> them :)
>>> - patch 5 is rewritten completely. Original one was deadbeaf
>>>
>>> Denis V. Lunev (5):
>>>    migration: split hmp_savevm to do_savevm and hmp_savevm wrapper
>>>    qmp: create qmp_savevm command
>>>    qmp: create qmp_delvm command
>>>    migration: improve error reporting for hmp_loadvm
>>>    qmp: create QMP implementation of loadvm command
>>>
>>>   include/sysemu/sysemu.h |   2 +-
>>>   migration/savevm.c      | 100 
>>> +++++++++++++++++++++++++++++++-----------------
>>>   monitor.c               |   7 +++-
>>>   qapi-schema.json        |  39 +++++++++++++++++++
>>>   qmp-commands.hx         |  71 ++++++++++++++++++++++++++++++++++
>>>   vl.c                    |   5 ++-
>>>   6 files changed, 185 insertions(+), 39 deletions(-)
>>>
>> ping
> ping v2
guys?

Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> This would be useful in the next step when QMP version of this call will
> be introduced.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  migration/savevm.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 

> @@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>      uint64_t vm_state_size;
>      qemu_timeval tv;
>      struct tm tm;
> -    const char *name = qdict_get_try_str(qdict, "name");
>      Error *local_err = NULL;
>      AioContext *aio_context;
>  
>      if (!bdrv_all_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.",

No trailing '.' in error_setg() calls.

> +                   bdrv_get_device_name(bs));
>          return;
>      }
>  
>      /* Delete old snapshots of the same name */
>      if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
> -        monitor_printf(mon,
> -                       "Error while deleting snapshot on device '%s': %s\n",
> -                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
> +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));

Markus' series to add a prefixing notation would be better to use here
(although I didn't check if he caught this one in that series already):
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html

>  
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;
> +
> +    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
> +
> +    if (local_err != NULL) {

I would have just written 'if (local_err) {'; but that's minor style.

Looks like a clean refactoring, other than the nit about the trailing
'.', so with that fixed:
Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3998 bytes --]

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> 'name' attribute is made mandatory in distinction with HMP command.
> 
> The patch also moves hmp_savevm implementation into hmp.c. This function
> is just a simple wrapper now and does not have knowledge about
> migration internals.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  hmp.c              | 12 ++++++++++++
>  migration/savevm.c | 13 +------------
>  qapi-schema.json   | 13 +++++++++++++
>  qmp-commands.hx    | 25 +++++++++++++++++++++++++
>  4 files changed, 51 insertions(+), 12 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 2140605..c9c7100 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -32,6 +32,7 @@
>  #include "ui/console.h"
>  #include "block/qapi.h"
>  #include "qemu-io.h"
> +#include "sysemu/sysemu.h"

What is this header needed for?

>  
>  #ifdef CONFIG_SPICE
>  #include <spice/enums.h>
> @@ -2378,3 +2379,14 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
>  
>      qapi_free_RockerOfDpaGroupList(list);
>  }
> +
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;

I've been favoring 'err' rather than 'local_err', but that's not a
show-stopper since we still have both spellings mixed in the tree.

> +
> +    qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err);

qdict_get_try_str() can return NULL;, but qmp_savevm() requires a
non-NULL string.  You'll need to invent a name in HMP if the user didn't
give you one.

> +++ b/migration/savevm.c
> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>      return ret;
>  }
>  
> -static void do_savevm(const char *name, Error **errp)
> +void qmp_savevm(const char *name, Error **errp)
>  {

This function still has some 'if (name)' conditionals; but according to
the qapi contract, name should never be NULL. You need to hoist the name
creation aspect to the caller (hmp_savevm), and in qmp_savevm you can
then assert(name).

>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp)
>      }
>  }
>  
> -void hmp_savevm(Monitor *mon, const QDict *qdict)
> -{
> -    Error *local_err = NULL;
> -
> -    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);

Hmm.  You are doing code motion of code you just added in 1/5; wouldn't
it be better if 1/5 stuck it in the right file to begin with?  And my
argument about needing the name logic in HMP (not QMP) means that maybe
the split in 1/5 isn't ideal, after all.

> +++ b/qapi-schema.json
> @@ -3971,3 +3971,16 @@
>  ##
>  { 'enum': 'ReplayMode',
>    'data': [ 'none', 'record', 'play' ] }
> +
> +##
> +# @savevm
> +#
> +# Save a VM snapshot. Without a name new snapshot is created",

Stale sentence, since a name is required.

> +#
> +# @name: identifier of a snapshot to be created
> +#
> +# Returns: Nothing on success
> +#
> +# Since 2.6
> +##
> +{ 'command': 'savevm', 'data': {'name': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9d8b42f..f216c5e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -4739,3 +4739,28 @@ Example:
>                   {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>                    "pop-vlan": 1, "id": 251658240}
>     ]}
> +
> +EQMP
> +
> +SQMP
> +savevm
> +------------------

Line too long. Make it match the length of the line above.

> +
> +Save a VM snapshot. If no tag or id are provided, a new snapshot is created

Stale sentence, since you are requiring a name.

Why does QMP not allow specifying an id?  Should it?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Denis V. Lunev]

On 12/24/2015 12:40 AM, Eric Blake wrote:
> On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
>> 'name' attribute is made mandatory in distinction with HMP command.
>>
>> The patch also moves hmp_savevm implementation into hmp.c. This function
>> is just a simple wrapper now and does not have knowledge about
>> migration internals.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> ---
>>   hmp.c              | 12 ++++++++++++
>>   migration/savevm.c | 13 +------------
>>   qapi-schema.json   | 13 +++++++++++++
>>   qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>   4 files changed, 51 insertions(+), 12 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 2140605..c9c7100 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -32,6 +32,7 @@
>>   #include "ui/console.h"
>>   #include "block/qapi.h"
>>   #include "qemu-io.h"
>> +#include "sysemu/sysemu.h"
> What is this header needed for?
>
>>   
>>   #ifdef CONFIG_SPICE
>>   #include <spice/enums.h>
>> @@ -2378,3 +2379,14 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
>>   
>>       qapi_free_RockerOfDpaGroupList(list);
>>   }
>> +
>> +void hmp_savevm(Monitor *mon, const QDict *qdict)
>> +{
>> +    Error *local_err = NULL;
> I've been favoring 'err' rather than 'local_err', but that's not a
> show-stopper since we still have both spellings mixed in the tree.
>
>> +
>> +    qmp_savevm(qdict_get_try_str(qdict, "name"), &local_err);
> qdict_get_try_str() can return NULL;, but qmp_savevm() requires a
> non-NULL string.  You'll need to invent a name in HMP if the user didn't
> give you one.
>
>> +++ b/migration/savevm.c
>> @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
>>       return ret;
>>   }
>>   
>> -static void do_savevm(const char *name, Error **errp)
>> +void qmp_savevm(const char *name, Error **errp)
>>   {
> This function still has some 'if (name)' conditionals; but according to
> the qapi contract, name should never be NULL. You need to hoist the name
> creation aspect to the caller (hmp_savevm), and in qmp_savevm you can
> then assert(name).
>
>>       BlockDriverState *bs, *bs1;
>>       QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
>> @@ -1999,17 +1999,6 @@ static void do_savevm(const char *name, Error **errp)
>>       }
>>   }
>>   
>> -void hmp_savevm(Monitor *mon, const QDict *qdict)
>> -{
>> -    Error *local_err = NULL;
>> -
>> -    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
> Hmm.  You are doing code motion of code you just added in 1/5; wouldn't
> it be better if 1/5 stuck it in the right file to begin with?  And my
> argument about needing the name logic in HMP (not QMP) means that maybe
> the split in 1/5 isn't ideal, after all.
I'll need a header change in 1/5 and subsequent removal of the item here
This was the motto of doing thing this way.


>> +++ b/qapi-schema.json
>> @@ -3971,3 +3971,16 @@
>>   ##
>>   { 'enum': 'ReplayMode',
>>     'data': [ 'none', 'record', 'play' ] }
>> +
>> +##
>> +# @savevm
>> +#
>> +# Save a VM snapshot. Without a name new snapshot is created",
> Stale sentence, since a name is required.
>
>> +#
>> +# @name: identifier of a snapshot to be created
>> +#
>> +# Returns: Nothing on success
>> +#
>> +# Since 2.6
>> +##
>> +{ 'command': 'savevm', 'data': {'name': 'str'} }
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 9d8b42f..f216c5e 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -4739,3 +4739,28 @@ Example:
>>                    {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
>>                     "pop-vlan": 1, "id": 251658240}
>>      ]}
>> +
>> +EQMP
>> +
>> +SQMP
>> +savevm
>> +------------------
> Line too long. Make it match the length of the line above.
>
>> +
>> +Save a VM snapshot. If no tag or id are provided, a new snapshot is created
> Stale sentence, since you are requiring a name.
>
> Why does QMP not allow specifying an id?  Should it?
>
ok. this is a something to think on.

Re: [Qemu-devel] [PATCH 3/5] qmp: create qmp_delvm command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> The patch also moves hmp_delvm implementation into hmp.c. This function
> is just a simple wrapper now and does not have knowledge about
> migration internals.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  hmp.c              | 11 +++++++++++
>  migration/savevm.c | 16 +++++++---------
>  qapi-schema.json   | 13 +++++++++++++
>  qmp-commands.hx    | 23 +++++++++++++++++++++++
>  4 files changed, 54 insertions(+), 9 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index c9c7100..a342c8c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2390,3 +2390,14 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>          error_report_err(local_err);
>      }
>  }
> +
> +void hmp_delvm(Monitor *mon, const QDict *qdict)
> +{
> +    Error *local_err = NULL;

Same comment as earlier, about 'err' vs. 'local_err' naming,

> +
> +    qmp_delvm(qdict_get_str(qdict, "name"), &local_err);
> +
> +    if (local_err != NULL) {

and redundant comparison to NULL.

> +++ b/migration/savevm.c
> @@ -2099,17 +2099,15 @@ int load_vmstate(const char *name)
>      return 0;
>  }
>  
> -void hmp_delvm(Monitor *mon, const QDict *qdict)
> +void qmp_delvm(const char *name, Error **errp)
>  {
>      BlockDriverState *bs;
> -    Error *err;
> -    const char *name = qdict_get_str(qdict, "name");
> -
> -    if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
> -        monitor_printf(mon,
> -                       "Error while deleting snapshot on device '%s': %s\n",
> -                       bdrv_get_device_name(bs), error_get_pretty(err));
> -        error_free(err);
> +    Error *local_err = NULL;

Instead of renaming 'err' to 'local_err',

> +
> +    if (bdrv_all_delete_snapshot(name, &bs, &local_err) < 0) {
> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
> +                   bdrv_get_device_name(bs), error_get_pretty(local_err));

you'll want to use Markus' proposed error prefixing code.


> +SQMP
> +delvm
> +------------------

Divider too long.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/5] migration: improve error reporting for load_vmstate

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4198 bytes --]

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> The patch adds Error ** parameter to load_vmstate call and fills error
> inside. The caller after that properly reports error either through
> monitor or via local stderr facility during VM start.
> 
> This helper will be useful too for qmp_loadvm implementation.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  include/sysemu/sysemu.h |  2 +-
>  migration/savevm.c      | 28 ++++++++++++++++------------
>  monitor.c               |  6 +++++-
>  vl.c                    |  4 +++-
>  4 files changed, 25 insertions(+), 15 deletions(-)
> 

> -int load_vmstate(const char *name)
> +int load_vmstate(const char *name, Error **errp)
>  {
>      BlockDriverState *bs, *bs_vm_state;
>      QEMUSnapshotInfo sn;
> @@ -2035,20 +2035,22 @@ int load_vmstate(const char *name)
>      AioContext *aio_context;
>  
>      if (!bdrv_all_can_snapshot(&bs)) {
> -        error_report("Device '%s' is writable but does not support snapshots.",
> -                     bdrv_get_device_name(bs));
> +        error_setg(errp,
> +                   "Device '%s' is writable but does not support snapshots.",

No trailing '.'

> @@ -2058,10 +2060,11 @@ int load_vmstate(const char *name)
>      ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
>      aio_context_release(aio_context);
>      if (ret < 0) {
> +        error_setg_errno(errp, ret, "Snapshot '%s' not found", name);

You need '-ret', since error_setg_errno() expects positive errno values.

[maybe, for convenience, we should teach error_setg_errno() to handle
both -EINVAL and EINVAL identically - but it would need good
justification and would touch a lot of the tree, so if we do it, it
would be a separate series]

>          return ret;
>      } else if (sn.vm_state_size == 0) {
> -        error_report("This is a disk-only snapshot. Revert to it offline "
> -            "using qemu-img.");
> +        error_setg(errp, "This is a disk-only snapshot. Revert to it offline "
> +                   "using qemu-img.");

According to Markus' recent cleanups, error_setg() should be a single
phrase, not two sentences.  You'll want the second sentence to be added
with error_append_hint():
https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03508.html

>          return -EINVAL;
>      }
>  
> @@ -2070,15 +2073,16 @@ int load_vmstate(const char *name)
>  
>      ret = bdrv_all_goto_snapshot(name, &bs);
>      if (ret < 0) {
> -        error_report("Error %d while activating snapshot '%s' on '%s'",
> -                     ret, name, bdrv_get_device_name(bs));
> +        error_setg_errno(errp, ret,

-ret again

> +                         "Error while activating snapshot '%s' on '%s'",
> +                         name, bdrv_get_device_name(bs));

but thanks for getting rid of the weird 'error -5 while...' in the
message :)

> @@ -2092,7 +2096,7 @@ int load_vmstate(const char *name)
>  
>      migration_incoming_state_destroy();
>      if (ret < 0) {
> -        error_report("Error %d while loading VM state", ret);
> +        error_setg_errno(errp, ret, "Error while loading VM state");

-ret again

> +++ b/monitor.c
> @@ -1739,10 +1739,14 @@ static void hmp_loadvm(Monitor *mon, const QDict *qdict)
>  {
>      int saved_vm_running  = runstate_is_running();
>      const char *name = qdict_get_str(qdict, "name");
> +    Error *local_err = NULL;
>  
>      vm_stop(RUN_STATE_RESTORE_VM);
>  
> -    if (load_vmstate(name) == 0 && saved_vm_running) {
> +    if (load_vmstate(name, &local_err) < 0) {
> +        error_report_err(local_err);
> +    }
> +    if (saved_vm_running) {
>          vm_start();

Logic bug.  You blindly fall through, and could end up attempting
vm_start() even after an error, where previously it was not possible.
You probably meant '} else if (saved_vm_running) {'.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/5] qmp: create QMP implementation of loadvm command

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2234 bytes --]

On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
> Unfortunately load_vmstate has a return code (int) and this code is checked
> in the other places. Thus we could not just rename it to qmp_loadvm as
> returns void.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  migration/savevm.c | 12 ++++++++++++
>  monitor.c          | 12 ++++++------
>  qapi-schema.json   | 13 +++++++++++++
>  qmp-commands.hx    | 23 +++++++++++++++++++++++
>  4 files changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 7846437..07b0bf4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2103,6 +2103,18 @@ int load_vmstate(const char *name, Error **errp)
>      return 0;
>  }
>  
> +void qmp_loadvm(const char *name, Error **errp)
> +{
> +    int saved_vm_running  = runstate_is_running();
> +    vm_stop(RUN_STATE_RESTORE_VM);
> +
> +    load_vmstate(name, errp);
> +
> +    if (saved_vm_running) {
> +        vm_start();

Do you really mean to be calling vm_start() even if an error was
reported?  But to properly short-circuit, you need a local 'Error *err =
NULL; load_vmstate(name, &err);', rather than reusing errp (since the
caller may pass in NULL).

> +++ b/monitor.c
> @@ -1737,18 +1737,18 @@ void qmp_closefd(const char *fdname, Error **errp)
>  
>  static void hmp_loadvm(Monitor *mon, const QDict *qdict)
>  {
> -    int saved_vm_running  = runstate_is_running();
>      const char *name = qdict_get_str(qdict, "name");
>      Error *local_err = NULL;
>  
> -    vm_stop(RUN_STATE_RESTORE_VM);
> +    if (name == NULL) {

Dead code.  qdict_get_str() crashes rather than returning NULL, if
"name" is not present.

> +++ b/qmp-commands.hx
> @@ -4787,3 +4787,26 @@ EQMP
>          .args_type  = "name:s",
>          .mhandler.cmd_new = qmp_marshal_delvm,
>      },
> +
> +SQMP
> +loadvm
> +------------------

Line too long.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Denis V. Lunev]

On 12/24/2015 12:27 AM, Eric Blake wrote:
> On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
>> This would be useful in the next step when QMP version of this call will
>> be introduced.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> ---
>>   migration/savevm.c | 38 +++++++++++++++++++++++---------------
>>   1 file changed, 23 insertions(+), 15 deletions(-)
>>
>> @@ -1915,28 +1915,27 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
>>       uint64_t vm_state_size;
>>       qemu_timeval tv;
>>       struct tm tm;
>> -    const char *name = qdict_get_try_str(qdict, "name");
>>       Error *local_err = NULL;
>>       AioContext *aio_context;
>>   
>>       if (!bdrv_all_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.",
> No trailing '.' in error_setg() calls.
>
>> +                   bdrv_get_device_name(bs));
>>           return;
>>       }
>>   
>>       /* Delete old snapshots of the same name */
>>       if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) < 0) {
>> -        monitor_printf(mon,
>> -                       "Error while deleting snapshot on device '%s': %s\n",
>> -                       bdrv_get_device_name(bs1), error_get_pretty(local_err));
>> +        error_setg(errp, "Error while deleting snapshot on device '%s': %s",
>> +                   bdrv_get_device_name(bs1), error_get_pretty(local_err));
> Markus' series to add a prefixing notation would be better to use here
> (although I didn't check if he caught this one in that series already):
> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html

this series is not yet merged. I think that we could do this refactoring 
later on.
This thing could be considered independent. Anyway, this series has its 
own value
and it takes a lot of time to push it in. Could we do  error setting 
improvement later on?

Messages are not changed etc. I'll change them as you suggest.


>>   
>> +void hmp_savevm(Monitor *mon, const QDict *qdict)
>> +{
>> +    Error *local_err = NULL;
>> +
>> +    do_savevm(qdict_get_try_str(qdict, "name"), &local_err);
>> +
>> +    if (local_err != NULL) {
> I would have just written 'if (local_err) {'; but that's minor style.
from my point of view explicit != NULL exposes that local_err is a
pointer rather than a boolean value.

> Looks like a clean refactoring, other than the nit about the trailing
> '.', so with that fixed:
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

Re: [Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Denis V. Lunev]

On 12/24/2015 12:40 AM, Eric Blake wrote:
> On 12/04/2015 07:44 AM, Denis V. Lunev wrote:
>> 'name' attribute is made mandatory in distinction with HMP command.
>>
>> The patch also moves hmp_savevm implementation into hmp.c. This function
>> is just a simple wrapper now and does not have knowledge about
>> migration internals.
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Amit Shah <amit.shah@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> ---
>>   hmp.c              | 12 ++++++++++++
>>   migration/savevm.c | 13 +------------
>>   qapi-schema.json   | 13 +++++++++++++
>>   qmp-commands.hx    | 25 +++++++++++++++++++++++++
>>   4 files changed, 51 insertions(+), 12 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 2140605..c9c7100 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -32,6 +32,7 @@
>>   #include "ui/console.h"
>>   #include "block/qapi.h"
>>   #include "qemu-io.h"
>> +#include "sysemu/sysemu.h"
> What is this header needed for?
>
it contains prototype of hmp_savevm. The following warning
appears without it:

irbis ~/src/qemu $ make -j8
   CC    hmp.o
hmp.c:2390:6: error: no previous prototype for ‘hmp_savevm’ 
[-Werror=missing-prototypes]
  void hmp_savevm(Monitor *mon, const QDict *qdict)
       ^
cc1: all warnings being treated as errors
/home/den/src/qemu/rules.mak:57: recipe for target 'hmp.o' failed
make: *** [hmp.o] Error 1
irbis ~/src/qemu $

[Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Denis V. Lunev]

'name' attribute is made mandatory in distinction with HMP command.

The patch also moves hmp_savevm implementation into hmp.c. This function
is just a simple wrapper now and does not have knowledge about
migration internals.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 hmp.c                         |  3 +--
 include/migration/migration.h |  2 --
 migration/savevm.c            |  2 +-
 qapi-schema.json              | 13 +++++++++++++
 qmp-commands.hx               | 25 +++++++++++++++++++++++++
 5 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/hmp.c b/hmp.c
index e7bab75..8d6eda6 100644
--- a/hmp.c
+++ b/hmp.c
@@ -33,7 +33,6 @@
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
-#include "migration/migration.h"
 
 #ifdef CONFIG_SPICE
 #include <spice/enums.h>
@@ -2406,7 +2405,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         name = name_buf;
     }
 
-    migrate_savevm(name, &local_err);
+    qmp_savevm(name, &local_err);
 
     if (local_err != NULL) {
         error_report_err(local_err);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 73c8bb1..58c04a9 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -277,8 +277,6 @@ int migrate_compress_threads(void);
 int migrate_decompress_threads(void);
 bool migrate_use_events(void);
 
-void migrate_savevm(const char *name, Error **errp);
-
 /* Sending on the return path - generic and then for each message type */
 void migrate_send_rp_message(MigrationIncomingState *mis,
                              enum mig_rp_message_type message_type,
diff --git a/migration/savevm.c b/migration/savevm.c
index 308302a..0dbb15f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }
 
-void migrate_savevm(const char *name, Error **errp)
+void qmp_savevm(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
diff --git a/qapi-schema.json b/qapi-schema.json
index 2e31733..09d1a1a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4054,3 +4054,16 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @savevm
+#
+# Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
+#
+# @name: identifier of a snapshot to be created
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'savevm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db072a6..b7851e1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4795,3 +4795,28 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+SQMP
+savevm
+------
+
+Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
+
+Arguments:
+
+- "name": snapshot name
+
+Example:
+
+-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "savevm",
+        .args_type  = "name:s",
+        .mhandler.cmd_new = qmp_marshal_savevm,
+    },
-- 
2.5.0

[Qemu-devel] [PATCH 2/5] qmp: create qmp_savevm command

[Denis V. Lunev]

'name' attribute is made mandatory in distinction with HMP command.

The patch also moves hmp_savevm implementation into hmp.c. This function
is just a simple wrapper now and does not have knowledge about
migration internals.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Juan Quintela <quintela@redhat.com>
CC: Amit Shah <amit.shah@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 hmp.c                         |  3 +--
 include/migration/migration.h |  2 --
 migration/savevm.c            |  2 +-
 qapi-schema.json              | 13 +++++++++++++
 qmp-commands.hx               | 25 +++++++++++++++++++++++++
 5 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/hmp.c b/hmp.c
index e7bab75..8d6eda6 100644
--- a/hmp.c
+++ b/hmp.c
@@ -33,7 +33,6 @@
 #include "ui/console.h"
 #include "block/qapi.h"
 #include "qemu-io.h"
-#include "migration/migration.h"
 
 #ifdef CONFIG_SPICE
 #include <spice/enums.h>
@@ -2406,7 +2405,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
         name = name_buf;
     }
 
-    migrate_savevm(name, &local_err);
+    qmp_savevm(name, &local_err);
 
     if (local_err != NULL) {
         error_report_err(local_err);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 73c8bb1..58c04a9 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -277,8 +277,6 @@ int migrate_compress_threads(void);
 int migrate_decompress_threads(void);
 bool migrate_use_events(void);
 
-void migrate_savevm(const char *name, Error **errp);
-
 /* Sending on the return path - generic and then for each message type */
 void migrate_send_rp_message(MigrationIncomingState *mis,
                              enum mig_rp_message_type message_type,
diff --git a/migration/savevm.c b/migration/savevm.c
index 308302a..0dbb15f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
     return ret;
 }
 
-void migrate_savevm(const char *name, Error **errp)
+void qmp_savevm(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
diff --git a/qapi-schema.json b/qapi-schema.json
index 2e31733..09d1a1a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4054,3 +4054,16 @@
 ##
 { 'enum': 'ReplayMode',
   'data': [ 'none', 'record', 'play' ] }
+
+##
+# @savevm
+#
+# Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
+#
+# @name: identifier of a snapshot to be created
+#
+# Returns: Nothing on success
+#
+# Since 2.6
+##
+{ 'command': 'savevm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db072a6..b7851e1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4795,3 +4795,28 @@ Example:
                  {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
                   "pop-vlan": 1, "id": 251658240}
    ]}
+
+EQMP
+
+SQMP
+savevm
+------
+
+Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
+
+Arguments:
+
+- "name": snapshot name
+
+Example:
+
+-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
+<- { "return": {} }
+
+EQMP
+
+    {
+        .name       = "savevm",
+        .args_type  = "name:s",
+        .mhandler.cmd_new = qmp_marshal_savevm,
+    },
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

On 01/08/2016 04:27 AM, Denis V. Lunev wrote:

>>>         /* Delete old snapshots of the same name */
>>>       if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) <
>>> 0) {
>>> -        monitor_printf(mon,
>>> -                       "Error while deleting snapshot on device
>>> '%s': %s\n",
>>> -                       bdrv_get_device_name(bs1),
>>> error_get_pretty(local_err));
>>> +        error_setg(errp, "Error while deleting snapshot on device
>>> '%s': %s",
>>> +                   bdrv_get_device_name(bs1),
>>> error_get_pretty(local_err));
>> Markus' series to add a prefixing notation would be better to use here
>> (although I didn't check if he caught this one in that series already):
>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html
> 
> this series is not yet merged. I think that we could do this refactoring
> later on.
> This thing could be considered independent. Anyway, this series has its
> own value
> and it takes a lot of time to push it in. Could we do  error setting
> improvement later on?

I don't care who rebases on top of the other, but maybe Markus will have
an opinion when he gets back online next week.


>>> +
>>> +    if (local_err != NULL) {
>> I would have just written 'if (local_err) {'; but that's minor style.
> from my point of view explicit != NULL exposes that local_err is a
> pointer rather than a boolean value.

But the code base already overwhelmingly relies on C's implicit
conversion of pointer to a boolean context, as it requires less typing;
being verbose doesn't make the code base any easier to read.  However,
since HACKING doesn't say one way or the other, I won't make you change.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Denis V. Lunev]

On 01/08/2016 07:14 PM, Eric Blake wrote:
> On 01/08/2016 04:27 AM, Denis V. Lunev wrote:
>
>>>>          /* Delete old snapshots of the same name */
>>>>        if (name && bdrv_all_delete_snapshot(name, &bs1, &local_err) <
>>>> 0) {
>>>> -        monitor_printf(mon,
>>>> -                       "Error while deleting snapshot on device
>>>> '%s': %s\n",
>>>> -                       bdrv_get_device_name(bs1),
>>>> error_get_pretty(local_err));
>>>> +        error_setg(errp, "Error while deleting snapshot on device
>>>> '%s': %s",
>>>> +                   bdrv_get_device_name(bs1),
>>>> error_get_pretty(local_err));
>>> Markus' series to add a prefixing notation would be better to use here
>>> (although I didn't check if he caught this one in that series already):
>>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html
>> this series is not yet merged. I think that we could do this refactoring
>> later on.
>> This thing could be considered independent. Anyway, this series has its
>> own value
>> and it takes a lot of time to push it in. Could we do  error setting
>> improvement later on?
> I don't care who rebases on top of the other, but maybe Markus will have
> an opinion when he gets back online next week.
>
why we have to wait with this set due to this reason?
The code with error_prepend and current code are BOTH
correct. One is a bit shorter then other. Yes, it would
be nice to switch to it, but why this should be done in
this set?

This set solves real problem which has not been addressed
for a long time. Let's proceed, cool and shiny stuff
could be done later on, when it will be merged.

Moreover, merging this set will make my life easier
with merging these changes to our downstream.
Fixes will be merged while improvements will stay
in upstream only.


>>>> +
>>>> +    if (local_err != NULL) {
>>> I would have just written 'if (local_err) {'; but that's minor style.
>> from my point of view explicit != NULL exposes that local_err is a
>> pointer rather than a boolean value.
> But the code base already overwhelmingly relies on C's implicit
> conversion of pointer to a boolean context, as it requires less typing;
> being verbose doesn't make the code base any easier to read.  However,
> since HACKING doesn't say one way or the other, I won't make you change.
>
I do not understand your last words.

I am not agitating you with one approach or another. This
is a reason why I am writing code this way. The code written
this way looks better to me. This code is NEW and this does
not contradict any written rule in coding style policy.

If the code is working and correct, can we just move on with it?

Den

Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2550 bytes --]

On 01/08/2016 09:40 AM, Denis V. Lunev wrote:

>>>> Markus' series to add a prefixing notation would be better to use here
>>>> (although I didn't check if he caught this one in that series already):
>>>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html
>>> this series is not yet merged. I think that we could do this refactoring
>>> later on.
>>> This thing could be considered independent. Anyway, this series has its
>>> own value
>>> and it takes a lot of time to push it in. Could we do  error setting
>>> improvement later on?
>> I don't care who rebases on top of the other, but maybe Markus will have
>> an opinion when he gets back online next week.
>>
> why we have to wait with this set due to this reason?

One of you will have to rebase on the other - either you wait for
Markus' error_prepend to go in and you use it, or your patch goes in and
Markus updates his error_prepend patch to cover your additional instance
that will be benefitted by it.  I don't care which, and the timing is
really up to the maintainers and how fast they send pull requests.

> The code with error_prepend and current code are BOTH
> correct. One is a bit shorter then other. Yes, it would
> be nice to switch to it, but why this should be done in
> this set?

Exactly, we're saying the same things.

>>>>> +    if (local_err != NULL) {
>>>> I would have just written 'if (local_err) {'; but that's minor style.
>>> from my point of view explicit != NULL exposes that local_err is a
>>> pointer rather than a boolean value.
>> But the code base already overwhelmingly relies on C's implicit
>> conversion of pointer to a boolean context, as it requires less typing;
>> being verbose doesn't make the code base any easier to read.  However,
>> since HACKING doesn't say one way or the other, I won't make you change.
>>
> I do not understand your last words.
> 
> I am not agitating you with one approach or another. This
> is a reason why I am writing code this way. The code written
> this way looks better to me. This code is NEW and this does
> not contradict any written rule in coding style policy.
> 
> If the code is working and correct, can we just move on with it?

Once again, we are saying the same thing.  I pointed out a cosmetic
issue, but one where I do not have a strong enough leg to stand on to
force you to change your style, so what you did is fine as is.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/5] migration: split hmp_savevm to do_savevm and hmp_savevm wrapper

[Denis V. Lunev]

On 01/08/2016 08:54 PM, Eric Blake wrote:
> On 01/08/2016 09:40 AM, Denis V. Lunev wrote:
>
>>>>> Markus' series to add a prefixing notation would be better to use here
>>>>> (although I didn't check if he caught this one in that series already):
>>>>> https://lists.gnu.org/archive/html/qemu-devel/2015-12/msg03495.html
>>>> this series is not yet merged. I think that we could do this refactoring
>>>> later on.
>>>> This thing could be considered independent. Anyway, this series has its
>>>> own value
>>>> and it takes a lot of time to push it in. Could we do  error setting
>>>> improvement later on?
>>> I don't care who rebases on top of the other, but maybe Markus will have
>>> an opinion when he gets back online next week.
>>>
>> why we have to wait with this set due to this reason?
> One of you will have to rebase on the other - either you wait for
> Markus' error_prepend to go in and you use it, or your patch goes in and
> Markus updates his error_prepend patch to cover your additional instance
> that will be benefitted by it.  I don't care which, and the timing is
> really up to the maintainers and how fast they send pull requests.
>
>> The code with error_prepend and current code are BOTH
>> correct. One is a bit shorter then other. Yes, it would
>> be nice to switch to it, but why this should be done in
>> this set?
> Exactly, we're saying the same things.
>
>>>>>> +    if (local_err != NULL) {
>>>>> I would have just written 'if (local_err) {'; but that's minor style.
>>>> from my point of view explicit != NULL exposes that local_err is a
>>>> pointer rather than a boolean value.
>>> But the code base already overwhelmingly relies on C's implicit
>>> conversion of pointer to a boolean context, as it requires less typing;
>>> being verbose doesn't make the code base any easier to read.  However,
>>> since HACKING doesn't say one way or the other, I won't make you change.
>>>
>> I do not understand your last words.
>>
>> I am not agitating you with one approach or another. This
>> is a reason why I am writing code this way. The code written
>> this way looks better to me. This code is NEW and this does
>> not contradict any written rule in coding style policy.
>>
>> If the code is working and correct, can we just move on with it?
> Once again, we are saying the same thing.  I pointed out a cosmetic
> issue, but one where I do not have a strong enough leg to stand on to
> force you to change your style, so what you did is fine as is.
>
ok. perfect to be on the same page :)

I'll promise to switch to error_prepend code when it will be
merged. I hope that v4 of the set is good enough to
proceed.

Den