[Qemu-devel] [PATCH v2 00/12] convert savevm to use qapi and introduce qmp command

[Qemu-devel] [PATCH v2 00/12] convert savevm to use qapi and introduce qmp command

[Pavel Hrdina]

This patch series convert the savevm command into qapi and introduce QMP
command vm-snapshot-save.
It also rewrite error report for functions used by this command.

The patch 10/11 introduce new functionality of savevm and vm-snapshot-save
that you cannot override existing snapshot without using 'force' parameter.

The last patch improves speed of savevm. Previous buffer was too small for
writing into block devices.

Changes from v1:
    - rebase on current master branch
    - improve the speed of savevm
    - name parameter remains optionl for HMP and QMP

Pavel Hrdina (12):
  block: add error parameter to bdrv_snapshot_create() and related
    functions
  block: add error parameter to del_existing_snapshots()
  savevm: add error parameter to qemu_savevm_state_begin()
  savevm: add error parameter to qemu_savevm_state_iterate()
  savevm: add error parameter to qemu_savevm_state_complete()
  savevm: add error parameter to qemu_savevm_state()
  qapi: Convert savevm
  qemu-img: introduce qemu_img_handle_error
  block: update return value from bdrv_snapshot_create
  savevm: update return value from qemu_savevm_state
  vm-snapshot-save: add force parameter
  savevm: icrease the IO_BUF_SIZE to improve the speed of savevm

 block.c                   | 26 +++++++++++-----
 block/qcow2-snapshot.c    | 13 ++++++--
 block/qcow2.h             |  4 ++-
 block/rbd.c               | 16 ++++++----
 block/sheepdog.c          | 21 ++++++-------
 hmp-commands.hx           | 17 ++++++-----
 hmp.c                     | 10 +++++++
 hmp.h                     |  1 +
 include/block/block.h     |  3 +-
 include/block/block_int.h |  3 +-
 include/sysemu/sysemu.h   |  8 ++---
 migration.c               |  6 ++--
 qapi-schema.json          | 21 +++++++++++++
 qemu-img.c                | 18 +++++++----
 qmp-commands.hx           | 33 ++++++++++++++++++++
 savevm.c                  | 76 ++++++++++++++++++++++++++---------------------
 16 files changed, 192 insertions(+), 84 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v2 01/12] block: add error parameter to bdrv_snapshot_create() and related functions

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block.c                   | 22 ++++++++++++++++------
 block/qcow2-snapshot.c    |  9 ++++++++-
 block/qcow2.h             |  4 +++-
 block/rbd.c               |  8 ++++++--
 block/sheepdog.c          | 17 +++++++++--------
 include/block/block.h     |  3 ++-
 include/block/block_int.h |  3 ++-
 qemu-img.c                |  2 +-
 savevm.c                  |  2 +-
 9 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 0a062c9..d1b9034 100644
--- a/block.c
+++ b/block.c
@@ -3202,15 +3202,25 @@ BlockDriverState *bdrv_snapshots(void)
 }
 
 int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info)
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp)
 {
     BlockDriver *drv = bs->drv;
-    if (!drv)
+
+    if (!drv) {
+        error_setg(errp, "Device '%s' has no medium.",
+                   bdrv_get_device_name(bs));
         return -ENOMEDIUM;
-    if (drv->bdrv_snapshot_create)
-        return drv->bdrv_snapshot_create(bs, sn_info);
-    if (bs->file)
-        return bdrv_snapshot_create(bs->file, sn_info);
+    }
+    if (drv->bdrv_snapshot_create) {
+        return drv->bdrv_snapshot_create(bs, sn_info, errp);
+    }
+    if (bs->file) {
+        return bdrv_snapshot_create(bs->file, sn_info, errp);
+    }
+
+    error_setg(errp, "Snapshot is not supported for '%s'.",
+               bdrv_get_format_name(bs));
     return -ENOTSUP;
 }
 
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 992a5c8..d6be9a6 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -315,7 +315,9 @@ static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
 }
 
 /* if no id is provided, a new one is constructed */
-int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+int qcow2_snapshot_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *new_snapshot_list = NULL;
@@ -334,6 +336,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
     /* Check that the ID is unique */
     if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+        error_setg(errp, "Parameter 'name' has to be unique ID.");
         return -EEXIST;
     }
 
@@ -351,6 +354,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     l1_table_offset = qcow2_alloc_clusters(bs, s->l1_size * sizeof(uint64_t));
     if (l1_table_offset < 0) {
         ret = l1_table_offset;
+        error_setg(errp, "Failed to allocate L1 table.");
         goto fail;
     }
 
@@ -365,6 +369,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
                       s->l1_size * sizeof(uint64_t));
     if (ret < 0) {
+        error_setg(errp, "Failed to save L1 table.");
         goto fail;
     }
 
@@ -378,6 +383,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
      */
     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1);
     if (ret < 0) {
+        error_setg(errp, "Failed to update snapshot refcount.");
         goto fail;
     }
 
@@ -395,6 +401,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     if (ret < 0) {
         g_free(s->snapshots);
         s->snapshots = old_snapshot_list;
+        error_setg(errp, "Failed to write new snapshots.");
         goto fail;
     }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index e4b5e11..35ef293 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -353,7 +353,9 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
 int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
 
 /* qcow2-snapshot.c functions */
-int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
+int qcow2_snapshot_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp);
 int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
 int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
diff --git a/block/rbd.c b/block/rbd.c
index 8cd10a7..914f9fa 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -815,12 +815,14 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
 }
 
 static int qemu_rbd_snap_create(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info)
+                                QEMUSnapshotInfo *sn_info,
+                                Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     int r;
 
     if (sn_info->name[0] == '\0') {
+        error_setg(errp, "Parameter 'name' cannot be empty.");
         return -EINVAL; /* we need a name for rbd snapshots */
     }
 
@@ -830,16 +832,18 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
      */
     if (sn_info->id_str[0] != '\0' &&
         strcmp(sn_info->id_str, sn_info->name) != 0) {
+        error_setg(errp, "ID and name have to be equal.");
         return -EINVAL;
     }
 
     if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
+        error_setg(errp, "Parameter 'name' has to be shorter that 127 chars.");
         return -ERANGE;
     }
 
     r = rbd_snap_create(s->image, sn_info->name);
     if (r < 0) {
-        error_report("failed to create snap: %s", strerror(-r));
+        error_setg(errp, "Failed to create snapshot: '%s'.", strerror(-r));
         return r;
     }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 54d3e53..f4ee80e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1766,7 +1766,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
     return acb->ret;
 }
 
