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

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

Last patch introduce new functionality of savevm and vm-snapshot-save
that you cannot override existing snapshot without using 'force' parameter
and for QMP you have to always provide name parameter.

Pavel Hrdina (13):
  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_begin
  savevm: update return value from qemu_savevm_state_complete
  savevm: update return value from qemu_savevm_state
  vm-snapshot-save: add force parameter

 block.c                   |  26 ++++++----
 block/qcow2-snapshot.c    |  14 ++++--
 block/qcow2.h             |   4 +-
 block/rbd.c               |  15 ++++--
 block/sheepdog.c          |  21 ++++----
 hmp-commands.hx           |  17 +++----
 hmp.c                     |  32 +++++++++++++
 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           |  32 +++++++++++++
 savevm.c                  | 120 +++++++++++++++++++++++++---------------------
 16 files changed, 237 insertions(+), 104 deletions(-)

-- 
1.8.1

[Qemu-devel] [PATCH 01/13] 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    | 10 +++++++++-
 block/qcow2.h             |  4 +++-
 block/rbd.c               |  7 ++++++-
 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, 49 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index 4e28c55..10e5d3e 100644
--- a/block.c
+++ b/block.c
@@ -3131,15 +3131,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 eb8fcd5..529e9ee 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -312,7 +312,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;
@@ -331,6 +333,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;
     }
 
@@ -348,6 +351,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;
     }
 
@@ -362,6 +366,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;
     }
 
@@ -375,11 +380,13 @@ 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;
     }
 
     ret = bdrv_flush(bs);
     if (ret < 0) {
+        error_setg(errp, "Failed to flush data.");
         goto fail;
     }
 
@@ -397,6 +404,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 718b52b..b618403 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -349,7 +349,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..1066159 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,15 +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_setg(errp, "Failed to create snapshot.");
         error_report("failed to create snap: %s", strerror(-r));
         return r;
     }
diff --git a/block/sheepdog.c b/block/sheepdog.c
index e821746..aacde64 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1736,7 +1736,9 @@ static int coroutine_fn sd_co_flush_to_disk(BlockDriverState *bs)
     return 0;
 }
 
-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;
@@ -1749,9 +1751,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;
     }
 
@@ -1770,21 +1771,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     fd = connect_to_sdog(s->addr, s->port);
     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_enabled);
     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->name, s->inode.vdi_size, s->inode.vdi_id, &new_vid, 1,
                        s->addr, s->port);
     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;
     }
 
@@ -1794,7 +1795,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
                       s->inode.nr_copies, datalen, 0, s->cache_enabled);
 
     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 0719339..bcadb72 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -321,7 +321,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 f83ffb8..93ef4198 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -147,7 +147,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 85d3740..5d667ae 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1564,7 +1564,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 529d60e..b1acbf1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2186,7 +2186,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

[Qemu-devel] [PATCH 02/13] 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 b1acbf1..fae0cba 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2061,7 +2061,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;
@@ -2074,9 +2074,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;
             }
         }
@@ -2101,6 +2100,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     struct tm tm;
 #endif
     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;
@@ -2161,7 +2161,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

[Qemu-devel] [PATCH 03/13] 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                | 7 +++++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 28a783e..e50f990 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);
 int qemu_savevm_state_begin(QEMUFile *f,
-                            const MigrationParams *params);
+                            const MigrationParams *params,
+                            Error **errp);
 int qemu_savevm_state_iterate(QEMUFile *f);
 int qemu_savevm_state_complete(QEMUFile *f);
 void qemu_savevm_state_cancel(QEMUFile *f);
