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

[Qemu-devel] [PATCH 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.

At the moment I have placed 2.6 version into QAPI. Though (if you feel
appropriate) I can change it to 2.5 :) This is up to you to decide.

Please note, this patchset is made on top of
  [PATCH for 2.5 v8 0/10] dataplane snapshot fixes

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>

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(-)

-- 
2.5.0

[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>
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 | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 2c65ab2..f83ffd0 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,26 @@ 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 +1943,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 +1975,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 +1998,18 @@ 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) {
+        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
+        error_free(local_err);
+    }
+}
+
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
 {
     QEMUFile *f;
-- 
2.5.0

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

[Markus Armbruster]

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

> 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>
> 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 | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 2c65ab2..f83ffd0 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,26 @@ 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));

Since you're touching this already, please drop the period and clean up
the line wrapping:

       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 +1943,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 +1975,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 +1998,18 @@ 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) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> +        error_free(local_err);

Easier:

        error_report_err(local_err);

Already used elsewhere in this file.

> +    }
> +}
> +
>  void qmp_xen_save_devices_state(const char *filename, Error **errp)
>  {
>      QEMUFile *f;

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

[Juan Quintela]

"Denis V. Lunev" <den@openvz.org> 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>
> 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>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[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

[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

[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

[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 3/5] qmp: create qmp_delvm 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 | 27 ++++++++++++++++++---------
 qapi-schema.json   | 13 +++++++++++++
 qmp-commands.hx    | 23 +++++++++++++++++++++++
 3 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 565b10a..90b6850 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2115,17 +2115,26 @@ 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, errp) < 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);
+    }
+}
+
+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) {
+        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
+        error_free(local_err);
     }
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 8cc8b44..193b34f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3975,3 +3975,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 cd895f6..b2b17ff 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

Re: [Qemu-devel] [PATCH 3/5] qmp: create qmp_delvm 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 | 27 ++++++++++++++++++---------
>  qapi-schema.json   | 13 +++++++++++++
>  qmp-commands.hx    | 23 +++++++++++++++++++++++
>  3 files changed, 54 insertions(+), 9 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 565b10a..90b6850 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2115,17 +2115,26 @@ 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, errp) < 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);
> +    }
> +}
> +
> +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) {
> +        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> +        error_free(local_err);

error_report_err(), please.

>      }
>  }

Juan, Amit, in case you'd prefer to move out the parts that implement
HMP on top of QMP: they can go into hmp.c as long as they're as simple
as this one.

> diff --git a/qapi-schema.json b/qapi-schema.json
> index 8cc8b44..193b34f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3975,3 +3975,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 cd895f6..b2b17ff 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,
> +    },

Re: [Qemu-devel] [PATCH 3/5] qmp: create qmp_delvm 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 | 27 ++++++++++++++++++---------
>>  qapi-schema.json   | 13 +++++++++++++
>>  qmp-commands.hx    | 23 +++++++++++++++++++++++
>>  3 files changed, 54 insertions(+), 9 deletions(-)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 565b10a..90b6850 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2115,17 +2115,26 @@ 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, errp) < 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);
>> +    }
>> +}
>> +
>> +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) {
>> +        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
>> +        error_free(local_err);
>
> error_report_err(), please.
>
>>      }
>>  }
>
> Juan, Amit, in case you'd prefer to move out the parts that implement
> HMP on top of QMP: they can go into hmp.c as long as they're as simple
> as this one.

Perfect for me.

Thanks, Juan.

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

[Juan Quintela]

"Denis V. Lunev" <den@openvz.org> wrote:
> 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>

I agree with the changes, move them to hmp.c as Markus suggests.

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

[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 usefull too for qmp_load_vmstate 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      | 25 +++++++++++++------------
 monitor.c               |  7 ++++++-
 vl.c                    |  5 ++++-
 4 files changed, 24 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 90b6850..08c6c65 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2042,7 +2042,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;
@@ -2051,20 +2051,20 @@ 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, true, &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);
@@ -2074,10 +2074,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;
     }
 
@@ -2086,15 +2087,15 @@ 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;
     }
 
@@ -2108,7 +2109,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 e4cf34e..1a0c932 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1739,10 +1739,15 @@ 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) {
+        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
+        error_free(local_err);
+    }
+    if (saved_vm_running) {
         vm_start();
     }
 }
diff --git a/vl.c b/vl.c
index 7d993a5..9912366 100644
--- a/vl.c
+++ b/vl.c
@@ -4645,8 +4645,11 @@ 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("%s", error_get_pretty(local_err));
+            error_free(local_err);
         }
     }
 
-- 
2.5.0

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

[Markus Armbruster]

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

> 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 usefull too for qmp_load_vmstate 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      | 25 +++++++++++++------------
>  monitor.c               |  7 ++++++-
>  vl.c                    |  5 ++++-
>  4 files changed, 24 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 90b6850..08c6c65 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2042,7 +2042,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;
> @@ -2051,20 +2051,20 @@ 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));

Since you're touching this already, please drop the period.

I'd prefer breaking lines between arguments instead of in the middle of
an argument:

        error_setg(errp,
                   "Device '%s' is writable but does not support snapshots",
                   bdrv_get_device_name(bs));

I'll continue to review when I have something that applies.

[...]

[Qemu-devel] [PATCH 5/5] qmp: create QMP implementation of loadvm 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    | 23 +++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 08c6c65..f67b5d9 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2116,6 +2116,11 @@ int load_vmstate(const char *name, Error **errp)
     return 0;
 }
 