-static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
+static int sd_snapshot_create(BlockDriverState *bs,
+                              QEMUSnapshotInfo *sn_info,
+                              Error **errp)
 {
     BDRVSheepdogState *s = bs->opaque;
     int ret, fd;
@@ -1779,9 +1781,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
             s->name, sn_info->vm_state_size, s->is_snapshot);
 
     if (s->is_snapshot) {
-        error_report("You can't create a snapshot of a snapshot VDI, "
-                     "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
-
+        error_setg(errp, "You can't create a snapshot '%s' of a VDI snapshot.",
+                   sn_info->name);
         return -EINVAL;
     }
 
@@ -1800,21 +1801,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     fd = connect_to_sdog(s);
     if (fd < 0) {
         ret = fd;
+        error_setg(errp, "Failed to connect to sdog.");
         goto cleanup;
     }
 
     ret = write_object(fd, (char *)&s->inode, vid_to_vdi_oid(s->inode.vdi_id),
                        s->inode.nr_copies, datalen, 0, false, s->cache_flags);
     if (ret < 0) {
-        error_report("failed to write snapshot's inode.");
+        error_setg(errp, "Failed to write snapshot's inode.");
         goto cleanup;
     }
 
     ret = do_sd_create(s, s->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid,
                        1);
     if (ret < 0) {
-        error_report("failed to create inode for snapshot. %s",
-                     strerror(errno));
+        error_setg(errp, "Failed to create inode for snapshot.");
         goto cleanup;
     }
 
@@ -1824,7 +1825,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
                       s->inode.nr_copies, datalen, 0, s->cache_flags);
 
     if (ret < 0) {
-        error_report("failed to read new inode info. %s", strerror(errno));
+        error_setg(errp, "Failed to read new inode info.");
         goto cleanup;
     }
 
diff --git a/include/block/block.h b/include/block/block.h
index d4f34d6..5276eca 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -331,7 +331,8 @@ int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);
 int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info);
+                         QEMUSnapshotInfo *sn_info,
+                         Error **errp);
 int bdrv_snapshot_goto(BlockDriverState *bs,
                        const char *snapshot_id);
 int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ce0aa26..5247dda 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -149,7 +149,8 @@ struct BlockDriver {
                                  const uint8_t *buf, int nb_sectors);
 
     int (*bdrv_snapshot_create)(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info);
+                                QEMUSnapshotInfo *sn_info,
+                                Error **errp);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                               const char *snapshot_id);
     int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
diff --git a/qemu-img.c b/qemu-img.c
index 31627b0..21d02bf 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2012,7 +2012,7 @@ static int img_snapshot(int argc, char **argv)
         sn.date_sec = tv.tv_sec;
         sn.date_nsec = tv.tv_usec * 1000;
 