diff --git a/migration.c b/migration.c
index c69e864..cb38867 100644
--- a/migration.c
+++ b/migration.c
@@ -678,7 +678,7 @@ static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
     if (s->first_time) {
         s->first_time = false;
         DPRINTF("beginning savevm\n");
-        ret = qemu_savevm_state_begin(s->file, &s->params);
+        ret = qemu_savevm_state_begin(s->file, &s->params, NULL);
         if (ret < 0) {
             DPRINTF("failed, %d\n", ret);
             migrate_fd_error(s);
diff --git a/savevm.c b/savevm.c
index fae0cba..e2ecb0b 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1557,7 +1557,8 @@ bool qemu_savevm_state_blocked(Error **errp)
 }
 
 int qemu_savevm_state_begin(QEMUFile *f,
-                            const MigrationParams *params)
+                            const MigrationParams *params,
+                            Error **errp)
 {
     SaveStateEntry *se;
     int ret;
@@ -1598,11 +1599,13 @@ int qemu_savevm_state_begin(QEMUFile *f,
         ret = se->ops->save_live_setup(f, se->opaque);
         if (ret < 0) {
             qemu_savevm_state_cancel(f);
+            error_setg(errp, "Failed to begin vmstate save.");
             return ret;
         }
     }
     ret = qemu_file_get_error(f);
     if (ret != 0) {
+        error_setg_errno(errp, errno, "Failed to begin vmstate save.");
         qemu_savevm_state_cancel(f);
     }
 
@@ -1758,7 +1761,7 @@ static int qemu_savevm_state(QEMUFile *f)
         goto out;
     }
 
-    ret = qemu_savevm_state_begin(f, &params);
+    ret = qemu_savevm_state_begin(f, &params, NULL);
     if (ret < 0)
         goto out;
 
-- 
1.8.1

[Qemu-devel] [PATCH 04/13] 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                | 19 ++++++++++++-------
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index e50f990..afc1fe6 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -76,7 +76,7 @@ bool qemu_savevm_state_blocked(Error **errp);
 int 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);
 int qemu_savevm_state_complete(QEMUFile *f);
 void qemu_savevm_state_cancel(QEMUFile *f);
 uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size);
diff --git a/migration.c b/migration.c
index cb38867..0f2d0fd 100644
--- a/migration.c
+++ b/migration.c
@@ -691,7 +691,7 @@ static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
     pending_size = qemu_savevm_state_pending(s->file, max_size);
     DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
     if (pending_size >= max_size) {
-        ret = qemu_savevm_state_iterate(s->file);
+        ret = qemu_savevm_state_iterate(s->file, NULL);
         if (ret < 0) {
             migrate_fd_error(s);
         }
diff --git a/savevm.c b/savevm.c
index e2ecb0b..4da2136 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1619,7 +1619,7 @@ int 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;
@@ -1652,13 +1652,18 @@ int qemu_savevm_state_iterate(QEMUFile *f)
             break;
         }
     }
-    if (ret != 0) {
-        return ret;
+
+    if (ret == 0) {
+        ret = qemu_file_get_error(f);
+        if (ret != 0) {
+            qemu_savevm_state_cancel(f);
+        }
     }
-    ret = qemu_file_get_error(f);
-    if (ret != 0) {
-        qemu_savevm_state_cancel(f);
+
+    if (ret < 0) {
+        error_setg_errno(errp, errno, "Failed to iterate vmstate save.");
     }
+
     return ret;
 }
 
@@ -1766,7 +1771,7 @@ static int qemu_savevm_state(QEMUFile *f)
         goto out;
 
     do {
-        ret = qemu_savevm_state_iterate(f);
+        ret = qemu_savevm_state_iterate(f, NULL);
         if (ret < 0)
             goto out;
     } while (ret == 0);
-- 
1.8.1

[Qemu-devel] [PATCH 05/13] 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                | 12 +++++++++---
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index afc1fe6..d02d13f 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -77,7 +77,7 @@ int qemu_savevm_state_begin(QEMUFile *f,
                             const MigrationParams *params,
                             Error **errp);
 int qemu_savevm_state_iterate(QEMUFile *f, Error **errp);
-int qemu_savevm_state_complete(QEMUFile *f);
+int qemu_savevm_state_complete(QEMUFile *f, Error **errp);
 void qemu_savevm_state_cancel(QEMUFile *f);
 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 0f2d0fd..181d4a4 100644
--- a/migration.c
+++ b/migration.c
@@ -708,7 +708,7 @@ static bool migrate_fd_put_ready(MigrationState *s, uint64_t max_size)
             vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
         }
 