+void qmp_loadvm(const char *name, Error **errp)
+{
+    load_vmstate(name, errp);
+}
+
 void qmp_delvm(const char *name, Error **errp)
 {
     BlockDriverState *bs;
diff --git a/qapi-schema.json b/qapi-schema.json
index 193b34f..d133711 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3988,3 +3988,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 b2b17ff..d5f80a1 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 5/5] qmp: create QMP implementation of loadvm command

[Juan Quintela]

"Denis V. Lunev" <den@openvz.org> wrote:
> 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    | 23 +++++++++++++++++++++++
>  3 files changed, 41 insertions(+)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 08c6c65..f67b5d9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2116,6 +2116,11 @@ int load_vmstate(const char *name, Error **errp)
>      return 0;
>  }
>  
> +void qmp_loadvm(const char *name, Error **errp)
> +{
> +    load_vmstate(name, errp);
> +}
> +

As Markus suggested for the other functions,  rename load_vmstate qmp_loadvm()

Thanks, Juan.

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

[Denis V. Lunev]

On 11/18/2015 02:41 PM, Juan Quintela wrote:
> "Denis V. Lunev" <den@openvz.org> wrote:
>> 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    | 23 +++++++++++++++++++++++
>>   3 files changed, 41 insertions(+)
>>
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index 08c6c65..f67b5d9 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -2116,6 +2116,11 @@ int load_vmstate(const char *name, Error **errp)
>>       return 0;
>>   }
>>   
>> +void qmp_loadvm(const char *name, Error **errp)
>> +{
>> +    load_vmstate(name, errp);
>> +}
>> +
> As Markus suggested for the other functions,  rename load_vmstate qmp_loadvm()
>
> Thanks, Juan.
load_vmstate is actually 'int load_vmstate' and return
code is checked in qemu_img.c

Should I replace error code check there with a check
to *errp != 0 or this would lead us into a problem
with block level guys?

Den

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

[Markus Armbruster]

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

> 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.
>
> At the moment I have placed 2.6 version into QAPI. Though (if you feel
> appropriate) I can change it to 2.5 :) This is up to you to decide.

Nice try, but I the soft freeze boat has sailed.

> Please note, this patchset is made on top of
>   [PATCH for 2.5 v8 0/10] dataplane snapshot fixes

That one applies fine, but this one on top doesn't.  Rebase?

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

[Denis V. Lunev]

On 11/17/2015 01:33 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <den@openvz.org> writes:
>
>> 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.
>>
>> At the moment I have placed 2.6 version into QAPI. Though (if you feel
>> appropriate) I can change it to 2.5 :) This is up to you to decide.
> Nice try, but I the soft freeze boat has sailed.
OK. no problem

>> Please note, this patchset is made on top of
>>    [PATCH for 2.5 v8 0/10] dataplane snapshot fixes
> That one applies fine, but this one on top doesn't.  Rebase?
yep. The difference is really small there (just additional 'true' parameter
in bdrv_find_all_snapshot). I think that I'll wait a day or two before
rework to give Juan and Amit their chances to blame me :)

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 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

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 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 v3 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 v2:
- patches 1/2 are resplit to move processing HMP specific handling
  of snapshot name generation to exclusive HMP code
- removed all '.' at the end of error_setg strings
- fixed too long lines with '-' in qmp-commands.hx
- error_setg_errno errno passing is fixed (-ret)
- fixed logical error in hmp_loadvm (vm_start on error)
- NOT switched to error_prepend code (it is not yet merged). Can we do this
  later? This will make my life easear merging code to our downstream.

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):
  qmp: process system-reset event in paused state
  qmp: create qmp_savevm command
  qmp: create qmp_delvm command
  migration: improve error reporting for load_vmstate
  qmp: create QMP implementation of loadvm command

 hmp.c                         | 14 +++++++--
 include/migration/migration.h |  2 --
 include/sysemu/sysemu.h       |  2 +-
 migration/savevm.c            | 56 ++++++++++++++++++++--------------
 monitor.c                     |  9 +++---
 qapi-schema.json              | 39 ++++++++++++++++++++++++
 qmp-commands.hx               | 71 +++++++++++++++++++++++++++++++++++++++++++
 qmp.c                         |  4 +++
 vl.c                          |  4 ++-
 9 files changed, 168 insertions(+), 33 deletions(-)

-- 
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

[Qemu-devel] [PATCH v4 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 v3:
- wrong patch 1 is replaced

Changes from v2:
- patches 1/2 are resplit to move processing HMP specific handling
  of snapshot name generation to exclusive HMP code
- removed all '.' at the end of error_setg strings
- fixed too long lines with '-' in qmp-commands.hx
- error_setg_errno errno passing is fixed (-ret)
- fixed logical error in hmp_loadvm (vm_start on error)
- NOT switched to error_prepend code (it is not yet merged). Can we do this
  later? This will make my life easear merging code to our downstream.

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 migrate_savevm and hmp_savevm wrapper
  qmp: create qmp_savevm command
  qmp: create qmp_delvm command
  migration: improve error reporting for load_vmstate
  qmp: create QMP implementation of loadvm command

 hmp.c                         |  36 +++++++++++++++
 include/migration/migration.h |   1 +
 include/sysemu/sysemu.h       |   2 +-
 migration/savevm.c            | 100 +++++++++++++++++++++---------------------
 monitor.c                     |   9 ++--
 qapi-schema.json              |  39 ++++++++++++++++
 qmp-commands.hx               |  71 ++++++++++++++++++++++++++++++
 vl.c                          |   4 +-
 8 files changed, 206 insertions(+), 56 deletions(-)

-- 
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