-        ret = bdrv_snapshot_create(bs, &sn);
+        ret = bdrv_snapshot_create(bs, &sn, NULL);
         if (ret) {
             error_report("Could not create snapshot '%s': %d (%s)",
                 snapshot_name, ret, strerror(-ret));
diff --git a/savevm.c b/savevm.c
index 35c8d1e..fadde55 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2218,7 +2218,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         if (bdrv_can_snapshot(bs1)) {
             /* Write VM state size only to the image that contains the state */
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
-            ret = bdrv_snapshot_create(bs1, sn);
+            ret = bdrv_snapshot_create(bs1, sn, NULL);
             if (ret < 0) {
                 monitor_printf(mon, "Error while creating snapshot on '%s'\n",
                                bdrv_get_device_name(bs1));
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 01/12] block: add error parameter to bdrv_snapshot_create() and related functions

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  block.c                   | 22 ++++++++++++++++------
>  block/qcow2-snapshot.c    |  9 ++++++++-
>  block/qcow2.h             |  4 +++-
>  block/rbd.c               |  8 ++++++--
>  block/sheepdog.c          | 17 +++++++++--------
>  include/block/block.h     |  3 ++-
>  include/block/block_int.h |  3 ++-
>  qemu-img.c                |  2 +-
>  savevm.c                  |  2 +-
>  9 files changed, 48 insertions(+), 22 deletions(-)
> 

>  int bdrv_snapshot_create(BlockDriverState *bs,
> -                         QEMUSnapshotInfo *sn_info)
> +                         QEMUSnapshotInfo *sn_info,
> +                         Error **errp)
>  {
>      BlockDriver *drv = bs->drv;
> -    if (!drv)
> +
> +    if (!drv) {
> +        error_setg(errp, "Device '%s' has no medium.",

In general, error_setg() should not print a trailing '.' (only two
offenders to git grep 'error_setg.*\."').  I think we also tend to start
messages with lower case, although that's not as obvious (49 cases of
'error_setg[^"*"[A-Z]' vs. 121 of error_setg[^"]*"[a-z]').

> +
> +    error_setg(errp, "Snapshot is not supported for '%s'.",

And again, and probably throughout your series (although I'll quit
pointing it out).

> @@ -830,16 +832,18 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
>       */
>      if (sn_info->id_str[0] != '\0' &&
>          strcmp(sn_info->id_str, sn_info->name) != 0) {
> +        error_setg(errp, "ID and name have to be equal.");
>          return -EINVAL;
>      }
>  
>      if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
> +        error_setg(errp, "Parameter 'name' has to be shorter that 127 chars.");

s/that/than/

>          return -ERANGE;
>      }
>  
>      r = rbd_snap_create(s->image, sn_info->name);
>      if (r < 0) {
> -        error_report("failed to create snap: %s", strerror(-r));
> +        error_setg(errp, "Failed to create snapshot: '%s'.", strerror(-r));

Use error_setg_errno(errp, -r, "failed to create snapshot").


> @@ -1779,9 +1781,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>              s->name, sn_info->vm_state_size, s->is_snapshot);
>  
>      if (s->is_snapshot) {
> -        error_report("You can't create a snapshot of a snapshot VDI, "
> -                     "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
> -
> +        error_setg(errp, "You can't create a snapshot '%s' of a VDI snapshot.",
> +                   sn_info->name);

Why are you losing information from the previous version of this error
message?


>      if (ret < 0) {
> -        error_report("failed to create inode for snapshot. %s",
> -                     strerror(errno));
> +        error_setg(errp, "Failed to create inode for snapshot.");

Another case of losing information; error_setg_errno would be better
here, and anywhere else the old message included a strerror() call.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 01/12] block: add error parameter to bdrv_snapshot_create() and related functions

[Pavel Hrdina]

On 26.3.2013 15:22, Eric Blake wrote:
> On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   block.c                   | 22 ++++++++++++++++------
>>   block/qcow2-snapshot.c    |  9 ++++++++-
>>   block/qcow2.h             |  4 +++-
>>   block/rbd.c               |  8 ++++++--
>>   block/sheepdog.c          | 17 +++++++++--------
>>   include/block/block.h     |  3 ++-
>>   include/block/block_int.h |  3 ++-
>>   qemu-img.c                |  2 +-
>>   savevm.c                  |  2 +-
>>   9 files changed, 48 insertions(+), 22 deletions(-)
>>
>
>>   int bdrv_snapshot_create(BlockDriverState *bs,
>> -                         QEMUSnapshotInfo *sn_info)
>> +                         QEMUSnapshotInfo *sn_info,
>> +                         Error **errp)
>>   {
>>       BlockDriver *drv = bs->drv;
>> -    if (!drv)
>> +
>> +    if (!drv) {
>> +        error_setg(errp, "Device '%s' has no medium.",
>
> In general, error_setg() should not print a trailing '.' (only two
> offenders to git grep 'error_setg.*\."').  I think we also tend to start
> messages with lower case, although that's not as obvious (49 cases of
> 'error_setg[^"*"[A-Z]' vs. 121 of error_setg[^"]*"[a-z]').

I don't know that there is any recommendation for that. I'll fix that in 
next series.

>
>> +
>> +    error_setg(errp, "Snapshot is not supported for '%s'.",
>
> And again, and probably throughout your series (although I'll quit
> pointing it out).
>
>> @@ -830,16 +832,18 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
>>        */
>>       if (sn_info->id_str[0] != '\0' &&
>>           strcmp(sn_info->id_str, sn_info->name) != 0) {
>> +        error_setg(errp, "ID and name have to be equal.");
>>           return -EINVAL;
>>       }
>>
>>       if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
>> +        error_setg(errp, "Parameter 'name' has to be shorter that 127 chars.");
>
> s/that/than/
>
>>           return -ERANGE;
>>       }
>>
>>       r = rbd_snap_create(s->image, sn_info->name);
>>       if (r < 0) {
>> -        error_report("failed to create snap: %s", strerror(-r));
>> +        error_setg(errp, "Failed to create snapshot: '%s'.", strerror(-r));
>
> Use error_setg_errno(errp, -r, "failed to create snapshot").
>
>
>> @@ -1779,9 +1781,8 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>               s->name, sn_info->vm_state_size, s->is_snapshot);
>>
>>       if (s->is_snapshot) {
>> -        error_report("You can't create a snapshot of a snapshot VDI, "
>> -                     "%s (%" PRIu32 ").", s->name, s->inode.vdi_id);
>> -
>> +        error_setg(errp, "You can't create a snapshot '%s' of a VDI snapshot.",
>> +                   sn_info->name);
>
> Why are you losing information from the previous version of this error
> message?
>
>
>>       if (ret < 0) {
>> -        error_report("failed to create inode for snapshot. %s",
>> -                     strerror(errno));
>> +        error_setg(errp, "Failed to create inode for snapshot.");
>
> Another case of losing information; error_setg_errno would be better
> here, and anywhere else the old message included a strerror() call.
>

Thanks for pointing this out. I've probably missed that during the 
rebase. I'll also check all other patches before I'll send another series.

Pavel

[Qemu-devel] [PATCH v2 02/12] block: add error parameter to del_existing_snapshots()

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 savevm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/savevm.c b/savevm.c
index fadde55..7b1867b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2110,7 +2110,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
 /*
  * Deletes snapshots of a given name in all opened images.
  */
-static int del_existing_snapshots(Monitor *mon, const char *name)
+static int del_existing_snapshots(const char *name, Error **errp)
 {
     BlockDriverState *bs;
     QEMUSnapshotInfo sn1, *snapshot = &sn1;
@@ -2123,9 +2123,8 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
         {
             ret = bdrv_snapshot_delete(bs, name);
             if (ret < 0) {
-                monitor_printf(mon,
-                               "Error while deleting snapshot on '%s'\n",
-                               bdrv_get_device_name(bs));
+                error_setg(errp, "Error while deleting snapshot on '%s'",
+                           bdrv_get_device_name(bs));
                 return -1;
             }
         }
@@ -2145,6 +2144,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     qemu_timeval tv;
     struct tm tm;
     const char *name = qdict_get_try_str(qdict, "name");
+    Error *local_err = NULL;
 
     /* Verify if there is a device that doesn't support snapshots and is writable */
     bs = NULL;
@@ -2193,7 +2193,9 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }
 
     /* Delete old snapshots of the same name */
-    if (name && del_existing_snapshots(mon, name) < 0) {
+    if (name && del_existing_snapshots(name, &local_err) < 0) {
+        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
+        error_free(local_err);
         goto the_end;
     }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 02/12] block: add error parameter to del_existing_snapshots()

[Eric Blake]

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



On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  savevm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

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: 621 bytes --]

[Qemu-devel] [PATCH v2 03/12] savevm: add error parameter to qemu_savevm_state_begin()

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 include/sysemu/sysemu.h | 3 ++-
 migration.c             | 2 +-
 savevm.c                | 6 ++++--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 6578782..2f35a05 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -74,7 +74,8 @@ void qemu_announce_self(void);
 
 bool qemu_savevm_state_blocked(Error **errp);
 void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params);
+                             const MigrationParams *params,
+                             Error **errp);
 int qemu_savevm_state_iterate(QEMUFile *f);
 void qemu_savevm_state_complete(QEMUFile *f);
 void qemu_savevm_state_cancel(void);
diff --git a/migration.c b/migration.c
index 185d112..66cba46 100644
--- a/migration.c
+++ b/migration.c
@@ -504,7 +504,7 @@ static void *migration_thread(void *opaque)
     bool old_vm_running = false;
 
     DPRINTF("beginning savevm\n");
-    qemu_savevm_state_begin(s->file, &s->params);
+    qemu_savevm_state_begin(s->file, &s->params, NULL);
 
     while (s->state == MIG_STATE_ACTIVE) {
         int64_t current_time;
diff --git a/savevm.c b/savevm.c
index 7b1867b..018e070 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1617,7 +1617,8 @@ bool qemu_savevm_state_blocked(Error **errp)
 }
 
 void qemu_savevm_state_begin(QEMUFile *f,
-                             const MigrationParams *params)
+                             const MigrationParams *params,
+                             Error **errp)
 {
     SaveStateEntry *se;
     int ret;
@@ -1807,9 +1808,10 @@ static int qemu_savevm_state(QEMUFile *f)
     }
 
     qemu_mutex_unlock_iothread();
-    qemu_savevm_state_begin(f, &params);
+    qemu_savevm_state_begin(f, &params, NULL);
     qemu_mutex_lock_iothread();
 
+
     while (qemu_file_get_error(f) == 0) {
         if (qemu_savevm_state_iterate(f) > 0) {
             break;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 03/12] savevm: add error parameter to qemu_savevm_state_begin()

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  include/sysemu/sysemu.h | 3 ++-
>  migration.c             | 2 +-
>  savevm.c                | 6 ++++--
>  3 files changed, 7 insertions(+), 4 deletions(-)

> @@ -1807,9 +1808,10 @@ static int qemu_savevm_state(QEMUFile *f)
>      }
>  
>      qemu_mutex_unlock_iothread();
> -    qemu_savevm_state_begin(f, &params);
> +    qemu_savevm_state_begin(f, &params, NULL);
>      qemu_mutex_lock_iothread();
>  
> +
>      while (qemu_file_get_error(f) == 0) {

Spurious line addition.  Trivial enough that you can keep:

Reviewed-by: Eric Blake <eblake@redhat.com>

if you fix it on the respin.

-- 
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: 621 bytes --]

[Qemu-devel] [PATCH v2 04/12] savevm: add error parameter to qemu_savevm_state_iterate()

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 include/sysemu/sysemu.h | 2 +-
 migration.c             | 2 +-
 savevm.c                | 5 ++---
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 2f35a05..af4f699 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -76,7 +76,7 @@ bool qemu_savevm_state_blocked(Error **errp);
 void qemu_savevm_state_begin(QEMUFile *f,
                              const MigrationParams *params,
                              Error **errp);
-int qemu_savevm_state_iterate(QEMUFile *f);
+int qemu_savevm_state_iterate(QEMUFile *f, Error **errp);
 void qemu_savevm_state_complete(QEMUFile *f);
 void qemu_savevm_state_cancel(void);
 uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size);
diff --git a/migration.c b/migration.c
index 66cba46..c2bf531 100644
--- a/migration.c
+++ b/migration.c
@@ -515,7 +515,7 @@ static void *migration_thread(void *opaque)
             pending_size = qemu_savevm_state_pending(s->file, max_size);
             DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
             if (pending_size && pending_size >= max_size) {
-                qemu_savevm_state_iterate(s->file);
+                qemu_savevm_state_iterate(s->file, NULL);
             } else {
                 DPRINTF("done iterating\n");
                 qemu_mutex_lock_iothread();
diff --git a/savevm.c b/savevm.c
index 018e070..c5ac701 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1670,7 +1670,7 @@ void qemu_savevm_state_begin(QEMUFile *f,
  *   0 : We haven't finished, caller have to go again
  *   1 : We have finished, we can go to complete phase
  */
-int qemu_savevm_state_iterate(QEMUFile *f)
+int qemu_savevm_state_iterate(QEMUFile *f, Error **errp)
 {
     SaveStateEntry *se;
     int ret = 1;
@@ -1811,9 +1811,8 @@ static int qemu_savevm_state(QEMUFile *f)
     qemu_savevm_state_begin(f, &params, NULL);
     qemu_mutex_lock_iothread();
 
-
     while (qemu_file_get_error(f) == 0) {
-        if (qemu_savevm_state_iterate(f) > 0) {
+        if (qemu_savevm_state_iterate(f, NULL) > 0) {
             break;
         }
     }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 04/12] savevm: add error parameter to qemu_savevm_state_iterate()

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  include/sysemu/sysemu.h | 2 +-
>  migration.c             | 2 +-
>  savevm.c                | 5 ++---
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>


> @@ -1811,9 +1811,8 @@ static int qemu_savevm_state(QEMUFile *f)
>      qemu_savevm_state_begin(f, &params, NULL);
>      qemu_mutex_lock_iothread();
>  
> -
>      while (qemu_file_get_error(f) == 0) {

Spurious whitespace change will go away when you fix 3/12 to not
introduce it in the first place.

> -        if (qemu_savevm_state_iterate(f) > 0) {
> +        if (qemu_savevm_state_iterate(f, NULL) > 0) {
>              break;
>          }
>      }
> 

-- 
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: 621 bytes --]

[Qemu-devel] [PATCH v2 05/12] savevm: add error parameter to qemu_savevm_state_complete()

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 include/sysemu/sysemu.h | 2 +-
 migration.c             | 2 +-
 savevm.c                | 5 +++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index af4f699..9e6a4a9 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -77,7 +77,7 @@ void qemu_savevm_state_begin(QEMUFile *f,
                              const MigrationParams *params,
                              Error **errp);
 int qemu_savevm_state_iterate(QEMUFile *f, Error **errp);
-void qemu_savevm_state_complete(QEMUFile *f);
+void qemu_savevm_state_complete(QEMUFile *f, Error **errp);
 void qemu_savevm_state_cancel(void);
 uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size);
 int qemu_loadvm_state(QEMUFile *f);
diff --git a/migration.c b/migration.c
index c2bf531..c8fdfc0 100644
--- a/migration.c
+++ b/migration.c
@@ -524,7 +524,7 @@ static void *migration_thread(void *opaque)
                 old_vm_running = runstate_is_running();
                 vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
                 qemu_file_set_rate_limit(s->file, INT_MAX);
-                qemu_savevm_state_complete(s->file);
+                qemu_savevm_state_complete(s->file, NULL);
                 qemu_mutex_unlock_iothread();
                 if (!qemu_file_get_error(s->file)) {
                     migrate_finish_set_state(s, MIG_STATE_COMPLETED);
diff --git a/savevm.c b/savevm.c
index c5ac701..76af390 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1709,7 +1709,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, Error **errp)
     return ret;
 }
 
-void qemu_savevm_state_complete(QEMUFile *f)
+void qemu_savevm_state_complete(QEMUFile *f, Error **errp)
 {
     SaveStateEntry *se;
     int ret;
@@ -1734,6 +1734,7 @@ void qemu_savevm_state_complete(QEMUFile *f)
         trace_savevm_section_end(se->section_id);
         if (ret < 0) {
             qemu_file_set_error(f, ret);
+            error_setg(errp, "Failed to complete vmstate save.");
             return;
         }
     }
@@ -1819,7 +1820,7 @@ static int qemu_savevm_state(QEMUFile *f)
 
     ret = qemu_file_get_error(f);
     if (ret == 0) {
-        qemu_savevm_state_complete(f);
+        qemu_savevm_state_complete(f, NULL);
         ret = qemu_file_get_error(f);
     }
     if (ret != 0) {
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 05/12] savevm: add error parameter to qemu_savevm_state_complete()

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  include/sysemu/sysemu.h | 2 +-
>  migration.c             | 2 +-
>  savevm.c                | 5 +++--
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 

> @@ -1734,6 +1734,7 @@ void qemu_savevm_state_complete(QEMUFile *f)
>          trace_savevm_section_end(se->section_id);
>          if (ret < 0) {
>              qemu_file_set_error(f, ret);
> +            error_setg(errp, "Failed to complete vmstate save.");

I know I said I wouldn't keep mentioning error message conventions...

therefore, I'm okay if you fix things, and add:

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: 621 bytes --]

[Qemu-devel] [PATCH v2 06/12] savevm: add error parameter to qemu_savevm_state()

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 savevm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/savevm.c b/savevm.c
index 76af390..71981d1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1796,7 +1796,7 @@ void qemu_savevm_state_cancel(void)
     }
 }
 
-static int qemu_savevm_state(QEMUFile *f)
+static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
     MigrationParams params = {
@@ -1804,23 +1804,23 @@ static int qemu_savevm_state(QEMUFile *f)
         .shared = 0
     };
 
-    if (qemu_savevm_state_blocked(NULL)) {
+    if (qemu_savevm_state_blocked(errp)) {
         return -EINVAL;
     }
 
     qemu_mutex_unlock_iothread();
-    qemu_savevm_state_begin(f, &params, NULL);
+    qemu_savevm_state_begin(f, &params, errp);
     qemu_mutex_lock_iothread();
 
     while (qemu_file_get_error(f) == 0) {
-        if (qemu_savevm_state_iterate(f, NULL) > 0) {
+        if (qemu_savevm_state_iterate(f, errp) > 0) {
             break;
         }
     }
 
     ret = qemu_file_get_error(f);
     if (ret == 0) {
-        qemu_savevm_state_complete(f, NULL);
+        qemu_savevm_state_complete(f, errp);
         ret = qemu_file_get_error(f);
     }
     if (ret != 0) {
@@ -2207,7 +2207,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "Could not open VM state file\n");
         goto the_end;
     }
-    ret = qemu_savevm_state(f);
+    ret = qemu_savevm_state(f, NULL);
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
     if (ret < 0) {
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 06/12] savevm: add error parameter to qemu_savevm_state()

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  savevm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 

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: 621 bytes --]

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

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hmp-commands.hx         |  4 ++--
 hmp.c                   |  9 +++++++++
 hmp.h                   |  1 +
 include/sysemu/sysemu.h |  1 -
 qapi-schema.json        | 18 ++++++++++++++++++
 qmp-commands.hx         | 29 +++++++++++++++++++++++++++++
 savevm.c                | 27 ++++++++++++---------------
 7 files changed, 71 insertions(+), 18 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index df44906..3c1cb05 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -310,7 +310,7 @@ ETEXI
         .args_type  = "name:s?",
         .params     = "[tag|id]",
         .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
-        .mhandler.cmd = do_savevm,
+        .mhandler.cmd = hmp_vm_snapshot_save,
     },
 
 STEXI
@@ -318,7 +318,7 @@ STEXI
 @findex savevm
 Create a snapshot of the whole virtual machine. If @var{tag} is
 provided, it is used as human readable identifier. If there is already
-a snapshot with the same tag or ID, it is replaced. More info at
+a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at
 @ref{vm_snapshots}.
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index b0a861c..914d88c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1423,3 +1423,12 @@ void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
     qmp_chardev_remove(qdict_get_str(qdict, "id"), &local_err);
     hmp_handle_error(mon, &local_err);
 }
+
+void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
+{
+    const char *name = qdict_get_try_str(qdict, "name");
+    Error *err = NULL;
+
+    qmp_vm_snapshot_save(!!name, name, &err);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 95fe76e..d5c61fa 100644
--- a/hmp.h
+++ b/hmp.h
@@ -85,5 +85,6 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
 void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
 void hmp_chardev_add(Monitor *mon, const QDict *qdict);
 void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
+void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 9e6a4a9..87b82d7 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);
 
 void qemu_add_machine_init_done_notifier(Notifier *notify);
 
-void do_savevm(Monitor *mon, const QDict *qdict);
 int load_vmstate(const char *name);
 void do_delvm(Monitor *mon, const QDict *qdict);
 void do_info_snapshots(Monitor *mon, const QDict *qdict);
diff --git a/qapi-schema.json b/qapi-schema.json
index fdaa9da..87842d3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3442,3 +3442,21 @@
 # Since: 1.5
 ##
 { 'command': 'query-tpm', 'returns': ['TPMInfo'] }
+
+##
+# @vm-snapshot-save:
+#
+# Create a snapshot of the whole virtual machine. If tag is provided as @name,
+# it is used as human readable identifier. If there is already a snapshot
+# with the same tag or ID, it is replaced.
+#
+# The VM is automatically stopped and resumed and saving a snapshot can take
+# a long time.
+#
+# @name: #optional tag of new snapshot or tag|id of existing snapshot
+#
+# Returns: Nothing on success
+#
+# Since: 1.5
+##
+{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b370060..0f5e544 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1423,6 +1423,35 @@ Example:
 
 EQMP
     {
+        .name       = "vm-snapshot-save",
+        .args_type  = "name:s?",
+        .params     = "name",
+        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+        .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save
+    },
+
+SQMP
+vm-snapshot-save
+------
+
+Create a snapshot of the whole virtual machine. If tag is provided as name,
+it is used as human readable identifier. If there is already a snapshot
+with the same tag or id, it is replaced.
+
+The VM is automatically stopped and resumed and saving a snapshot can take
+a long time.
+
+Arguments:
+
+- "name": #optional tag of new snapshot or tag|id of existing snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
+<- { "return": {} }
+
+EQMP
+    {
         .name       = "qmp_capabilities",
         .args_type  = "",
         .params     = "",
diff --git a/savevm.c b/savevm.c
index 71981d1..45d1b09 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2135,7 +2135,7 @@ static int del_existing_snapshots(const char *name, Error **errp)
     return 0;
 }
 
-void do_savevm(Monitor *mon, const QDict *qdict)
+void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2145,7 +2145,6 @@ void do_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;
 
     /* Verify if there is a device that doesn't support snapshots and is writable */
@@ -2157,15 +2156,15 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         }
 
         if (!bdrv_can_snapshot(bs)) {
-            monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
-                               bdrv_get_device_name(bs));
+            error_setg(errp, "Device '%s' is writable but does not support "
+                       "snapshots.", bdrv_get_device_name(bs));
             return;
         }
     }
 
     bs = bdrv_snapshots();
     if (!bs) {
-        monitor_printf(mon, "No block device can accept snapshots\n");
+        error_setg(errp, "No block device can accept snapshots.");
         return;
     }
 
@@ -2180,7 +2179,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     sn->date_nsec = tv.tv_usec * 1000;
     sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
 
-    if (name) {
+    if (has_name) {
         ret = bdrv_snapshot_find(bs, old_sn, name);
         if (ret >= 0) {
             pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
@@ -2195,23 +2194,22 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     }
 
     /* Delete old snapshots of the same name */
-    if (name && del_existing_snapshots(name, &local_err) < 0) {
-        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
-        error_free(local_err);
+    if (has_name && del_existing_snapshots(name, &local_err) < 0) {
+        error_propagate(errp, local_err);
         goto the_end;
     }
 
     /* 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, "Failed to open '%s' file.", bdrv_get_device_name(bs));
         goto the_end;
     }
-    ret = qemu_savevm_state(f, NULL);
+    ret = qemu_savevm_state(f, &local_err);
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
     if (ret < 0) {
-        monitor_printf(mon, "Error %d while writing VM\n", ret);
+        error_propagate(errp, local_err);
         goto the_end;
     }
 
@@ -2222,10 +2220,9 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         if (bdrv_can_snapshot(bs1)) {
             /* Write VM state size only to the image that contains the state */
             sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
-            ret = bdrv_snapshot_create(bs1, sn, NULL);
+            ret = bdrv_snapshot_create(bs1, sn, &local_err);
             if (ret < 0) {
-                monitor_printf(mon, "Error while creating snapshot on '%s'\n",
-                               bdrv_get_device_name(bs1));
+                error_propagate(errp, local_err);
             }
         }
     }
-- 
1.8.1.4

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

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hmp-commands.hx         |  4 ++--
>  hmp.c                   |  9 +++++++++
>  hmp.h                   |  1 +
>  include/sysemu/sysemu.h |  1 -
>  qapi-schema.json        | 18 ++++++++++++++++++
>  qmp-commands.hx         | 29 +++++++++++++++++++++++++++++
>  savevm.c                | 27 ++++++++++++---------------
>  7 files changed, 71 insertions(+), 18 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -3442,3 +3442,21 @@
>  # Since: 1.5
>  ##
>  { 'command': 'query-tpm', 'returns': ['TPMInfo'] }
> +
> +##
> +# @vm-snapshot-save:
> +#
> +# Create a snapshot of the whole virtual machine. If tag is provided as @name,
> +# it is used as human readable identifier. If there is already a snapshot
> +# with the same tag or ID, it is replaced.

Any reason we have to fix this up later in the series, instead of
getting the interface right to begin with (in regards to having an
optional force argument)?

> +#
> +# The VM is automatically stopped and resumed and saving a snapshot can take
> +# a long time.

Transactionally, are we exposing the right building blocks?  While the
existing HMP command pauses the domain up front, I think the QMP
interface should be job-oriented.  That is, it should be possible to
start a long-running job and have QMP return immediately, so that QMP
remains responsive, and lets us do a live migrate, live bandwidth
tuning, the ability to gracefully abort, and the ability to pause the
domain if live migrate isn't converging fast enough.  HMP would then
preserve its existing interface by calling multiple QMP commands,
instead of trying to make QMP an exact analogue to the sub-par HMP
interface.

Libvirt _really_ wants to be able to cancel an in-progress snapshot
creation action, but can't if all we expose is the same interface as HMP
has always had.

> +# @name: #optional tag of new snapshot or tag|id of existing snapshot
> +#
> +# Returns: Nothing on success

Bad.  If @name is optional, and one is generated on your behalf, then
you need to return the 'name' that was generated.  Also, even if 'name'
is provided, knowing which 'id' was allocated is useful, since later
APIs can operate on 'name' or 'id'.

> +#
> +# Since: 1.5
> +##
> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }

In other words, this needs a return value.


>          if (!bdrv_can_snapshot(bs)) {
> -            monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
> -                               bdrv_get_device_name(bs));
> +            error_setg(errp, "Device '%s' is writable but does not support "
> +                       "snapshots.", bdrv_get_device_name(bs));
>              return;
>          }
>      }
>  
>      bs = bdrv_snapshots();
>      if (!bs) {
> -        monitor_printf(mon, "No block device can accept snapshots\n");
> +        error_setg(errp, "No block device can accept snapshots.");

Odd that we weren't consistent on whether errors ended with '.' when
using monitor_printf; your patch at least tried to be self-consistent,
even if opposite the normal usage of error_setg() :)

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


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

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