-        if (qemu_savevm_state_complete(s->file) < 0) {
+        if (qemu_savevm_state_complete(s->file, NULL) < 0) {
             migrate_fd_error(s);
         } else {
             migrate_fd_completed(s);
diff --git a/savevm.c b/savevm.c
index 4da2136..14f61e9 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1667,7 +1667,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, Error **errp)
     return ret;
 }
 
-int qemu_savevm_state_complete(QEMUFile *f)
+int qemu_savevm_state_complete(QEMUFile *f, Error **errp)
 {
     SaveStateEntry *se;
     int ret;
@@ -1691,6 +1691,7 @@ int qemu_savevm_state_complete(QEMUFile *f)
         ret = se->ops->save_live_complete(f, se->opaque);
         trace_savevm_section_end(se->section_id);
         if (ret < 0) {
+            error_setg(errp, "Failed to complete vmstate save.");
             return ret;
         }
     }
@@ -1720,7 +1721,12 @@ int qemu_savevm_state_complete(QEMUFile *f)
 
     qemu_put_byte(f, QEMU_VM_EOF);
 
-    return qemu_file_get_error(f);
+    ret = qemu_file_get_error(f);
+    if (ret < 0) {
+        error_setg_errno(errp, errno, "Failed to complete vmstate save.");
+    }
+
+    return ret;
 }
 
 uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size)
@@ -1776,7 +1782,7 @@ static int qemu_savevm_state(QEMUFile *f)
             goto out;
     } while (ret == 0);
 
-    ret = qemu_savevm_state_complete(f);
+    ret = qemu_savevm_state_complete(f, NULL);
 
 out:
     if (ret == 0) {
-- 
1.8.1

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

[Pavel Hrdina]

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

diff --git a/savevm.c b/savevm.c
index 14f61e9..b705693 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1759,7 +1759,7 @@ void qemu_savevm_state_cancel(QEMUFile *f)
     }
 }
 
-static int qemu_savevm_state(QEMUFile *f)
+static int qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
     MigrationParams params = {
@@ -1767,26 +1767,29 @@ static int qemu_savevm_state(QEMUFile *f)
         .shared = 0
     };
 
-    if (qemu_savevm_state_blocked(NULL)) {
+    if (qemu_savevm_state_blocked(errp)) {
         ret = -EINVAL;
         goto out;
     }
 
-    ret = qemu_savevm_state_begin(f, &params, NULL);
+    ret = qemu_savevm_state_begin(f, &params, errp);
     if (ret < 0)
         goto out;
 
     do {
-        ret = qemu_savevm_state_iterate(f, NULL);
+        ret = qemu_savevm_state_iterate(f, errp);
         if (ret < 0)
             goto out;
     } while (ret == 0);
 
-    ret = qemu_savevm_state_complete(f, NULL);
+    ret = qemu_savevm_state_complete(f, errp);
 
 out:
     if (ret == 0) {
         ret = qemu_file_get_error(f);
+        if (ret < 0) {
+            error_setg_errno(errp, errno, "Failed to create vmstate.");
+        }
     }
 
     return ret;
@@ -2187,7 +2190,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

[Qemu-devel] [PATCH 07/13] 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 010b8c9..3b781f7 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 9e9e624..2d8600a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1336,3 +1336,12 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
     qmp_nbd_server_stop(&errp);
     hmp_handle_error(mon, &errp);
 }
+
+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 21f3e05..e447f99 100644
--- a/hmp.h
+++ b/hmp.h
@@ -80,5 +80,6 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict);
 void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
 void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
 void hmp_nbd_server_stop(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 d02d13f..d4db699 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);
diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..a6f4cb0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3017,3 +3017,21 @@
 # Since: 1.3.0
 ##
 { 'command': 'nbd-server-stop' }
+
+##
+# @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.4.0
+##
+{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5c692d0..9a0b566 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1348,6 +1348,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 b705693..930ef65 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2101,7 +2101,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;
@@ -2116,7 +2116,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     struct timeval tv;
     struct tm tm;
 #endif
-    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 */
@@ -2128,15 +2127,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;
     }
 
@@ -2157,7 +2156,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
 #endif
     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);
@@ -2178,23 +2177,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;
     }
 
@@ -2205,10 +2203,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

[Qemu-devel] [PATCH 08/13] 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 5d667ae..c283bf6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -292,6 +292,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;
@@ -367,8 +375,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);
     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

[Qemu-devel] [PATCH 09/13] 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 10e5d3e..44f43bd 100644
--- a/block.c
+++ b/block.c
@@ -3139,7 +3139,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);
@@ -3150,7 +3150,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 529e9ee..313c7cd 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -334,7 +334,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 */
@@ -423,7 +423,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 1066159..03f04c4 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,19 +833,19 @@ 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.");
         error_report("failed to create snap: %s", strerror(-r));
-        return r;
+        return -1;
     }
 
     return 0;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index aacde64..5c6adf7 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1753,7 +1753,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);
@@ -1805,7 +1805,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 c283bf6..06dd484 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1498,6 +1498,7 @@ static int img_snapshot(int argc, char **argv)
     int c, ret = 0, bdrv_oflags;
     int action = 0;
     qemu_timeval tv;
+    Error *local_err;
 
     bdrv_oflags = BDRV_O_FLAGS | BDRV_O_RDWR;
     /* Parse commandline parameters */
@@ -1571,10 +1572,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

[Qemu-devel] [PATCH 10/13] savevm: update return value from qemu_savevm_state_begin

[Pavel Hrdina]

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

diff --git a/savevm.c b/savevm.c
index 930ef65..f204748 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1600,17 +1600,17 @@ int qemu_savevm_state_begin(QEMUFile *f,
         if (ret < 0) {
             qemu_savevm_state_cancel(f);
             error_setg(errp, "Failed to begin vmstate save.");
-            return ret;
+            return -1;
         }
     }
     ret = qemu_file_get_error(f);
     if (ret != 0) {
         error_setg_errno(errp, errno, "Failed to begin vmstate save.");
         qemu_savevm_state_cancel(f);
+        return -1;
     }
 