[Pavel Hrdina]

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

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

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

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

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

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

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

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

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

Pavel

[Qemu-devel] [PATCH v2 08/12] qemu-img: introduce qemu_img_handle_error

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 qemu-img.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 21d02bf..34badad 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -322,6 +322,14 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
     return 0;
 }
 
+static void qemu_img_handle_error(Error *err)
+{
+    if (error_is_set(&err)) {
+        error_report("%s", error_get_pretty(err));
+        error_free(err);
+    }
+}
+
 static int img_create(int argc, char **argv)
 {
     int c;
@@ -401,8 +409,7 @@ static int img_create(int argc, char **argv)
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
                     options, img_size, BDRV_O_FLAGS, &local_err, quiet);
     if (error_is_set(&local_err)) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        qemu_img_handle_error(local_err);
         return 1;
     }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 08/12] qemu-img: introduce qemu_img_handle_error

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  qemu-img.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 21d02bf..34badad 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -322,6 +322,14 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
>      return 0;
>  }
>  
> +static void qemu_img_handle_error(Error *err)
> +{
> +    if (error_is_set(&err)) {

Here you say it is permissible to pass in an unset err,...

> +        error_report("%s", error_get_pretty(err));
> +        error_free(err);
> +    }
> +}
> +
>  static int img_create(int argc, char **argv)
>  {
>      int c;
> @@ -401,8 +409,7 @@ static int img_create(int argc, char **argv)
>      bdrv_img_create(filename, fmt, base_filename, base_fmt,
>                      options, img_size, BDRV_O_FLAGS, &local_err, quiet);
>      if (error_is_set(&local_err)) {
> -        error_report("%s", error_get_pretty(local_err));
> -        error_free(local_err);
> +        qemu_img_handle_error(local_err);

...which means you are duplicating the error_is_set() call here.

>          return 1;
>      }

Does it make sense to have qemu_img_handle_error() return a different
value according to whether an error was present, so that you could
instead write:

if (qemu_img_handle_error(local_err) < 0) {
    return 1;
}

Or, if all callers are already checking for an actual error before
calling qemu_img_handle_error (for other reasons, such as img_create's
reason of control flow), should you drop the redundant condition out of
the helper function?

-- 
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: 621 bytes --]

[Qemu-devel] [PATCH v2 09/12] block: update return value from bdrv_snapshot_create

[Pavel Hrdina]

If we provide error message, we should also provide a return code.
In some cases we could not care about any error message and the return
code is enough for as.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block.c                | 4 ++--
 block/qcow2-snapshot.c | 4 ++--
 block/rbd.c            | 8 ++++----
 block/sheepdog.c       | 4 ++--
 qemu-img.c             | 7 ++++---
 5 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index d1b9034..e97a68c 100644
--- a/block.c
+++ b/block.c
@@ -3210,7 +3210,7 @@ int bdrv_snapshot_create(BlockDriverState *bs,
     if (!drv) {
         error_setg(errp, "Device '%s' has no medium.",
                    bdrv_get_device_name(bs));
-        return -ENOMEDIUM;
+        return -1;
     }
     if (drv->bdrv_snapshot_create) {
         return drv->bdrv_snapshot_create(bs, sn_info, errp);
@@ -3221,7 +3221,7 @@ int bdrv_snapshot_create(BlockDriverState *bs,
 
     error_setg(errp, "Snapshot is not supported for '%s'.",
                bdrv_get_format_name(bs));
-    return -ENOTSUP;
+    return -1;
 }
 
 int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index d6be9a6..ff2e774 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -337,7 +337,7 @@ int qcow2_snapshot_create(BlockDriverState *bs,
     /* Check that the ID is unique */
     if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
         error_setg(errp, "Parameter 'name' has to be unique ID.");
-        return -EEXIST;
+        return -1;
     }
 
     /* Populate sn with passed data */
@@ -420,7 +420,7 @@ fail:
     g_free(sn->name);
     g_free(l1_table);
 
-    return ret;
+    return -1;
 }
 
 /* copy the snapshot 'snapshot_name' into the current disk image */
diff --git a/block/rbd.c b/block/rbd.c
index 914f9fa..9b5dfb4 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -823,7 +823,7 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
 
     if (sn_info->name[0] == '\0') {
         error_setg(errp, "Parameter 'name' cannot be empty.");
-        return -EINVAL; /* we need a name for rbd snapshots */
+        return -1; /* we need a name for rbd snapshots */
     }
 
     /*
@@ -833,18 +833,18 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
     if (sn_info->id_str[0] != '\0' &&
         strcmp(sn_info->id_str, sn_info->name) != 0) {
         error_setg(errp, "ID and name have to be equal.");
-        return -EINVAL;
+        return -1;
     }
 
     if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
         error_setg(errp, "Parameter 'name' has to be shorter that 127 chars.");
-        return -ERANGE;
+        return -1;
     }
 
     r = rbd_snap_create(s->image, sn_info->name);
     if (r < 0) {
         error_setg(errp, "Failed to create snapshot: '%s'.", strerror(-r));
-        return r;
+        return -1;
     }
 
     return 0;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index f4ee80e..a55856e 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1783,7 +1783,7 @@ static int sd_snapshot_create(BlockDriverState *bs,
     if (s->is_snapshot) {
         error_setg(errp, "You can't create a snapshot '%s' of a VDI snapshot.",
                    sn_info->name);
-        return -EINVAL;
+        return -1;
     }
 
     dprintf("%s %s\n", sn_info->name, sn_info->id_str);
@@ -1835,7 +1835,7 @@ static int sd_snapshot_create(BlockDriverState *bs,
 
 cleanup:
     closesocket(fd);
-    return ret;
+    return ret < 0 ? -1 : 0;
 }
 
 static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
diff --git a/qemu-img.c b/qemu-img.c
index 34badad..5090a53 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1943,6 +1943,7 @@ static int img_snapshot(int argc, char **argv)
     int action = 0;
     qemu_timeval tv;
     bool quiet = false;
+    Error *local_err;
 
     bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
     /* Parse commandline parameters */
@@ -2019,10 +2020,10 @@ static int img_snapshot(int argc, char **argv)
         sn.date_sec = tv.tv_sec;
         sn.date_nsec = tv.tv_usec * 1000;
 
-        ret = bdrv_snapshot_create(bs, &sn, NULL);
+        local_err = NULL;
+        ret = bdrv_snapshot_create(bs, &sn, &local_err);
         if (ret) {
-            error_report("Could not create snapshot '%s': %d (%s)",
-                snapshot_name, ret, strerror(-ret));
+            qemu_img_handle_error(local_err);
         }
         break;
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 09/12] block: update return value from bdrv_snapshot_create

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> If we provide error message, we should also provide a return code.
> In some cases we could not care about any error message and the return
> code is enough for as.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---