-    return ret;
-
+    return 0;
 }
 
 /*
-- 
1.8.1

[Qemu-devel] [PATCH 11/13] savevm: update return value from qemu_savevm_state_complete

[Pavel Hrdina]

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

diff --git a/savevm.c b/savevm.c
index f204748..f9858cc 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1692,7 +1692,7 @@ int qemu_savevm_state_complete(QEMUFile *f, Error **errp)
         trace_savevm_section_end(se->section_id);
         if (ret < 0) {
             error_setg(errp, "Failed to complete vmstate save.");
-            return ret;
+            return -1;
         }
     }
 
@@ -1724,9 +1724,10 @@ int qemu_savevm_state_complete(QEMUFile *f, Error **errp)
     ret = qemu_file_get_error(f);
     if (ret < 0) {
         error_setg_errno(errp, errno, "Failed to complete vmstate save.");
+        return -1;
     }
 
-    return ret;
+    return 0;
 }
 
 uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size)
-- 
1.8.1

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

[Pavel Hrdina]

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

diff --git a/savevm.c b/savevm.c
index f9858cc..a7254b2 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1768,8 +1768,8 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
         .shared = 0
     };
 
-    if (qemu_savevm_state_blocked(errp)) {
-        ret = -EINVAL;
+    ret = qemu_savevm_state_blocked(errp);
+    if (ret) {
         goto out;
     }
 
@@ -1793,7 +1793,7 @@ out:
         }
     }
 
-    return ret;
+    return ret == 0 ? 0 : -1;
 }
 
 static int qemu_save_device_state(QEMUFile *f)
@@ -2189,6 +2189,7 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
         error_setg(errp, "Failed to open '%s' file.", bdrv_get_device_name(bs));
         goto the_end;
     }
+
     ret = qemu_savevm_state(f, &local_err);
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
-- 
1.8.1

[Qemu-devel] [PATCH 13/13] vm-snapshot-save: add force parameter

[Pavel Hrdina]

HMP command "savevm" now takes extra optional force parameter to specifi 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            | 25 ++++++++++++++++++++++++-
 qapi-schema.json |  9 ++++++---
 qmp-commands.hx  | 15 +++++++++------
 savevm.c         | 36 ++++++++++++++----------------------
 5 files changed, 61 insertions(+), 39 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 3b781f7..d857682 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. Paramtere "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 2d8600a..02493ca 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1340,8 +1340,31 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
 void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
 {
     const char *name = qdict_get_try_str(qdict, "name");
+    char new_name[256];
+    bool force = qdict_get_try_bool(qdict, "force", 0);
     Error *err = NULL;
+#ifdef _WIN32
+    struct _timeb tb;
+    struct tm *ptm;
+#else
+    struct timeval tv;
+    struct tm tm;
+#endif
+
+    if (!name) {
+#ifdef _WIN32
+        time_t t = tb.time;
+        ptm = localtime(&t);
+        strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", ptm);
+#else
+        /* cast below needed for OpenBSD where tv_sec is still 'long' */
+        localtime_r((const time_t *)&tv.tv_sec, &tm);
+        strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", &tm);
+#endif
+    } else {
+        pstrcpy(new_name, sizeof(new_name), name);
+    }
 
-    qmp_vm_snapshot_save(!!name, name, &err);
+    qmp_vm_snapshot_save(new_name, !!force, force, &err);
     hmp_handle_error(mon, &err);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index a6f4cb0..7f1639c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3023,15 +3023,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
+# @name: 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.4.0
 ##
-{ '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 9a0b566..1bd16f1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1349,9 +1349,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
     },
 
@@ -1360,15 +1360,18 @@ 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)
+
+- "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 a7254b2..090ab1f 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2102,7 +2102,7 @@ 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(const char *name, bool has_force, bool force, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2112,10 +2112,8 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
     uint64_t vm_state_size;
 #ifdef _WIN32
     struct _timeb tb;
-    struct tm *ptm;
 #else
     struct timeval tv;
-    struct tm tm;
 #endif
     Error *local_err = NULL;
 
@@ -2157,30 +2155,24 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
 #endif
     sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
 