> +++ b/qemu-img.c
> @@ -1943,6 +1943,7 @@ static int img_snapshot(int argc, char **argv)
>      int action = 0;
>      qemu_timeval tv;
>      bool quiet = false;
> +    Error *local_err;
>  
>      bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
>      /* Parse commandline parameters */
> @@ -2019,10 +2020,10 @@ static int img_snapshot(int argc, char **argv)
>          sn.date_sec = tv.tv_sec;
>          sn.date_nsec = tv.tv_usec * 1000;
>  
> -        ret = bdrv_snapshot_create(bs, &sn, NULL);
> +        local_err = NULL;
> +        ret = bdrv_snapshot_create(bs, &sn, &local_err);
>          if (ret) {

Isn't it true that 'ret' is non-zero iff 'local_err' is no longer NULL?
 In which case, why not have the functions return 'void' instead of an
integer, and use the null-ness of local_err be your test of whether
bdrv_snapshot_create failed.

> -            error_report("Could not create snapshot '%s': %d (%s)",
> -                snapshot_name, ret, strerror(-ret));
> +            qemu_img_handle_error(local_err);

Same question as in 8/12 - you have already guaranteed that
qemu_img_handle_error will only be called if local_err is nonnull, so
does that helper function have to do a sanity check?

-- 
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: 621 bytes --]

[Qemu-devel] [PATCH v2 10/12] savevm: update return value from qemu_savevm_state

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 savevm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/savevm.c b/savevm.c
index 45d1b09..2e5f029 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1805,7 +1805,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
     };
 
     if (qemu_savevm_state_blocked(errp)) {
-        return -EINVAL;
+        return -1;
     }
 
     qemu_mutex_unlock_iothread();
@@ -1826,7 +1826,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
     if (ret != 0) {
         qemu_savevm_state_cancel();
     }
-    return ret;
+    return ret == 0 ? 0 : -1;
 }
 
 static int qemu_save_device_state(QEMUFile *f)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 10/12] savevm: update return value from qemu_savevm_state

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---

The commit message could usefully explain _why_ it is okay to collapse
all error values into one.  As written, the code appears accurate; and
the lone caller (do_savevm) handles the new semantics.  But this goes
back to my question in 9/10 - why not use errp as the witness, and
return void, instead of duplicating information?

>  savevm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 45d1b09..2e5f029 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1805,7 +1805,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>      };
>  
>      if (qemu_savevm_state_blocked(errp)) {
> -        return -EINVAL;
> +        return -1;
>      }
>  
>      qemu_mutex_unlock_iothread();
> @@ -1826,7 +1826,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>      if (ret != 0) {
>          qemu_savevm_state_cancel();
>      }
> -    return ret;
> +    return ret == 0 ? 0 : -1;
>  }
>  
>  static int qemu_save_device_state(QEMUFile *f)
> 

-- 
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: 621 bytes --]

[Qemu-devel] [PATCH v2 11/12] vm-snapshot-save: add force parameter

[Pavel Hrdina]

HMP command "savevm" now takes extra optional force parameter to specify whether
replace existing snapshot or not.

QMP command "vm-snapshot-save" has also extra optional force parameter and
name parameter isn't optional anymore.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hmp-commands.hx  | 15 ++++++++-------
 hmp.c            |  3 ++-
 qapi-schema.json |  7 +++++--
 qmp-commands.hx  | 16 ++++++++++------
 savevm.c         | 25 ++++++++++++++++---------
 5 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 3c1cb05..bfe0645 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -307,18 +307,19 @@ ETEXI
 
     {
         .name       = "savevm",
-        .args_type  = "name:s?",
-        .params     = "[tag|id]",
-        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+        .args_type  = "force:-f,name:s?",
+        .params     = "[-f] [tag|id]",
+        .help       = "save a VM snapshot. To replace existing snapshot use force flag.",
         .mhandler.cmd = hmp_vm_snapshot_save,
     },
 
 STEXI
-@item savevm [@var{tag}|@var{id}]
+@item savevm [@var{-f}] @var{tag}|@var{id}
 @findex savevm