-    if (has_name) {
-        ret = bdrv_snapshot_find(bs, old_sn, name);
-        if (ret >= 0) {
+    ret = bdrv_snapshot_find(bs, old_sn, name);
+    if (ret >= 0) {
+        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 {
-            pstrcpy(sn->name, sizeof(sn->name), name);
+            error_setg(errp, "Snapshot '%s' exist. For override add "
+                       "'force' parameter.", name);
+            goto the_end;
         }
     } else {
-#ifdef _WIN32
-        time_t t = tb.time;
-        ptm = localtime(&t);
-        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", ptm);
-#else
-        /* cast below needed for OpenBSD where tv_sec is still 'long' */
-        localtime_r((const time_t *)&tv.tv_sec, &tm);
-        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
-#endif
-    }
-
-    /* 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;
+        pstrcpy(sn->name, sizeof(sn->name), name);
     }
 
     /* save the VM state */
-- 
1.8.1

Re: [Qemu-devel] [PATCH 00/13] convert savevm to use qapi and introduce qmp command

[Pavel Hrdina]

Please remove from CC the bad address with c0m.

On Wed, 2013-01-09 at 16:17 +0100, Pavel Hrdina wrote:
> 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.
> 
> Last patch introduce new functionality of savevm and vm-snapshot-save
> that you cannot override existing snapshot without using 'force' parameter
> and for QMP you have to always provide name parameter.
> 
> Pavel Hrdina (13):
>   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_begin
>   savevm: update return value from qemu_savevm_state_complete
>   savevm: update return value from qemu_savevm_state
>   vm-snapshot-save: add force parameter
> 
>  block.c                   |  26 ++++++----
>  block/qcow2-snapshot.c    |  14 ++++--
>  block/qcow2.h             |   4 +-
>  block/rbd.c               |  15 ++++--
>  block/sheepdog.c          |  21 ++++----
>  hmp-commands.hx           |  17 +++----
>  hmp.c                     |  32 +++++++++++++
>  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           |  32 +++++++++++++
>  savevm.c                  | 120 +++++++++++++++++++++++++---------------------
>  16 files changed, 237 insertions(+), 104 deletions(-)
> 

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

[Eric Blake]

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

On 01/09/2013 08:18 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(-)
> 

For patches 1-6, you may add Reviewed-by: Eric Blake <eblake@redhat.com>
However, for this patch, I think we may need to think more about the design.

> +++ b/qapi-schema.json
> @@ -3017,3 +3017,21 @@
>  # Since: 1.3.0
>  ##
>  { 'command': 'nbd-server-stop' }
> +
> +##
> +# @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.

Since my initial review on your earlier RFC, I have changed my mind a
bit and started thinking that merely exposing the existing HMP commands
as raw QMP commands is gross.  The HMP design is blocking - you can't
abort the snapshot, and since it is long-running, you can't do anything
else with the guest during that time, either.  Of course, the guest is
paused, but why does it have to be?  Couldn't we enhance the snapshot
code to use the same principles as live migration to take an internal
snapshot without pausing the guest up front?

This our chance to make life better by breaking the QMP interface into a
sequence of commands, and merely rewriting the existing HMP command call
the entire QMP sequence under the hood, rather than just exposing the
same clunky one-command design in QMP.  In other words, I think it would
be nicer to have a job-based snapshot, where one command starts the
snapshot, and then another command can query status to see if the
snapshot is still underway, an event gets fired when the snapshot
completes, and where yet another command can be used to cleanly abort a
long-running snapshot if it turns out the snapshot is no longer needed
because something else came up that must be dealt with now.

In other words, I think this series and Wenchao Xia's series[1] need to
coordinate on a common design.
[1] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg00645.html

> +#
> +# @name: #optional tag of new snapshot or tag|id of existing snapshot
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 1.4.0
> +##
> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }

But if we insist making the same clunky QMP interface, then this (plus
the followup improvements in 13/13 for adding a force option) seems like
a valid translation from HMP.

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

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

[Eric Blake]

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

On 01/09/2013 08:18 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  savevm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

> @@ -2189,6 +2189,7 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
>          error_setg(errp, "Failed to open '%s' file.", bdrv_get_device_name(bs));
>          goto the_end;
>      }
> +
>      ret = qemu_savevm_state(f, &local_err);

Spurious whitespace change?

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


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

Re: [Qemu-devel] [PATCH 13/13] vm-snapshot-save: add force parameter

[Eric Blake]

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

On 01/09/2013 08:18 AM, Pavel Hrdina wrote:
> HMP command "savevm" now takes extra optional force parameter to specifi whether

s/specifi/specify/

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

The qcow2 format lets 'name' be optional; why are we insisting at the
QMP layer that we cannot use this feature?  It may be a change worth
making, but you'll need more arguments in the commit message; and you'll
still need to make sure that qemu can handle an arbitrary qcow2 file
that has an existing snapshot created by older qemu when name was still
optional.

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

> +@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. Paramtere "name" is optional.