-Create a snapshot of the whole virtual machine. If @var{tag} is
-provided, it is used as human readable identifier. If there is already
-a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at
+Create a snapshot of the whole virtual machine. Parameter "name" is optional.
+If @var{tag} is provided, it is used as human readable identifier. If there is
+already a snapshot with the same @var{tag} or @var{id}, @var{-f} flag needs to
+be specified. More info at @ref{vm_snapshots}.
 @ref{vm_snapshots}.
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index 914d88c..ff20917 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1427,8 +1427,9 @@ void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
 void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
 {
     const char *name = qdict_get_try_str(qdict, "name");
+    bool force = qdict_get_try_bool(qdict, "force", 0);
     Error *err = NULL;
 
-    qmp_vm_snapshot_save(!!name, name, &err);
+    qmp_vm_snapshot_save(!!name, name, !!force, force, &err);
     hmp_handle_error(mon, &err);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 87842d3..9bceb8f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3448,15 +3448,18 @@
 #
 # Create a snapshot of the whole virtual machine. If tag is provided as @name,
 # it is used as human readable identifier. If there is already a snapshot
-# with the same tag or ID, it is replaced.
+# with the same tag or id, the force argument needs to be true to replace it.
 #
 # The VM is automatically stopped and resumed and saving a snapshot can take
 # a long time.
 #
 # @name: #optional tag of new snapshot or tag|id of existing snapshot
 #
+# @force: #optional specify whether existing snapshot is replaced or not,
+#         default is false
+#
 # Returns: Nothing on success
 #
 # Since: 1.5
 ##
-{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
+{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str', '*force': 'bool'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0f5e544..7e02131 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1424,9 +1424,9 @@ Example:
 EQMP
     {
         .name       = "vm-snapshot-save",
-        .args_type  = "name:s?",
-        .params     = "name",
-        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+        .args_type  = "name:s?,force:b?",
+        .params     = "[name] [force]",
+        .help       = "save a VM snapshot. To replace existing snapshot use force parameter.",
         .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save
     },
 
@@ -1435,15 +1435,19 @@ vm-snapshot-save
 ------
 
 Create a snapshot of the whole virtual machine. If tag is provided as name,
-it is used as human readable identifier. If there is already a snapshot
-with the same tag or id, it is replaced.
+it is used as human readable identifier. If there is already a snapshot with
+the same tag, the force argument needs to be true to replace it.
 
 The VM is automatically stopped and resumed and saving a snapshot can take
 a long time.
 
 Arguments:
 
-- "name": #optional tag of new snapshot or tag|id of existing snapshot (json-string, optional)
+- "name": tag of new snapshot or tag|id of existing snapshot
+          (json-string, optional)
+
+- "force": specify whether existing snapshot is replaced or not,
+           default is false (json-bool, optional)
 
 Example:
 
diff --git a/savevm.c b/savevm.c
index 2e5f029..3e3d5f8 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2135,7 +2135,8 @@ static int del_existing_snapshots(const char *name, Error **errp)
     return 0;
 }
 
-void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
+void qmp_vm_snapshot_save(bool has_name, const char *name, bool has_force,
+                          bool force, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2182,8 +2183,20 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
     if (has_name) {
         ret = bdrv_snapshot_find(bs, old_sn, name);
         if (ret >= 0) {
-            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
-            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
+            if (has_force && force) {
+                pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
+                pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
+
+                /* Delete old snapshots of the same name */
+                if (del_existing_snapshots(name, &local_err) < 0) {
+                    error_propagate(errp, local_err);
+                    goto the_end;
+                }
+            } else {
+                error_setg(errp, "Snapshot '%s' exists. For override use "
+                           "'force' parameter.", name);
+                goto the_end;
+            }
         } else {
             pstrcpy(sn->name, sizeof(sn->name), name);
         }
@@ -2193,12 +2206,6 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }
 
-    /* Delete old snapshots of the same name */
-    if (has_name && del_existing_snapshots(name, &local_err) < 0) {
-        error_propagate(errp, local_err);
-        goto the_end;
-    }
-
     /* save the VM state */
     f = qemu_fopen_bdrv(bs, 1);
     if (!f) {
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 11/12] vm-snapshot-save: add force parameter

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> HMP command "savevm" now takes extra optional force parameter to specify whether
> replace existing snapshot or not.
> 
> QMP command "vm-snapshot-save" has also extra optional force parameter and
> name parameter isn't optional anymore.

Again, my question from 7/12 - why not rebase this series to implement
QMP correctly from the getgo, and have this patch be JUST an HMP
addition to expose the new QMP capability?

> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hmp-commands.hx  | 15 ++++++++-------
>  hmp.c            |  3 ++-
>  qapi-schema.json |  7 +++++--
>  qmp-commands.hx  | 16 ++++++++++------
>  savevm.c         | 25 ++++++++++++++++---------
>  5 files changed, 41 insertions(+), 25 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 3c1cb05..bfe0645 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -307,18 +307,19 @@ ETEXI
>  
>      {
>          .name       = "savevm",
> -        .args_type  = "name:s?",
> -        .params     = "[tag|id]",
> -        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
> +        .args_type  = "force:-f,name:s?",
> +        .params     = "[-f] [tag|id]",
> +        .help       = "save a VM snapshot. To replace existing snapshot use force flag.",

Not an error message, so I'm not duplicating myself :)
Prevailing convention seems to be omitting trailing '.' in .help messages.

>          .mhandler.cmd = hmp_vm_snapshot_save,
>      },
>  
>  STEXI
> -@item savevm [@var{tag}|@var{id}]
> +@item savevm [@var{-f}] @var{tag}|@var{id}
>  @findex savevm
> -Create a snapshot of the whole virtual machine. If @var{tag} is
> -provided, it is used as human readable identifier. If there is already
> -a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at
> +Create a snapshot of the whole virtual machine. Parameter "name" is optional.
> +If @var{tag} is provided, it is used as human readable identifier. If there is
> +already a snapshot with the same @var{tag} or @var{id}, @var{-f} flag needs to
> +be specified. More info at @ref{vm_snapshots}.
>  @ref{vm_snapshots}.

@ref{vm_snapshots} now appears twice in the output.

-- 
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: 621 bytes --]

[Qemu-devel] [PATCH v2 12/12] savevm: icrease the IO_BUF_SIZE to improve the speed of savevm

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 savevm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/savevm.c b/savevm.c
index 3e3d5f8..532c1be 100644
--- a/savevm.c
+++ b/savevm.c
@@ -112,7 +112,7 @@ void qemu_announce_self(void)
 /***********************************************************/
 /* savevm/loadvm support */
 
-#define IO_BUF_SIZE 32768
+#define IO_BUF_SIZE 1 * 1024 * 1024
 
 struct QEMUFile {
     const QEMUFileOps *ops;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2 12/12] savevm: icrease the IO_BUF_SIZE to improve the speed of savevm

[Eric Blake]

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

On 03/22/2013 07:16 AM, Pavel Hrdina wrote:

s/icrease/increase/ in the subject

Some benchmark numbers in the commit message justifying your change
would be nice.

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  savevm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 3e3d5f8..532c1be 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -112,7 +112,7 @@ void qemu_announce_self(void)
>  /***********************************************************/
>  /* savevm/loadvm support */
>  
> -#define IO_BUF_SIZE 32768
> +#define IO_BUF_SIZE 1 * 1024 * 1024

Improperly parenthesized; needs to be:

#define IO_BUF_SIZE (1 * 1024 * 1024)

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH v2 12/12] savevm: icrease the IO_BUF_SIZE to improve the speed of savevm

[Pavel Hrdina]

On 26.3.2013 23:05, Eric Blake wrote:
> On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
>
> s/icrease/increase/ in the subject
>
> Some benchmark numbers in the commit message justifying your change
> would be nice.
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   savevm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 3e3d5f8..532c1be 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -112,7 +112,7 @@ void qemu_announce_self(void)
>>   /***********************************************************/
>>   /* savevm/loadvm support */
>>
>> -#define IO_BUF_SIZE 32768
>> +#define IO_BUF_SIZE 1 * 1024 * 1024
>
> Improperly parenthesized; needs to be:
>
> #define IO_BUF_SIZE (1 * 1024 * 1024)
>

I'm testing it now and it seems that there is no speed improvement.
I'll drop this patch from the series.

Both values makes first snapshot (means that there is no snapshot at 
all) in about 30s. Second snapshots takes about 320s and third snapshot 
after restarting qemu process takes the same as second snapshot.

Test enviroment is:
     guest:
         2G ram
         2 vcpus
         qcow2 disk image

     host:
         12G ram
         1cpu/4 core/8 threads
         ordinary 7200rpm/500G hdd