s/Paramtere/Parameter/

> +++ b/hmp.c
> @@ -1340,8 +1340,31 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
>  void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
>  {
>      const char *name = qdict_get_try_str(qdict, "name");
> +    char new_name[256];
> +    bool force = qdict_get_try_bool(qdict, "force", 0);
>      Error *err = NULL;
> +#ifdef _WIN32
> +    struct _timeb tb;
> +    struct tm *ptm;
> +#else
> +    struct timeval tv;
> +    struct tm tm;
> +#endif
> +
> +    if (!name) {
> +#ifdef _WIN32
> +        time_t t = tb.time;
> +        ptm = localtime(&t);
> +        strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", ptm);

It feels like lots of pending patches are trying to touch this same area
of code; make sure you coordinate the efforts (for example,
http://patchwork.ozlabs.org/patch/210250/ should go in first).

> +++ b/qapi-schema.json
> @@ -3023,15 +3023,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
> +# @name: tag of new snapshot or tag|id of existing snapshot

It feels like you are doing too much in this commit.  Either name should
never have been optional earlier in the series, or the change to make
name mandatory should be its own commit (since nothing in the one-line
summary mentions making name mandatory).

> +    if (ret >= 0) {
> +        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 {
> -            pstrcpy(sn->name, sizeof(sn->name), name);
> +            error_setg(errp, "Snapshot '%s' exist. For override add "

s/exist/exists/

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

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

[Wenchao Xia]

于 2013-1-10 6:32, Eric Blake 写道:
> On 01/09/2013 08:18 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(-)
>>
>
> For patches 1-6, you may add Reviewed-by: Eric Blake <eblake@redhat.com>
> However, for this patch, I think we may need to think more about the design.
>
>> +++ b/qapi-schema.json
>> @@ -3017,3 +3017,21 @@
>>   # Since: 1.3.0
>>   ##
>>   { 'command': 'nbd-server-stop' }
>> +
>> +##
>> +# @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.
>
> Since my initial review on your earlier RFC, I have changed my mind a
> bit and started thinking that merely exposing the existing HMP commands
> as raw QMP commands is gross.  The HMP design is blocking - you can't
> abort the snapshot, and since it is long-running, you can't do anything
> else with the guest during that time, either.  Of course, the guest is
> paused, but why does it have to be?  Couldn't we enhance the snapshot
> code to use the same principles as live migration to take an internal
> snapshot without pausing the guest up front?
>
> This our chance to make life better by breaking the QMP interface into a
> sequence of commands, and merely rewriting the existing HMP command call
> the entire QMP sequence under the hood, rather than just exposing the
> same clunky one-command design in QMP.  In other words, I think it would
> be nicer to have a job-based snapshot, where one command starts the
> snapshot, and then another command can query status to see if the
> snapshot is still underway, an event gets fired when the snapshot
> completes, and where yet another command can be used to cleanly abort a
> long-running snapshot if it turns out the snapshot is no longer needed
> because something else came up that must be dealt with now.
>
> In other words, I think this series and Wenchao Xia's series[1] need to
> coordinate on a common design.
> [1] https://lists.gnu.org/archive/html/qemu-devel/2013-01/msg00645.html
>
   I think the job based snapshot may comes later after vmstate live save
is implemented, and now this interface is acceptable but mark it in
name or comment that it is sync.

>> +#
>> +# @name: #optional tag of new snapshot or tag|id of existing snapshot
>> +#
>> +# Returns: Nothing on success
>> +#
>> +# Since: 1.4.0
>> +##
>> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
>
> But if we insist making the same clunky QMP interface, then this (plus
> the followup improvements in 13/13 for adding a force option) seems like
> a valid translation from HMP.
>


-- 
Best Regards

Wenchao Xia

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

[Pavel Hrdina]

On Wed, 2013-01-09 at 15:39 -0700, Eric Blake wrote:
> On 01/09/2013 08:18 AM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  savevm.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> 
> > @@ -2189,6 +2189,7 @@ void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
> >          error_setg(errp, "Failed to open '%s' file.", bdrv_get_device_name(bs));
> >          goto the_end;
> >      }
> > +
> >      ret = qemu_savevm_state(f, &local_err);
> 
> Spurious whitespace change?
> 

I'll fix that, thanks.

Re: [Qemu-devel] [PATCH 13/13] vm-snapshot-save: add force parameter

[Pavel Hrdina]

On Wed, 2013-01-09 at 15:47 -0700, Eric Blake wrote:
> On 01/09/2013 08:18 AM, Pavel Hrdina wrote:
> > HMP command "savevm" now takes extra optional force parameter to specifi whether
> 
> s/specifi/specify/
> 
> > replace existing snapshot or not.
> > 
> > QMP command "vm-snapshot-save" has also extra optional force parameter and
> > name parameter isn't optional anymore.
> 
> The qcow2 format lets 'name' be optional; why are we insisting at the
> QMP layer that we cannot use this feature?  It may be a change worth
> making, but you'll need more arguments in the commit message; and you'll
> still need to make sure that qemu can handle an arbitrary qcow2 file
> that has an existing snapshot created by older qemu when name was still
> optional.
> 

Based on your comment I think that we should create this behavior:

We will have for HMP and QMP two parameters id and name and you can
choose only one of them next to the force option.

If name is specified and there is a snapshot with specified name, the
snapshot will be overwritten.

If name is specified and there isn't a snapshot with specified name, a
new snapshot will be written.

If id is specified and there is a snapshot with specified id, the
snapshot will be overwritten.

If id is specified and there isn't a snapshot with specified id,
the command ends with error message that there isn't snapshot to
overwrite with specified id.

If id or name is not specified the new snapshot with generated name and
id will be created.

In all successful cases the information about snapshot will be returned.

> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  hmp-commands.hx  | 15 ++++++++-------
> >  hmp.c            | 25 ++++++++++++++++++++++++-
> >  qapi-schema.json |  9 ++++++---
> >  qmp-commands.hx  | 15 +++++++++------
> >  savevm.c         | 36 ++++++++++++++----------------------
> >  5 files changed, 61 insertions(+), 39 deletions(-)
> > 
> 
> > +@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. Paramtere "name" is optional.
> 
> s/Paramtere/Parameter/
> 
> > +++ b/hmp.c
> > @@ -1340,8 +1340,31 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
> >  void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
> >  {
> >      const char *name = qdict_get_try_str(qdict, "name");
> > +    char new_name[256];
> > +    bool force = qdict_get_try_bool(qdict, "force", 0);
> >      Error *err = NULL;
> > +#ifdef _WIN32
> > +    struct _timeb tb;
> > +    struct tm *ptm;
> > +#else
> > +    struct timeval tv;
> > +    struct tm tm;
> > +#endif
> > +
> > +    if (!name) {
> > +#ifdef _WIN32
> > +        time_t t = tb.time;
> > +        ptm = localtime(&t);
> > +        strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", ptm);
> 
> It feels like lots of pending patches are trying to touch this same area
> of code; make sure you coordinate the efforts (for example,
> http://patchwork.ozlabs.org/patch/210250/ should go in first).
> 
> > +++ b/qapi-schema.json
> > @@ -3023,15 +3023,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
> > +# @name: tag of new snapshot or tag|id of existing snapshot
> 
> It feels like you are doing too much in this commit.  Either name should
> never have been optional earlier in the series, or the change to make
> name mandatory should be its own commit (since nothing in the one-line
> summary mentions making name mandatory).
> 
> > +    if (ret >= 0) {
> > +        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 {
> > -            pstrcpy(sn->name, sizeof(sn->name), name);
> > +            error_setg(errp, "Snapshot '%s' exist. For override add "
> 
> s/exist/exists/
>