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

[Qemu-devel] [PATCH v4 00/11] 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 last patch introduce new functionality of savevm that you cannot override
existing snapshot without using 'force' parameter.

If non-blocking behaviour of this command is required and we cannot wait
until live snapshots will be finished, I could improve this basic command
to be non-blocking.

Changes from v3:
    - correct hopefully all error messages
    - proper commit message for 'qapi: Convert savevm'

Changes from v2:
    - correct error messages
    - introduce of 'force' option moved to qapi: Convert savevm
    - update of return value for used functions
    - drop of the speed improve because it isn't actually speed improve
    - vm-snapshot-save and savevm now returns snapshot information

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


Pavel Hrdina (11):
  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
  savevm: add force parameter to HMP command and return snapshot info

 block.c                   | 24 +++++++-----
 block/qcow2-snapshot.c    | 15 +++++---
 block/qcow2.h             |  4 +-
 block/rbd.c               | 19 +++++-----
 block/sheepdog.c          | 20 +++++-----
 hmp-commands.hx           | 18 ++++-----
 hmp.c                     | 27 ++++++++++++++
 hmp.h                     |  1 +
 include/block/block.h     |  5 ++-
 include/block/block_int.h |  5 ++-
 include/sysemu/sysemu.h   |  8 ++--
 migration.c               |  6 +--
 qapi-schema.json          | 22 +++++++++++
 qemu-img.c                | 27 ++++++++------
 qmp-commands.hx           | 43 ++++++++++++++++++++++
 savevm.c                  | 93 ++++++++++++++++++++++++++++-------------------
 16 files changed, 235 insertions(+), 102 deletions(-)

-- 
1.8.1.4

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

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@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 0ae2e93..17231d2 100644
--- a/block.c
+++ b/block.c
@@ -3305,15 +3305,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..b34179e 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 bf8db2a..9d93b83 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -382,7 +382,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 1a8ea6d..cdbee18 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -816,12 +816,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 */
     }
 
@@ -831,16 +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;
     }
 
     if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
+        error_setg(errp, "parameter 'name' has to be shorter than 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_errno(errp, -r, "failed to create snapshot");
         return r;
     }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index bb67c4c..e38bdf5 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1767,7 +1767,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;
@@ -1780,9 +1782,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 snapshot VDI "
+                   "%s (%" PRIu32 ")", sn_info->name, s->name, s->inode.vdi_id);
         return -EINVAL;
     }
 
@@ -1801,21 +1802,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     fd = connect_to_sdog(s);
     if (fd < 0) {
         ret = fd;
+        error_setg_errno(errp, errno, "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_errno(errp, errno, "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_errno(errp, errno, "failed to create inode for snapshot");
         goto cleanup;
     }
 
@@ -1825,7 +1826,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_errno(errp, errno, "failed to read new inode info");
         goto cleanup;
     }
 
diff --git a/include/block/block.h b/include/block/block.h
index 9dc6aad..ee9399c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -332,7 +332,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 0986a2d..2feaa16 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -154,7 +154,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 406caa9..77c5291 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2332,7 +2332,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 v4 01/11] block: add error parameter to bdrv_snapshot_create() and related functions

[Markus Armbruster]

When doing an error conversion, you have to decide "where to cut".
Everything above the cut is converted to Error.  Everything below keeps
using other methods, such as returning -errno.

Using Error sucks, because it's cumbersome.

Using Error is nice, because you can report errors in more detail.

Converting to Error is worthwhile when the niceness is worth the
suckage.

This patch cuts below the block driver API.  I'm not saying that's bad.
I just want to hear Kevin's opinion on it (cc'ed).

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@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 0ae2e93..17231d2 100644
> --- a/block.c
> +++ b/block.c
> @@ -3305,15 +3305,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);

Shit happens when a driver returns a negative value without setting
errp, or sets errp and returns a non-negative value.  Similar traps are
common in existing Error code, you're just following existing practice.

> +    }
> +    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..b34179e 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 bf8db2a..9d93b83 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -382,7 +382,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 1a8ea6d..cdbee18 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -816,12 +816,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 */
>      }
>  
> @@ -831,16 +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;
>      }
>  
>      if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
> +        error_setg(errp, "parameter 'name' has to be shorter than 127 chars");

Cleaner:

        error_setg(errp, "parameter 'name' has to be shorter than %zd chars",
                   sizeof(sn_info->id_str));

If you need to respin anyway...

>          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_errno(errp, -r, "failed to create snapshot");
>          return r;
>      }
>  

Interesting.  Before your patch, this function reports some, but not all
errors via error_report().  That can't be right.  I blame the lack of
written contract for the BlockDriver method.  Few of them have one.

> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index bb67c4c..e38bdf5 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1767,7 +1767,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;
> @@ -1780,9 +1782,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 snapshot VDI "
> +                   "%s (%" PRIu32 ")", sn_info->name, s->name, s->inode.vdi_id);
>          return -EINVAL;
>      }
>  
> @@ -1801,21 +1802,21 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>      fd = connect_to_sdog(s);

I think you need to convert connect_to_sdog(), too!  It calls
unix_connect() and inet_connect(), which use Error.  These errors should
be propagated up.

>      if (fd < 0) {
>          ret = fd;
> +        error_setg_errno(errp, errno, "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_errno(errp, errno, "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_errno(errp, errno, "failed to create inode for snapshot");
>          goto cleanup;
>      }
>  
> @@ -1825,7 +1826,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_errno(errp, errno, "failed to read new inode info");
>          goto cleanup;
>      }
>  
> diff --git a/include/block/block.h b/include/block/block.h
> index 9dc6aad..ee9399c 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -332,7 +332,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 0986a2d..2feaa16 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -154,7 +154,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));

First you go all the trouble to create detailed error reports deep down
in the block drivers, then you ignore them, and simply report errno
instead :)

Might change later in the series, but I haven't read that far.

> diff --git a/savevm.c b/savevm.c
> index 406caa9..77c5291 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2332,7 +2332,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));

Same here.

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

[Kevin Wolf]

Am 09.04.2013 um 15:13 hat Markus Armbruster geschrieben:
> When doing an error conversion, you have to decide "where to cut".
> Everything above the cut is converted to Error.  Everything below keeps
> using other methods, such as returning -errno.
> 
> Using Error sucks, because it's cumbersome.
> 
> Using Error is nice, because you can report errors in more detail.
> 
> Converting to Error is worthwhile when the niceness is worth the
> suckage.
> 
> This patch cuts below the block driver API.  I'm not saying that's bad.
> I just want to hear Kevin's opinion on it (cc'ed).

I think for bdrv_snapshot_create() it could make sense to pass an Error
object to the block drivers. Same thing for bdrv_create() or
bdrv_open(). Most other functions probably can't make much use of it.

Kevin

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

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
[...]
> diff --git a/block/rbd.c b/block/rbd.c
> index 1a8ea6d..cdbee18 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -816,12 +816,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 */
>      }
>  

Comment is now redundant.  Drop it if you need to respin anyway.

[...]

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

[Pavel Hrdina]

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

diff --git a/savevm.c b/savevm.c
index 77c5291..dc1f4a4 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2224,7 +2224,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;
@@ -2237,9 +2237,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;
             }
         }
@@ -2259,6 +2258,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;
@@ -2307,7 +2307,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 v4 02/11] block: add error parameter to del_existing_snapshots()

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  savevm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 77c5291..dc1f4a4 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2224,7 +2224,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;
> @@ -2237,9 +2237,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;
>              }
>          }

Any particular reason for de-capitalizing "Error"?

> @@ -2259,6 +2258,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;
> @@ -2307,7 +2307,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;
>      }

Could use hmp_handle_error(), except it's static in hmp.c.  On the other
hand, even hmp.c doesn't always use it...  Luiz, what do you think?

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

[Luiz Capitulino]

On Tue, 09 Apr 2013 15:27:32 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Pavel Hrdina <phrdina@redhat.com> writes:
> 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  savevm.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/savevm.c b/savevm.c
> > index 77c5291..dc1f4a4 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -2224,7 +2224,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;
> > @@ -2237,9 +2237,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;
> >              }
> >          }
> 
> Any particular reason for de-capitalizing "Error"?
> 
> > @@ -2259,6 +2258,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;
> > @@ -2307,7 +2307,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;
> >      }
> 
> Could use hmp_handle_error(), except it's static in hmp.c.  On the other
> hand, even hmp.c doesn't always use it...  Luiz, what do you think?

I've added hmp_handle_error() to avoid copy & paste in hmp.c, but I don't
think it's a good function because it doesn't actually handle the error
and it does two unrelated things (print the error and free it).

So, I think it's better to restrict it to hmp.c.

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

[Pavel Hrdina]

On 9.4.2013 15:27, Markus Armbruster wrote:
> Pavel Hrdina <phrdina@redhat.com> writes:
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   savevm.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 77c5291..dc1f4a4 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2224,7 +2224,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;
>> @@ -2237,9 +2237,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;
>>               }
>>           }
>
> Any particular reason for de-capitalizing "Error"?

Yes, Eric told me that we should start the error message with lover case 
and also we shouldn't print '.' at the end of the error message.

>
>> @@ -2259,6 +2258,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;
>> @@ -2307,7 +2307,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;
>>       }
>
> Could use hmp_handle_error(), except it's static in hmp.c.  On the other
> hand, even hmp.c doesn't always use it...  Luiz, what do you think?
>

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

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> On 9.4.2013 15:27, Markus Armbruster wrote:
>> Pavel Hrdina <phrdina@redhat.com> writes:
>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>   savevm.c | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/savevm.c b/savevm.c
>>> index 77c5291..dc1f4a4 100644
>>> --- a/savevm.c
>>> +++ b/savevm.c
>>> @@ -2224,7 +2224,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;
>>> @@ -2237,9 +2237,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;
>>>               }
>>>           }
>>
>> Any particular reason for de-capitalizing "Error"?
>
> Yes, Eric told me that we should start the error message with lover
> case and also we shouldn't print '.' at the end of the error message.

I agree on not ending error messages with a period.  Whether to start
them with a capital letter or not isn't worth arguing over, given the
general state of our error reporting.

[...]

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

[Eric Blake]

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

On 04/10/2013 03:57 AM, Pavel Hrdina wrote:

>> Any particular reason for de-capitalizing "Error"?
> 
> Yes, Eric told me that we should start the error message with lover case
> and also we shouldn't print '.' at the end of the error message.

More precisely, I pointed to 'git grep' results that proved that the
code base overwhelmingly avoids trailing '.', but that it was rather
ambiguous on whether messages start with a capital or a lower case.
Changing the period was important, changing the capitalization feels a
bit like churn unless we set a coding standard in HACKING telling which
way new code should use.  I can live with the churn in this commit as
long as you are changing other things in the same hunk at the same time,
but I wasn't specifically asking for capitalization changes.

-- 
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 v4 03/11] savevm: add error parameter to qemu_savevm_state_begin()

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/sysemu/sysemu.h | 3 ++-
 migration.c             | 2 +-
 savevm.c                | 5 +++--
 3 files changed, 6 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 7fb2147..d517dd6 100644
--- a/migration.c
+++ b/migration.c
@@ -505,7 +505,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 dc1f4a4..56da096 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1731,7 +1731,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;
@@ -1921,7 +1922,7 @@ 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) {
-- 
1.8.1.4

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

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/sysemu/sysemu.h | 3 ++-
>  migration.c             | 2 +-
>  savevm.c                | 5 +++--
>  3 files changed, 6 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 7fb2147..d517dd6 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -505,7 +505,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 dc1f4a4..56da096 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1731,7 +1731,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;
> @@ -1921,7 +1922,7 @@ 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) {

Unlike PATCH 01+02, this one only adds Error parameters, no actual
errors.  Do they come later in the series?

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Pavel Hrdina <phrdina@redhat.com> writes:
>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  include/sysemu/sysemu.h | 3 ++-
>>  migration.c             | 2 +-
>>  savevm.c                | 5 +++--
>>  3 files changed, 6 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 7fb2147..d517dd6 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -505,7 +505,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 dc1f4a4..56da096 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -1731,7 +1731,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;
>> @@ -1921,7 +1922,7 @@ 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) {
>
> Unlike PATCH 01+02, this one only adds Error parameters, no actual
> errors.  Do they come later in the series?

Unlikely, because qemu_savevm_state_begin() can't fail.  Why add an
Error parameter then?

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

[Pavel Hrdina]

On 9.4.2013 15:37, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Pavel Hrdina <phrdina@redhat.com> writes:
>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>   include/sysemu/sysemu.h | 3 ++-
>>>   migration.c             | 2 +-
>>>   savevm.c                | 5 +++--
>>>   3 files changed, 6 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 7fb2147..d517dd6 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -505,7 +505,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 dc1f4a4..56da096 100644
>>> --- a/savevm.c
>>> +++ b/savevm.c
>>> @@ -1731,7 +1731,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;
>>> @@ -1921,7 +1922,7 @@ 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) {
>>
>> Unlike PATCH 01+02, this one only adds Error parameters, no actual
>> errors.  Do they come later in the series?
>
> Unlikely, because qemu_savevm_state_begin() can't fail.  Why add an
> Error parameter then?
>

Thanks for pointing this out. I somehow dropped the error message from 
this function and this also applies for the next patch. I'll fix it.

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Pavel Hrdina <phrdina@redhat.com> writes:
>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>  include/sysemu/sysemu.h | 3 ++-
>>>  migration.c             | 2 +-
>>>  savevm.c                | 5 +++--
>>>  3 files changed, 6 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 7fb2147..d517dd6 100644
>>> --- a/migration.c
>>> +++ b/migration.c
>>> @@ -505,7 +505,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 dc1f4a4..56da096 100644
>>> --- a/savevm.c
>>> +++ b/savevm.c
>>> @@ -1731,7 +1731,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;
>>> @@ -1921,7 +1922,7 @@ 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) {
>>
>> Unlike PATCH 01+02, this one only adds Error parameters, no actual
>> errors.  Do they come later in the series?
>
> Unlikely, because qemu_savevm_state_begin() can't fail.  Why add an
> Error parameter then?

Bear with me, I'm confused...

Actually, it can fail, sort of: it calls qemu_file_set_error() then.
But you don't return that error via your new Error parameter.  Perhaps
that comes later in the series.

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

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/sysemu/sysemu.h | 2 +-
 migration.c             | 2 +-
 savevm.c                | 4 ++--
 3 files changed, 4 insertions(+), 4 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 d517dd6..e9d2f40 100644
--- a/migration.c
+++ b/migration.c
@@ -516,7 +516,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 56da096..575f1b2 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1784,7 +1784,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;
@@ -1926,7 +1926,7 @@ static int qemu_savevm_state(QEMUFile *f)
     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 v4 04/11] savevm: add error parameter to qemu_savevm_state_iterate()

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/sysemu/sysemu.h | 2 +-
>  migration.c             | 2 +-
>  savevm.c                | 4 ++--
>  3 files changed, 4 insertions(+), 4 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 d517dd6..e9d2f40 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -516,7 +516,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 56da096..575f1b2 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1784,7 +1784,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;

       QTAILQ_FOREACH(se, &savevm_handlers, entry) {
           if (!se->ops || !se->ops->save_live_iterate) {
               continue;
           }
           if (se->ops && se->ops->is_active) {
               if (!se->ops->is_active(se->opaque)) {
                   continue;
               }
           }
           if (qemu_file_rate_limit(f)) {
               return 0;
           }
           trace_savevm_section_start();
           /* Section type */
           qemu_put_byte(f, QEMU_VM_SECTION_PART);
           qemu_put_be32(f, se->section_id);

           ret = se->ops->save_live_iterate(f, se->opaque);
           trace_savevm_section_end(se->section_id);

           if (ret < 0) {
               qemu_file_set_error(f, ret);
           }
           if (ret <= 0) {
               /* Do not proceed to the next vmstate before this one reported
                  completion of the current stage. This serializes the migration
                  and reduces the probability that a faster changing state is
                  synchronized over and over again. */
               break;

Returns an error without setting the Error parameter.  Evil :)

           }
       }
       return ret;
   }

> @@ -1926,7 +1926,7 @@ static int qemu_savevm_state(QEMUFile *f)
>      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;
>          }
>      }

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

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@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 e9d2f40..c891a45 100644
--- a/migration.c
+++ b/migration.c
@@ -525,7 +525,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 575f1b2..7598934 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1823,7 +1823,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;
@@ -1848,6 +1848,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;
         }
     }
@@ -1933,7 +1934,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 v4 05/11] savevm: add error parameter to qemu_savevm_state_complete()

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@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 e9d2f40..c891a45 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -525,7 +525,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 575f1b2..7598934 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1823,7 +1823,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;
> @@ -1848,6 +1848,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;
>          }
>      }

Unlike PATCH 3+4, this one returns an error via the Error parameter on
qemu_file_set_error().

If I remember correctly, qemu_file_set_error() takes a -errno argument,
here: ret.  error_setg_errno(), perhaps?

> @@ -1933,7 +1934,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) {

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

[Pavel Hrdina]

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

diff --git a/savevm.c b/savevm.c
index 7598934..3c1ac9e 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1910,7 +1910,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 = {
@@ -1918,23 +1918,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) {
@@ -2321,7 +2321,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 v4 06/11] savevm: add error parameter to qemu_savevm_state()

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  savevm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 7598934..3c1ac9e 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1910,7 +1910,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 = {
> @@ -1918,23 +1918,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) {
> @@ -2321,7 +2321,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) {
           monitor_printf(mon, "Error %d while writing VM\n", ret);
           goto the_end;
       }

First you go all the trouble to create detailed error reports deep down
in savevm.c, then you ignore them, and simply report (numeric!) errno
instead :)

[Qemu-devel] [PATCH v4 07/11] qapi: Convert savevm

[Pavel Hrdina]

QMP command "vm-snapshot-save" has also extra optional force parameter
to specify whether replace existing snapshot or not. It also returns
information about created snapshot.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hmp-commands.hx         |  4 ++--
 hmp.c                   | 11 +++++++++
 hmp.h                   |  1 +
 include/sysemu/sysemu.h |  1 -
 qapi-schema.json        | 22 ++++++++++++++++++
 qmp-commands.hx         | 43 ++++++++++++++++++++++++++++++++++
 savevm.c                | 62 +++++++++++++++++++++++++++++++------------------
 7 files changed, 118 insertions(+), 26 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 3d98604..382b87d 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 dbe9b90..b38b6ce 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1433,3 +1433,14 @@ 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;
+    SnapshotInfo *info = NULL;
+
+    info = qmp_vm_snapshot_save(!!name, name, true, true, &err);
+    qapi_free_SnapshotInfo(info);
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 80e8b41..1bb8590 100644
--- a/hmp.h
+++ b/hmp.h
@@ -86,5 +86,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 f629a24..cbb73d9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3453,3 +3453,25 @@
 # 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, 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: SnapshotInfo on success
+#
+# Since: 1.5
+##
+{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str', '*force': 'bool'},
+  'returns': 'SnapshotInfo' }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1e0e11e..119e7a5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1445,6 +1445,49 @@ Example:
 
 EQMP
     {
+        .name       = "vm-snapshot-save",
+        .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
+    },
+
+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, 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": 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:
+
+-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
+<- {
+      "return": {
+         "id": "1",
+         "name": "my_snapshot",
+         "date-sec": 1364480534,
+         "date-nsec": 978215000,
+         "vm-clock-sec": 5,
+         "vm-clock-nsec": 153620449,
+         "vm-state-size": 5709953
+      }
+   }
+
+EQMP
+    {
         .name       = "qmp_capabilities",
         .args_type  = "",
         .params     = "",
diff --git a/savevm.c b/savevm.c
index 3c1ac9e..7b127eb 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2249,17 +2249,18 @@ static int del_existing_snapshots(const char *name, Error **errp)
     return 0;
 }
 
-void do_savevm(Monitor *mon, const QDict *qdict)
+SnapshotInfo *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;
+    SnapshotInfo *info = NULL;
     int ret;
     QEMUFile *f;
     int saved_vm_running;
     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 */
@@ -2271,16 +2272,16 @@ 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));
-            return;
+            error_setg(errp, "device '%s' is writable but does not support "
+                       "snapshots", bdrv_get_device_name(bs));
+            return NULL;
         }
     }
 
     bs = bdrv_snapshots();
     if (!bs) {
-        monitor_printf(mon, "No block device can accept snapshots\n");
-        return;
+        error_setg(errp, "no block device can accept snapshots");
+        return NULL;
     }
 
     saved_vm_running = runstate_is_running();
@@ -2294,11 +2295,23 @@ 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);
-            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);
         }
@@ -2308,24 +2321,17 @@ void do_savevm(Monitor *mon, const QDict *qdict)
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }
 
-    /* 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);
-        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;
     }
 
@@ -2336,17 +2342,27 @@ 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);
             }
         }
     }
 
+    info = g_malloc0(sizeof(SnapshotInfo));
+    info->id = g_strdup(sn->id_str);
+    info->name = g_strdup(sn->name);
+    info->date_nsec = sn->date_nsec;
+    info->date_sec = sn->date_sec;
+    info->vm_state_size = vm_state_size;
+    info->vm_clock_nsec = sn->vm_clock_nsec % 1000000000;
+    info->vm_clock_sec = sn->vm_clock_nsec / 1000000000;
+
  the_end:
     if (saved_vm_running)
         vm_start();
+
+    return info;
 }
 
 void qmp_xen_save_devices_state(const char *filename, Error **errp)
-- 
1.8.1.4

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

[Eric Blake]

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

On 03/29/2013 08:12 AM, Pavel Hrdina wrote:
> QMP command "vm-snapshot-save" has also extra optional force parameter
> to specify whether replace existing snapshot or not. It also returns
> information about created snapshot.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hmp-commands.hx         |  4 ++--
>  hmp.c                   | 11 +++++++++
>  hmp.h                   |  1 +
>  include/sysemu/sysemu.h |  1 -
>  qapi-schema.json        | 22 ++++++++++++++++++
>  qmp-commands.hx         | 43 ++++++++++++++++++++++++++++++++++
>  savevm.c                | 62 +++++++++++++++++++++++++++++++------------------
>  7 files changed, 118 insertions(+), 26 deletions(-)

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

Rest of series already has my review from earlier versions, so I only
gave them a cursory glance this time around, but assume they are still okay.

-- 
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 v4 07/11] qapi: Convert savevm

[Pavel Hrdina]

On 29.3.2013 17:12, Eric Blake wrote:
> On 03/29/2013 08:12 AM, Pavel Hrdina wrote:
>> QMP command "vm-snapshot-save" has also extra optional force parameter
>> to specify whether replace existing snapshot or not. It also returns
>> information about created snapshot.
>>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   hmp-commands.hx         |  4 ++--
>>   hmp.c                   | 11 +++++++++
>>   hmp.h                   |  1 +
>>   include/sysemu/sysemu.h |  1 -
>>   qapi-schema.json        | 22 ++++++++++++++++++
>>   qmp-commands.hx         | 43 ++++++++++++++++++++++++++++++++++
>>   savevm.c                | 62 +++++++++++++++++++++++++++++++------------------
>>   7 files changed, 118 insertions(+), 26 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Rest of series already has my review from earlier versions, so I only
> gave them a cursory glance this time around, but assume they are still okay.
>

Thanks a lot for your reviews,

Pavel

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

[Markus Armbruster]

Patch no longer applies.

Pavel Hrdina <phrdina@redhat.com> writes:

> QMP command "vm-snapshot-save" has also extra optional force parameter
> to specify whether replace existing snapshot or not. It also returns
> information about created snapshot.

Took me a minute to realize that you're comparing it to HMP command
savevm.

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hmp-commands.hx         |  4 ++--
>  hmp.c                   | 11 +++++++++
>  hmp.h                   |  1 +
>  include/sysemu/sysemu.h |  1 -
>  qapi-schema.json        | 22 ++++++++++++++++++
>  qmp-commands.hx         | 43 ++++++++++++++++++++++++++++++++++
>  savevm.c                | 62 +++++++++++++++++++++++++++++++------------------
>  7 files changed, 118 insertions(+), 26 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 3d98604..382b87d 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
>  

Markup fix squashed in.  Acceptable, because it's really minor.

> diff --git a/hmp.c b/hmp.c
> index dbe9b90..b38b6ce 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1433,3 +1433,14 @@ 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;
> +    SnapshotInfo *info = NULL;
> +
> +    info = qmp_vm_snapshot_save(!!name, name, true, true, &err);
> +    qapi_free_SnapshotInfo(info);
> +    hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 80e8b41..1bb8590 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -86,5 +86,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 f629a24..cbb73d9 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3453,3 +3453,25 @@
>  # 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, the force argument needs to be true to replace it.

"tag or id"?

HMP command savevm's argument is matched against both QEMUSnapshotInfo
member id_str (a.k.a. id) and name (a.k.a. tag).  Do we really want that
kind of overloading in QMP?  Shouldn't we make it tag vs. id explicit
there?

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

Should we make this mandatory?  We have to keep the argument optional in
HMP, but that needn't concern us here.

> +#
> +# @force: #optional specify whether existing snapshot is replaced or not,
> +#         default is false
> +#
> +# Returns: SnapshotInfo on success
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str', '*force': 'bool'},
> +  'returns': 'SnapshotInfo' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1e0e11e..119e7a5 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1445,6 +1445,49 @@ Example:
>  
>  EQMP
>      {
> +        .name       = "vm-snapshot-save",
> +        .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
> +    },
> +
> +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, 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": 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:
> +
> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
> +<- {
> +      "return": {
> +         "id": "1",
> +         "name": "my_snapshot",
> +         "date-sec": 1364480534,
> +         "date-nsec": 978215000,
> +         "vm-clock-sec": 5,
> +         "vm-clock-nsec": 153620449,
> +         "vm-state-size": 5709953
> +      }
> +   }
> +
> +EQMP
> +    {
>          .name       = "qmp_capabilities",
>          .args_type  = "",
>          .params     = "",
> diff --git a/savevm.c b/savevm.c
> index 3c1ac9e..7b127eb 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2249,17 +2249,18 @@ static int del_existing_snapshots(const char *name, Error **errp)
>      return 0;
>  }
>  
> -void do_savevm(Monitor *mon, const QDict *qdict)
> +SnapshotInfo *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;
> +    SnapshotInfo *info = NULL;
>      int ret;
>      QEMUFile *f;
>      int saved_vm_running;
>      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 */
> @@ -2271,16 +2272,16 @@ 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));
> -            return;
> +            error_setg(errp, "device '%s' is writable but does not support "
> +                       "snapshots", bdrv_get_device_name(bs));
> +            return NULL;

Any particular reason for de-capitalizing "Device"?  More of the same below.

>          }
>      }
>  
>      bs = bdrv_snapshots();
>      if (!bs) {
> -        monitor_printf(mon, "No block device can accept snapshots\n");
> -        return;
> +        error_setg(errp, "no block device can accept snapshots");
> +        return NULL;
>      }
>  
>      saved_vm_running = runstate_is_running();
> @@ -2294,11 +2295,23 @@ 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);
> -            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);
>          }
       } else {
           /* cast below needed for OpenBSD where tv_sec is still 'long' */
           localtime_r((const time_t *)&tv.tv_sec, &tm);
> @@ -2308,24 +2321,17 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>          strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);

Interestingly, we do not check whether this auto-generated name already
exists.  Not your fault, but could use fixing.

>      }
>  
> -    /* 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);
> -        goto the_end;
> -    }
> -

Your code motion adds two conditions for calling
del_existing_snapshots():

1. has_force && force: feature.  I guess I'd add the feature in a
separate patch, to keep this one as simple as possible.  Your choice.

2. bdrv_snapshot_find() succeeds.  Care to explain why this one's
correct?

>      /* 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;
>      }
>  
> @@ -2336,17 +2342,27 @@ 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);

When we go through this failure...

>              }
>          }
>      }
>  
> +    info = g_malloc0(sizeof(SnapshotInfo));
> +    info->id = g_strdup(sn->id_str);
> +    info->name = g_strdup(sn->name);
> +    info->date_nsec = sn->date_nsec;
> +    info->date_sec = sn->date_sec;
> +    info->vm_state_size = vm_state_size;
> +    info->vm_clock_nsec = sn->vm_clock_nsec % 1000000000;
> +    info->vm_clock_sec = sn->vm_clock_nsec / 1000000000;
> +
>   the_end:
>      if (saved_vm_running)
>          vm_start();
> +
> +    return info;

... we return both info and an Error.  Stupid question: is that okay?

>  }
>  
>  void qmp_xen_save_devices_state(const char *filename, Error **errp)

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

[Eric Blake]

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

On 04/09/2013 10:04 AM, Markus Armbruster wrote:

>> +
>> +##
>> +# @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, the force argument needs to be true to replace it.
> 
> "tag or id"?
> 
> HMP command savevm's argument is matched against both QEMUSnapshotInfo
> member id_str (a.k.a. id) and name (a.k.a. tag).  Do we really want that
> kind of overloading in QMP?  Shouldn't we make it tag vs. id explicit
> there?

The qcow2 file format already allows for the creation of a snapshot with
an id but no tag.  We've been back and forth about whether QMP should
allow a user to be able to create all valid qcow2 files (and hence allow
the omission of tag).  But things really get confusing if you allow the
creation of a tag that matches an existing id (create a snapshot, then
create another snapshot with the tag '1'); or even if a tag can match a
potential future id (create a snapshot with a tag named '2', then create
another snapshot).  So the lookup should always check for BOTH tag and
id, even though the command is supplying only a 'tag'; and if a tag is
omitted, a unique id will always be returned.

> 
>> +#
>> +# 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
> 
> Should we make this mandatory?  We have to keep the argument optional in
> HMP, but that needn't concern us here.

If we mandate that a tag always be supplied, then we cannot create qcow2
files with a snapshot that lacks a tag using just QMP; but even if we do
that, we STILL have to support use of existing qcow2 files that were
created by earlier qemu and/or other tools that have snapshots without a
tag.

-- 
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 v4 07/11] qapi: Convert savevm

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 04/09/2013 10:04 AM, Markus Armbruster wrote:
>
>>> +
>>> +##
>>> +# @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, the force argument needs to be true to
>>> replace it.
>> 
>> "tag or id"?
>> 
>> HMP command savevm's argument is matched against both QEMUSnapshotInfo
>> member id_str (a.k.a. id) and name (a.k.a. tag).  Do we really want that
>> kind of overloading in QMP?  Shouldn't we make it tag vs. id explicit
>> there?
>
> The qcow2 file format already allows for the creation of a snapshot with
> an id but no tag.  We've been back and forth about whether QMP should
> allow a user to be able to create all valid qcow2 files (and hence allow
> the omission of tag).  But things really get confusing if you allow the

I don't have all that context; I hope I'm not wasting everyone's time.

> creation of a tag that matches an existing id (create a snapshot, then
> create another snapshot with the tag '1'); or even if a tag can match a
> potential future id (create a snapshot with a tag named '2', then create
> another snapshot).  So the lookup should always check for BOTH tag and
> id, even though the command is supplying only a 'tag'; and if a tag is
> omitted, a unique id will always be returned.
>
>> 
>>> +#
>>> +# 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
>> 
>> Should we make this mandatory?  We have to keep the argument optional in
>> HMP, but that needn't concern us here.
>
> If we mandate that a tag always be supplied, then we cannot create qcow2
> files with a snapshot that lacks a tag using just QMP; but even if we do

Are you sure you can do that with the proposed patches?  If I read them
correctly, we make up a tag vm-%Y%m%d%H%M%S when none is given.

> that, we STILL have to support use of existing qcow2 files that were
> created by earlier qemu and/or other tools that have snapshots without a
> tag.

I understand the need to deal with existing qcow2 files lacking tags.

I understand the desire to be able to create such files via QMP.  I
don't share it, though :)

For block driver method bdrv_snapshot_create() both tag and ID are
optional.  If ID is missing, it picks one.  qcow2 picks max. existing ID
+ 1.  If tag is missing, the snapshot doesn't get one.

When savevm overwrites an existing snapshot, it reuses both tag and ID.
One of them matches @name.

When savevm creates a new snapshot, @name becomes the tag.  If @name
isn't given, we make one up.  ID is left to the block driver.

vm-snapshot-save behaves the same.

Say we have two snapshots, with IDs 1 and 5.  Say I want to overwrite
the first one, and chose to identify it by its ID.  Since I got dirt on
my monitor, I misread ID 7.  Since neither ID nor tag 7 exist, I
accidentally create a new snapshot with tag 7 and ID 6 (picked by qcow2
block driver).  Next new snapshot will inevitably get ID 7 and the
confusion is complete.  All because the stupid command doesn't let me
express myself clearly: I mean *ID* 7, *not* tag 7!

I think I'd try the following for QMP:

* Drop the capability to overwrite existing snapshot.  No real loss, as
  it's not an atomic overwrite: it first deletes the old one, then
  creates the new one, and if create fails, the old one's still gone.

* Take parameter @tag and @id.

  Fail if @tag is given and matches any existing tag.  If it's not
  given, we make one up that doesn't match any existing tag.

  Fail if @id is given and matches any existing ID.  If it's not given,
  we make one up that doesn't match any existing ID.

* Seriously consider making @tag mandatory.

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

[Eric Blake]

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

On 04/09/2013 11:23 AM, Markus Armbruster wrote:
>> If we mandate that a tag always be supplied, then we cannot create qcow2
>> files with a snapshot that lacks a tag using just QMP; but even if we do
> 
> Are you sure you can do that with the proposed patches?  If I read them
> correctly, we make up a tag vm-%Y%m%d%H%M%S when none is given.

HMP is higher-level than QMP, so having HMP default to always generating
a tag seems like a reasonable thing.  In my opinion, it's okay for
higher layers to ensure a name even when lower layers leave it optional.
 Besides, HMP aims to ease user interface and it is easier to work with
named snapshots than it is to work with anonymous ones, even if that
means HMP generates a name.  We already have instances where HMP is
intentionally less powerful than QMP, because if you are using the human
interface, you don't need to be bothered by the subtleties - and if you
need the full power, then you use the lower-level interface.  On the
other hand, we could always change our mind later to have HMP create
anonymous snapshots (by adding another flag to HMP 'savevm') instead of
generating a name.

Where we would get into trouble is if QMP enforces a name but we change
our mind and later want HMP to again expose the possibility of anonymous
snapshots.  Also, if QMP doesn't expose the full power of the file
format on creation, but still has to support the full power of the file
format on the loadvm side, it becomes that much harder to test that the
loadvm support is correct in the presence of anonymous snapshots.

> 
>> that, we STILL have to support use of existing qcow2 files that were
>> created by earlier qemu and/or other tools that have snapshots without a
>> tag.
> 
> I understand the need to deal with existing qcow2 files lacking tags.
> 
> I understand the desire to be able to create such files via QMP.  I
> don't share it, though :)

I'm on the fence - I could live with the requirement that QMP requires a
name, and that creation of anonymous snapshots requires other tools.
Libvirt will always pass a name, for what it is worth, regardless of
whether HMP generates a name, and regardless of whether QMP requires a
name.  But testing nameless support is harder if it requires an extra
tool to create a nameless snapshot in the first place.

> 
> For block driver method bdrv_snapshot_create() both tag and ID are
> optional.  If ID is missing, it picks one.  qcow2 picks max. existing ID
> + 1.  If tag is missing, the snapshot doesn't get one.
> 
> When savevm overwrites an existing snapshot, it reuses both tag and ID.
> One of them matches @name.
> 
> When savevm creates a new snapshot, @name becomes the tag.  If @name
> isn't given, we make one up.  ID is left to the block driver.
> 
> vm-snapshot-save behaves the same.
> 
> Say we have two snapshots, with IDs 1 and 5.  Say I want to overwrite
> the first one, and chose to identify it by its ID.  Since I got dirt on
> my monitor, I misread ID 7.  Since neither ID nor tag 7 exist, I
> accidentally create a new snapshot with tag 7 and ID 6 (picked by qcow2
> block driver).  Next new snapshot will inevitably get ID 7 and the
> confusion is complete.  All because the stupid command doesn't let me
> express myself clearly: I mean *ID* 7, *not* tag 7!

Yep - another instance of the confusion that current HMP 'savevm' has,
and that I want to get rid of by adding -f as a force option at a
minimum to the HMP layer.

> 
> I think I'd try the following for QMP:
> 
> * Drop the capability to overwrite existing snapshot.  No real loss, as
>   it's not an atomic overwrite: it first deletes the old one, then
>   creates the new one, and if create fails, the old one's still gone.

We'd _still_ want to add HMP 'savevm -f' to force an overwrite, but you
are saying that the HMP command would then be a sequence of _two_ QMP
calls - delete then create, where QMP create _always_ fails if a
snapshot already exists with that name.  Makes sense - it just means
that the 'force' semantics are now put as a burden on the caller,
instead of on QMP.

> 
> * Take parameter @tag and @id.
> 
>   Fail if @tag is given and matches any existing tag.  If it's not
>   given, we make one up that doesn't match any existing tag.

Generating an id makes sense.  And the command must return the id that
got used (whether passed in by the user or generated).

> 
>   Fail if @id is given and matches any existing ID.  If it's not given,
>   we make one up that doesn't match any existing ID.

I don't know if I like the idea of QMP generating a tag, vs. leaving the
snapshot anonymous.  But note that the answer to the next question
determines whether we can bypass this question, since if...

> 
> * Seriously consider making @tag mandatory.

...tag is made mandatory, then QMP never has to generate a tag.  I'm
still on the fence on whether to always have a tag in place (whether by
QMP generation or by mandatory argument) vs. allowing anonymous
snapshots.  But again, libvirt always names its snapshots, and I trust
your judgment as maintainer of this area of code in which way we should go.

-- 
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 v4 07/11] qapi: Convert savevm

[Markus Armbruster]

[Note Cc: Kevin]

Eric Blake <eblake@redhat.com> writes:

> On 04/09/2013 11:23 AM, Markus Armbruster wrote:
>>> If we mandate that a tag always be supplied, then we cannot create qcow2
>>> files with a snapshot that lacks a tag using just QMP; but even if we do
>> 
>> Are you sure you can do that with the proposed patches?  If I read them
>> correctly, we make up a tag vm-%Y%m%d%H%M%S when none is given.
>
> HMP is higher-level than QMP, so having HMP default to always generating
> a tag seems like a reasonable thing.  In my opinion, it's okay for
> higher layers to ensure a name even when lower layers leave it optional.
>  Besides, HMP aims to ease user interface and it is easier to work with
> named snapshots than it is to work with anonymous ones, even if that
> means HMP generates a name.  We already have instances where HMP is
> intentionally less powerful than QMP, because if you are using the human
> interface, you don't need to be bothered by the subtleties - and if you
> need the full power, then you use the lower-level interface.  On the
> other hand, we could always change our mind later to have HMP create
> anonymous snapshots (by adding another flag to HMP 'savevm') instead of
> generating a name.

We agree on HMP.

> Where we would get into trouble is if QMP enforces a name but we change
> our mind and later want HMP to again expose the possibility of anonymous
> snapshots.

The proposed patches don't let you create anonymous snapshots via QMP,
as far as I can tell.  The same default tag is used.

>             Also, if QMP doesn't expose the full power of the file
> format on creation, but still has to support the full power of the file
> format on the loadvm side, it becomes that much harder to test that the
> loadvm support is correct in the presence of anonymous snapshots.

Are anonymous snapshots actually good for anything?  Or are they just a
regrettable historical accident we need to cope with somehow?

In the latter case, we can handle them in qemu (loadvm accepts them), or
in qemu-img (set snapshot tag).

Regardless of where we support anonymous snapshots, we need to be able
to create them for testing purposes.  We can create them either in qemu
(vm-snapshot-save), or in qemu-img (unset snapshot tag).

>>> that, we STILL have to support use of existing qcow2 files that were
>>> created by earlier qemu and/or other tools that have snapshots without a
>>> tag.
>> 
>> I understand the need to deal with existing qcow2 files lacking tags.
>> 
>> I understand the desire to be able to create such files via QMP.  I
>> don't share it, though :)
>
> I'm on the fence - I could live with the requirement that QMP requires a
> name, and that creation of anonymous snapshots requires other tools.
> Libvirt will always pass a name, for what it is worth, regardless of
> whether HMP generates a name, and regardless of whether QMP requires a
> name.  But testing nameless support is harder if it requires an extra
> tool to create a nameless snapshot in the first place.

Not really hard if we limit anonymous snapshot support to qemu-img:
change snapshot tag.

As far as I can tell, qemu can't create anonymous snapshots anymore
since commit 7d631a1 "savevm: Generate a name when run without one" from
August 2010.  Hasn't impeded testing so far (possibly because there has
been none).

>> For block driver method bdrv_snapshot_create() both tag and ID are
>> optional.  If ID is missing, it picks one.  qcow2 picks max. existing ID
>> + 1.  If tag is missing, the snapshot doesn't get one.
>> 
>> When savevm overwrites an existing snapshot, it reuses both tag and ID.
>> One of them matches @name.
>> 
>> When savevm creates a new snapshot, @name becomes the tag.  If @name
>> isn't given, we make one up.  ID is left to the block driver.
>> 
>> vm-snapshot-save behaves the same.
>> 
>> Say we have two snapshots, with IDs 1 and 5.  Say I want to overwrite
>> the first one, and chose to identify it by its ID.  Since I got dirt on
>> my monitor, I misread ID 7.  Since neither ID nor tag 7 exist, I
>> accidentally create a new snapshot with tag 7 and ID 6 (picked by qcow2
>> block driver).  Next new snapshot will inevitably get ID 7 and the
>> confusion is complete.  All because the stupid command doesn't let me
>> express myself clearly: I mean *ID* 7, *not* tag 7!
>
> Yep - another instance of the confusion that current HMP 'savevm' has,
> and that I want to get rid of by adding -f as a force option at a
> minimum to the HMP layer.

Current patches' -f doesn't help in my example.

>> I think I'd try the following for QMP:
>> 
>> * Drop the capability to overwrite existing snapshot.  No real loss, as
>>   it's not an atomic overwrite: it first deletes the old one, then
>>   creates the new one, and if create fails, the old one's still gone.
>
> We'd _still_ want to add HMP 'savevm -f' to force an overwrite, but you
> are saying that the HMP command would then be a sequence of _two_ QMP
> calls - delete then create, where QMP create _always_ fails if a
> snapshot already exists with that name.  Makes sense - it just means
> that the 'force' semantics are now put as a burden on the caller,
> instead of on QMP.

I'm fine with HMP 'savevm -f'.  In HMP, we go for user convenience in
common use cases.

For QMP, it's generally better to go for building blocks, i.e. commands
that do just one thing.  Let the client combine them however it sees
fit.

>> * Take parameter @tag and @id.
>> 
>>   Fail if @tag is given and matches any existing tag.  If it's not
>>   given, we make one up that doesn't match any existing tag.
>
> Generating an id makes sense.  And the command must return the id that
> got used (whether passed in by the user or generated).

Yes.

>>   Fail if @id is given and matches any existing ID.  If it's not given,
>>   we make one up that doesn't match any existing ID.
>
> I don't know if I like the idea of QMP generating a tag, vs. leaving the
> snapshot anonymous.  But note that the answer to the next question
> determines whether we can bypass this question, since if...
>
>> 
>> * Seriously consider making @tag mandatory.
>
> ...tag is made mandatory, then QMP never has to generate a tag.  I'm
> still on the fence on whether to always have a tag in place (whether by
> QMP generation or by mandatory argument) vs. allowing anonymous
> snapshots.  But again, libvirt always names its snapshots, and I trust
> your judgment as maintainer of this area of code in which way we should go.

>From a QMP point of view, I like a mandatory tag, because it reduces
headaches.

For the QCOW2 / block layer / qemu-img point of view, I'm cc'ing Kevin.

[Qemu-devel] [PATCH v4 08/11] qemu-img: introduce qemu_img_handle_error

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 21d02bf..d5f81cc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -322,6 +322,17 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
     return 0;
 }
 
+static int qemu_img_handle_error(Error *err)
+{
+    if (error_is_set(&err)) {
+        error_report("%s", error_get_pretty(err));
+        error_free(err);
+        return 1;
+    }
+
+    return 0;
+}
+
 static int img_create(int argc, char **argv)
 {
     int c;
@@ -400,13 +411,8 @@ 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);
-        return 1;
-    }
 
-    return 0;
+    return qemu_img_handle_error(local_err);
 }
 
 static void dump_json_image_check(ImageCheck *check, bool quiet)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v4 08/11] qemu-img: introduce qemu_img_handle_error

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-img.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 21d02bf..d5f81cc 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -322,6 +322,17 @@ static int add_old_style_options(const char *fmt, QEMUOptionParameter *list,
>      return 0;
>  }
>  
> +static int qemu_img_handle_error(Error *err)
> +{
> +    if (error_is_set(&err)) {
> +        error_report("%s", error_get_pretty(err));
> +        error_free(err);
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
>  static int img_create(int argc, char **argv)
>  {
>      int c;
> @@ -400,13 +411,8 @@ 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);
> -        return 1;
> -    }
>  
> -    return 0;
> +    return qemu_img_handle_error(local_err);
>  }
>  
>  static void dump_json_image_check(ImageCheck *check, bool quiet)

If you put this before PATCH 1, you can use qemu_img_handle_error() in
PATCH 01 for reporting bdrv_snapshot_create() failure the right way
right away.  You do it in PATCH 09 now.

[Qemu-devel] [PATCH v4 09/11] block: update return value from bdrv_snapshot_create

[Pavel Hrdina]

If we provide error message we don't have to also provide return
value because we could check if there is any error message or not.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                   | 24 ++++++++++--------------
 block/qcow2-snapshot.c    | 12 +++++-------
 block/qcow2.h             |  6 +++---
 block/rbd.c               | 15 ++++++---------
 block/sheepdog.c          |  9 ++++-----
 include/block/block.h     |  6 +++---
 include/block/block_int.h |  6 +++---
 qemu-img.c                |  9 ++++-----
 savevm.c                  |  4 ++--
 9 files changed, 40 insertions(+), 51 deletions(-)

diff --git a/block.c b/block.c
index 17231d2..d009ce7 100644
--- a/block.c
+++ b/block.c
@@ -3304,27 +3304,23 @@ BlockDriverState *bdrv_snapshots(void)
     return NULL;
 }
 
-int bdrv_snapshot_create(BlockDriverState *bs,
-                         QEMUSnapshotInfo *sn_info,
-                         Error **errp)
+void bdrv_snapshot_create(BlockDriverState *bs,
+                          QEMUSnapshotInfo *sn_info,
+                          Error **errp)
 {
     BlockDriver *drv = bs->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, errp);
-    }
-    if (bs->file) {
-        return bdrv_snapshot_create(bs->file, sn_info, errp);
+    } else if (drv->bdrv_snapshot_create) {
+        drv->bdrv_snapshot_create(bs, sn_info, errp);
+    } else if (bs->file) {
+        bdrv_snapshot_create(bs->file, sn_info, errp);
+    } else {
+        error_setg(errp, "snapshot is not supported for '%s'",
+                   bdrv_get_format_name(bs));
     }
-
-    error_setg(errp, "snapshot is not supported for '%s'",
-               bdrv_get_format_name(bs));
-    return -ENOTSUP;
 }
 
 int bdrv_snapshot_goto(BlockDriverState *bs,
diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index b34179e..f49b95f 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -315,9 +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,
-                          Error **errp)
+void qcow2_snapshot_create(BlockDriverState *bs,
+                           QEMUSnapshotInfo *sn_info,
+                           Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot *new_snapshot_list = NULL;
@@ -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;
     }
 
     /* Populate sn with passed data */
@@ -413,14 +413,12 @@ int qcow2_snapshot_create(BlockDriverState *bs,
       qcow2_check_refcounts(bs, &result, 0);
     }
 #endif
-    return 0;
+    return;
 
 fail:
     g_free(sn->id_str);
     g_free(sn->name);
     g_free(l1_table);
-
-    return ret;
 }
 
 /* copy the snapshot 'snapshot_name' into the current disk image */
diff --git a/block/qcow2.h b/block/qcow2.h
index 9d93b83..d467478 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -382,9 +382,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,
-                          Error **errp);
+void 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 cdbee18..5167f2c 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -815,16 +815,16 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
     return 0;
 }
 
-static int qemu_rbd_snap_create(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info,
-                                Error **errp)
+static void qemu_rbd_snap_create(BlockDriverState *bs,
+                                 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 */
+        return; /* we need a name for rbd snapshots */
     }
 
     /*
@@ -834,21 +834,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;
     }
 
     if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
         error_setg(errp, "parameter 'name' has to be shorter than 127 chars");
-        return -ERANGE;
+        return;
     }
 
     r = rbd_snap_create(s->image, sn_info->name);
     if (r < 0) {
         error_setg_errno(errp, -r, "failed to create snapshot");
-        return r;
     }
-
-    return 0;
 }
 
 static int qemu_rbd_snap_remove(BlockDriverState *bs,
diff --git a/block/sheepdog.c b/block/sheepdog.c
index e38bdf5..cf11dfb 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1767,9 +1767,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,
-                              Error **errp)
+static void sd_snapshot_create(BlockDriverState *bs,
+                               QEMUSnapshotInfo *sn_info,
+                               Error **errp)
 {
     BDRVSheepdogState *s = bs->opaque;
     int ret, fd;
@@ -1784,7 +1784,7 @@ static int sd_snapshot_create(BlockDriverState *bs,
     if (s->is_snapshot) {
         error_setg(errp, "you can't create a snapshot '%s' of a snapshot VDI "
                    "%s (%" PRIu32 ")", sn_info->name, s->name, s->inode.vdi_id);
-        return -EINVAL;
+        return;
     }
 
     dprintf("%s %s\n", sn_info->name, sn_info->id_str);
@@ -1836,7 +1836,6 @@ static int sd_snapshot_create(BlockDriverState *bs,
 
 cleanup:
     closesocket(fd);
-    return ret;
 }
 
 static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
diff --git a/include/block/block.h b/include/block/block.h
index ee9399c..1a6a6a7 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -331,9 +331,9 @@ BlockStats *bdrv_query_stats(const BlockDriverState *bs);
 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,
-                         Error **errp);
+void bdrv_snapshot_create(BlockDriverState *bs,
+                          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 2feaa16..8760517 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -153,9 +153,9 @@ struct BlockDriver {
     int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
                                  const uint8_t *buf, int nb_sectors);
 
-    int (*bdrv_snapshot_create)(BlockDriverState *bs,
-                                QEMUSnapshotInfo *sn_info,
-                                Error **errp);
+    void (*bdrv_snapshot_create)(BlockDriverState *bs,
+                                 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 d5f81cc..154d913 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1942,6 +1942,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 */
@@ -2018,11 +2019,9 @@ 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);
-        if (ret) {
-            error_report("Could not create snapshot '%s': %d (%s)",
-                snapshot_name, ret, strerror(-ret));
-        }
+        local_err = NULL;
+        bdrv_snapshot_create(bs, &sn, &local_err);
+        ret = qemu_img_handle_error(local_err);
         break;
 
     case SNAPSHOT_APPLY:
diff --git a/savevm.c b/savevm.c
index 7b127eb..6ac4ece 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2342,8 +2342,8 @@ SnapshotInfo *qmp_vm_snapshot_save(bool has_name, const char *name,
         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, &local_err);
-            if (ret < 0) {
+            bdrv_snapshot_create(bs1, sn, &local_err);
+            if (error_is_set(&local_err)) {
                 error_propagate(errp, local_err);
             }
         }
-- 
1.8.1.4

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

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> If we provide error message we don't have to also provide return
> value because we could check if there is any error message or not.
>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c                   | 24 ++++++++++--------------
>  block/qcow2-snapshot.c    | 12 +++++-------
>  block/qcow2.h             |  6 +++---
>  block/rbd.c               | 15 ++++++---------
>  block/sheepdog.c          |  9 ++++-----
>  include/block/block.h     |  6 +++---
>  include/block/block_int.h |  6 +++---
>  qemu-img.c                |  9 ++++-----
>  savevm.c                  |  4 ++--
>  9 files changed, 40 insertions(+), 51 deletions(-)
>
> diff --git a/block.c b/block.c
> index 17231d2..d009ce7 100644
> --- a/block.c
> +++ b/block.c
> @@ -3304,27 +3304,23 @@ BlockDriverState *bdrv_snapshots(void)
>      return NULL;
>  }
>  
> -int bdrv_snapshot_create(BlockDriverState *bs,
> -                         QEMUSnapshotInfo *sn_info,
> -                         Error **errp)
> +void bdrv_snapshot_create(BlockDriverState *bs,
> +                          QEMUSnapshotInfo *sn_info,
> +                          Error **errp)
>  {
>      BlockDriver *drv = bs->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, errp);
> -    }
> -    if (bs->file) {
> -        return bdrv_snapshot_create(bs->file, sn_info, errp);
> +    } else if (drv->bdrv_snapshot_create) {
> +        drv->bdrv_snapshot_create(bs, sn_info, errp);
> +    } else if (bs->file) {
> +        bdrv_snapshot_create(bs->file, sn_info, errp);
> +    } else {
> +        error_setg(errp, "snapshot is not supported for '%s'",
> +                   bdrv_get_format_name(bs));
>      }
> -
> -    error_setg(errp, "snapshot is not supported for '%s'",
> -               bdrv_get_format_name(bs));
> -    return -ENOTSUP;
>  }
>  
>  int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index b34179e..f49b95f 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -315,9 +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,
> -                          Error **errp)
> +void qcow2_snapshot_create(BlockDriverState *bs,
> +                           QEMUSnapshotInfo *sn_info,
> +                           Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot *new_snapshot_list = NULL;
> @@ -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;
>      }
>  
>      /* Populate sn with passed data */
> @@ -413,14 +413,12 @@ int qcow2_snapshot_create(BlockDriverState *bs,
>        qcow2_check_refcounts(bs, &result, 0);
>      }
>  #endif
> -    return 0;
> +    return;
>  
>  fail:
>      g_free(sn->id_str);
>      g_free(sn->name);
>      g_free(l1_table);
> -
> -    return ret;
>  }

One of the assignment to ret is now redundant: ret = l1_table_offset.

The variable could be eliminated entirely, but that's a matter of taste.
Some people prefer "ret = foo(); if (ret < 0)", others "if (foo() < 0)".

Actually, some of the error_setg()s should perhaps be
error_setg_errno()s.  Then we'd still need ret.

>  
>  /* copy the snapshot 'snapshot_name' into the current disk image */
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 9d93b83..d467478 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -382,9 +382,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,
> -                          Error **errp);
> +void 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 cdbee18..5167f2c 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -815,16 +815,16 @@ static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
>      return 0;
>  }
>  
> -static int qemu_rbd_snap_create(BlockDriverState *bs,
> -                                QEMUSnapshotInfo *sn_info,
> -                                Error **errp)
> +static void qemu_rbd_snap_create(BlockDriverState *bs,
> +                                 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 */
> +        return; /* we need a name for rbd snapshots */
>      }
>  
>      /*
> @@ -834,21 +834,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;
>      }
>  
>      if (strlen(sn_info->name) >= sizeof(sn_info->id_str)) {
>          error_setg(errp, "parameter 'name' has to be shorter than 127 chars");
> -        return -ERANGE;
> +        return;
>      }
>  
>      r = rbd_snap_create(s->image, sn_info->name);
>      if (r < 0) {
>          error_setg_errno(errp, -r, "failed to create snapshot");
> -        return r;
>      }
> -
> -    return 0;
>  }
>  
>  static int qemu_rbd_snap_remove(BlockDriverState *bs,
> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index e38bdf5..cf11dfb 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1767,9 +1767,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,
> -                              Error **errp)
> +static void sd_snapshot_create(BlockDriverState *bs,
> +                               QEMUSnapshotInfo *sn_info,
> +                               Error **errp)
>  {
>      BDRVSheepdogState *s = bs->opaque;
>      int ret, fd;
> @@ -1784,7 +1784,7 @@ static int sd_snapshot_create(BlockDriverState *bs,
>      if (s->is_snapshot) {
>          error_setg(errp, "you can't create a snapshot '%s' of a snapshot VDI "
>                     "%s (%" PRIu32 ")", sn_info->name, s->name, s->inode.vdi_id);
> -        return -EINVAL;
> +        return;
>      }
>  
>      dprintf("%s %s\n", sn_info->name, sn_info->id_str);
> @@ -1836,7 +1836,6 @@ static int sd_snapshot_create(BlockDriverState *bs,
>  
>  cleanup:
>      closesocket(fd);
> -    return ret;
>  }
>  

Same here.

>  static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id)
> diff --git a/include/block/block.h b/include/block/block.h
> index ee9399c..1a6a6a7 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -331,9 +331,9 @@ BlockStats *bdrv_query_stats(const BlockDriverState *bs);
>  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,
> -                         Error **errp);
> +void bdrv_snapshot_create(BlockDriverState *bs,
> +                          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 2feaa16..8760517 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -153,9 +153,9 @@ struct BlockDriver {
>      int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>                                   const uint8_t *buf, int nb_sectors);
>  
> -    int (*bdrv_snapshot_create)(BlockDriverState *bs,
> -                                QEMUSnapshotInfo *sn_info,
> -                                Error **errp);
> +    void (*bdrv_snapshot_create)(BlockDriverState *bs,
> +                                 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 d5f81cc..154d913 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1942,6 +1942,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 */
> @@ -2018,11 +2019,9 @@ 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);
> -        if (ret) {
> -            error_report("Could not create snapshot '%s': %d (%s)",
> -                snapshot_name, ret, strerror(-ret));
> -        }
> +        local_err = NULL;
> +        bdrv_snapshot_create(bs, &sn, &local_err);
> +        ret = qemu_img_handle_error(local_err);
>          break;
>  
>      case SNAPSHOT_APPLY:

Error message changes, hopefully for the better.  Should be mentioned in
commit message.

> diff --git a/savevm.c b/savevm.c
> index 7b127eb..6ac4ece 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2342,8 +2342,8 @@ SnapshotInfo *qmp_vm_snapshot_save(bool has_name, const char *name,
>          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, &local_err);
> -            if (ret < 0) {
> +            bdrv_snapshot_create(bs1, sn, &local_err);
> +            if (error_is_set(&local_err)) {
>                  error_propagate(errp, local_err);
>              }
>          }

I think this changes HMP error messages and QMP error objects, hopefully
for the better.  Should be mentioned in commit message.

[Qemu-devel] [PATCH v4 10/11] savevm: update return value from qemu_savevm_state

[Pavel Hrdina]

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 savevm.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/savevm.c b/savevm.c
index 6ac4ece..75f64d1 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1910,7 +1910,7 @@ void qemu_savevm_state_cancel(void)
     }
 }
 
-static int qemu_savevm_state(QEMUFile *f, Error **errp)
+static void qemu_savevm_state(QEMUFile *f, Error **errp)
 {
     int ret;
     MigrationParams params = {
@@ -1919,7 +1919,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
     };
 
     if (qemu_savevm_state_blocked(errp)) {
-        return -EINVAL;
+        return;
     }
 
     qemu_mutex_unlock_iothread();
@@ -1940,7 +1940,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
     if (ret != 0) {
         qemu_savevm_state_cancel();
     }
-    return ret;
 }
 
 static int qemu_save_device_state(QEMUFile *f)
@@ -2327,10 +2326,10 @@ SnapshotInfo *qmp_vm_snapshot_save(bool has_name, const char *name,
         error_setg(errp, "failed to open '%s' file", bdrv_get_device_name(bs));
         goto the_end;
     }
-    ret = qemu_savevm_state(f, &local_err);
+    qemu_savevm_state(f, &local_err);
     vm_state_size = qemu_ftell(f);
     qemu_fclose(f);
-    if (ret < 0) {
+    if (error_is_set(&local_err)) {
         error_propagate(errp, local_err);
         goto the_end;
     }
-- 
1.8.1.4

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

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  savevm.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 6ac4ece..75f64d1 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1910,7 +1910,7 @@ void qemu_savevm_state_cancel(void)
>      }
>  }
>  
> -static int qemu_savevm_state(QEMUFile *f, Error **errp)
> +static void qemu_savevm_state(QEMUFile *f, Error **errp)
>  {
>      int ret;
>      MigrationParams params = {
> @@ -1919,7 +1919,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>      };
>  
>      if (qemu_savevm_state_blocked(errp)) {
> -        return -EINVAL;
> +        return;
>      }
>  
>      qemu_mutex_unlock_iothread();
> @@ -1940,7 +1940,6 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
       ret = qemu_file_get_error(f);
       if (ret == 0) {
           qemu_savevm_state_complete(f, errp);
           ret = qemu_file_get_error(f);
       }
>      if (ret != 0) {
>          qemu_savevm_state_cancel();
>      }
> -    return ret;
>  }
>  

Could be simplified to:

    if (!qemu_file_get_error(f)) {
        qemu_savevm_state_complete(f, errp);
    }
    if (qemu_file_get_error(f)) {
        qemu_savevm_state_cancel();
    }

>  static int qemu_save_device_state(QEMUFile *f)
> @@ -2327,10 +2326,10 @@ SnapshotInfo *qmp_vm_snapshot_save(bool has_name, const char *name,
>          error_setg(errp, "failed to open '%s' file", bdrv_get_device_name(bs));
>          goto the_end;
>      }
> -    ret = qemu_savevm_state(f, &local_err);
> +    qemu_savevm_state(f, &local_err);
>      vm_state_size = qemu_ftell(f);
>      qemu_fclose(f);
> -    if (ret < 0) {
> +    if (error_is_set(&local_err)) {
>          error_propagate(errp, local_err);
>          goto the_end;
>      }

[Qemu-devel] [PATCH v4 11/11] savevm: add force parameter to HMP command and return snapshot info

[Pavel Hrdina]

HMP command "savevm" now takes extra optional force parameter to specify
whether replace existing snapshot or not. It also returns information
about created snapshot.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hmp-commands.hx | 16 ++++++++--------
 hmp.c           | 18 +++++++++++++++++-
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 382b87d..9719cc0 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -307,19 +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
-@ref{vm_snapshots}.
+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}.
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index b38b6ce..151e48b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1437,10 +1437,26 @@ 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;
     SnapshotInfo *info = NULL;
 
-    info = qmp_vm_snapshot_save(!!name, name, true, true, &err);
+    info = qmp_vm_snapshot_save(!!name, name, !!force, force, &err);
+
+    if (info) {
+        char buf[256];
+        QEMUSnapshotInfo sn = {
+            .vm_state_size = info->vm_state_size,
+            .date_sec = info->date_sec,
+            .date_nsec = info->date_nsec,
+            .vm_clock_nsec = info->vm_clock_sec * 1000000000 +
+                             info->vm_clock_nsec,
+        };
+        pstrcpy(sn.id_str, sizeof(sn.id_str), info->id);
+        pstrcpy(sn.name, sizeof(sn.name), info->name);
+        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+    }
+
     qapi_free_SnapshotInfo(info);
     hmp_handle_error(mon, &err);
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v4 11/11] savevm: add force parameter to HMP command and return snapshot info

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> HMP command "savevm" now takes extra optional force parameter to specify
> whether replace existing snapshot or not. It also returns information
> about created snapshot.

It doesn't return anything, it prints :)

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  hmp-commands.hx | 16 ++++++++--------
>  hmp.c           | 18 +++++++++++++++++-
>  2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 382b87d..9719cc0 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -307,19 +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",

Humor me: semicolon instead of comma.

>          .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
> -@ref{vm_snapshots}.
> +Create a snapshot of the whole virtual machine. Parameter "name" is optional.

Huh?  There is no parameter "name".

> +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}.

Suggest "needs to be specified to replace it".

>  ETEXI
>  
>      {

Since you're touching the texinfo in this patch anyway, suggest to move
the markup fix from PATCH 07 here.  Only if you respin anyway.

> diff --git a/hmp.c b/hmp.c
> index b38b6ce..151e48b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1437,10 +1437,26 @@ 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;
>      SnapshotInfo *info = NULL;
>  
> -    info = qmp_vm_snapshot_save(!!name, name, true, true, &err);
> +    info = qmp_vm_snapshot_save(!!name, name, !!force, force, &err);
> +
> +    if (info) {
> +        char buf[256];
> +        QEMUSnapshotInfo sn = {
> +            .vm_state_size = info->vm_state_size,
> +            .date_sec = info->date_sec,
> +            .date_nsec = info->date_nsec,
> +            .vm_clock_nsec = info->vm_clock_sec * 1000000000 +
> +                             info->vm_clock_nsec,
> +        };
> +        pstrcpy(sn.id_str, sizeof(sn.id_str), info->id);
> +        pstrcpy(sn.name, sizeof(sn.name), info->name);

Need to copy because QAPI type SnapshotInfo duplicates QEMUSnapshotInfo.
Cleaning that up is outside the scope of this series.

> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));

Printing this makes lots of sense :)

Stick in a print of bdrv_snapshot_dump(..., NULL) to get a nice header,
like we do elsewhere?

> +    }
> +
>      qapi_free_SnapshotInfo(info);
>      hmp_handle_error(mon, &err);
>  }

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

[Markus Armbruster]

Pavel Hrdina <phrdina@redhat.com> writes:

> 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 last patch introduce new functionality of savevm that you cannot override
> existing snapshot without using 'force' parameter.
>
> If non-blocking behaviour of this command is required and we cannot wait
> until live snapshots will be finished, I could improve this basic command
> to be non-blocking.

I had a couple of questions, but the code looks generally solid.  The
most serious question is about the design of the QMP command.  We need
to reach consensus there before we can make further progress.

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

[Pavel Hrdina]

On 10.4.2013 10:18, Markus Armbruster wrote:
> Pavel Hrdina <phrdina@redhat.com> writes:
>
>> 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 last patch introduce new functionality of savevm that you cannot override
>> existing snapshot without using 'force' parameter.
>>
>> If non-blocking behaviour of this command is required and we cannot wait
>> until live snapshots will be finished, I could improve this basic command
>> to be non-blocking.
>
> I had a couple of questions, but the code looks generally solid.  The
> most serious question is about the design of the QMP command.  We need
> to reach consensus there before we can make further progress.
>

I agree that the savevm needs to be modified.

The first thing what we should discuss is the use of name as tak or
id. I think that the best solution should be that we will have separate
arguments of both. We will use the 'name' argument to specify the tag
of a snapshot and we will add a new argument named 'id' to specify the
id of the snapshot.
There will be some restriction how to use them.
   - If you are creating new snapshot, you could use only the 'name'
     argument because the id will be generated.
   - If you want to overwrite an existing snapshot, you could specify
     the 'id' or the 'name' argument or both of them and also you will
     have to use the 'force' argument
   - I think that the 'name' argument don't need to be mandatory for QMP
     and also for HMP if we will generate the name.
   - We always return/print information about created snapshot that
     everyone knows which snapshot has been created.

The loadvm will use also both arguments and with this we will support
snapshots without tags, because you can specify that you want load
the snapshot by 'id'

Pavel

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

[Eric Blake]

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

On 04/10/2013 04:53 AM, Pavel Hrdina wrote:
> The first thing what we should discuss is the use of name as tak or
> id. I think that the best solution should be that we will have separate
> arguments of both. We will use the 'name' argument to specify the tag
> of a snapshot and we will add a new argument named 'id' to specify the
> id of the snapshot.
> There will be some restriction how to use them.
>   - If you are creating new snapshot, you could use only the 'name'
>     argument because the id will be generated.

This works.

>   - If you want to overwrite an existing snapshot, you could specify
>     the 'id' or the 'name' argument or both of them and also you will
>     have to use the 'force' argument

But the argument made in this thread is that QMP should _not_ have a
force argument.  It should be a flat-out error in QMP to try to create a
snapshot with a conflicting name or tag; preferably with a distinct
error type.  Higher-level apps (HMP savevm -f) would try to create; if
-f is not specified, the error is good enough; if -f is specified and
that particular error is returned, then HMP calls delete and then
re-tries the create.  No 'force' argument needed at the QMP layer.

>   - I think that the 'name' argument don't need to be mandatory for QMP
>     and also for HMP if we will generate the name.

Since HMP always generates a tag, and libvirt always generates a tag, I
think that teaching 'qemu-img' enough plumbing to modify the tag of an
existing snapshot will be sufficient for the purposes of testing loadvm
on a tagless snapshot.  I'm starting to be convinced that having QMP
mandate a tag name, and leaving only qemu-img as the way to create a
tagless snapshot, makes more sense.

>   - We always return/print information about created snapshot that
>     everyone knows which snapshot has been created.

Thus, I'm leaning towards:

{ 'command': 'vm-snapshot-save',
  'data': { 'tag': 'str', '*id': 'int' },
  'returns': 'SnapshotInfo' }

Mandatory tag, optional id, no force flag.

Addition of a qemu-img snapshot subcommand, which can be used to alter
(including removing) the tag of an existing snapshot.

> 
> The loadvm will use also both arguments and with this we will support
> snapshots without tags, because you can specify that you want load
> the snapshot by 'id'

Here, I could see two different options:

{ 'command': 'vm-snapshot-load',
  'data': { '*name': 'str', '*id': 'int' } }

Optional name, optional id; with an additional semantic check that at
least one was provided, and that if both were provided that they name
the same snapshot (but JSON doesn't express this constraint).  Or:

{ 'enum': 'SnapshotLookup',
  'data': [ 'id-only', 'tag-only', 'default' ] }
{ 'command': 'vm-snapshot-load',
  'data': { 'name': 'str', '*mode': 'SnapshotLookup' } }

where mode defaults to matching 'name' against first 'id' then 'tag',
but where 'id-only' and 'tag-only' can refine the search to force 'name'
to match only one of the two identifiers.  Here, the only semantic
constraint that JSON cannot express is that 'name' must be the
stringized form of an integer for an id lookup to succeed.

-- 
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 v4 00/11] convert savevm to use qapi and introduce qmp command

[Luiz Capitulino]

On Wed, 10 Apr 2013 06:24:11 -0600
Eric Blake <eblake@redhat.com> wrote:

> >   - If you want to overwrite an existing snapshot, you could specify
> >     the 'id' or the 'name' argument or both of them and also you will
> >     have to use the 'force' argument
> 
> But the argument made in this thread is that QMP should _not_ have a
> force argument.  It should be a flat-out error in QMP to try to create a
> snapshot with a conflicting name or tag; preferably with a distinct
> error type.  Higher-level apps (HMP savevm -f) would try to create; if
> -f is not specified, the error is good enough; if -f is specified and
> that particular error is returned, then HMP calls delete and then
> re-tries the create.  No 'force' argument needed at the QMP layer.

To avoid adding a new error class, the HMP command could query for the
snapshot name and delete it if it exists before creating the snapshot.

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

[Eric Blake]

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

On 04/10/2013 06:40 AM, Luiz Capitulino wrote:
> On Wed, 10 Apr 2013 06:24:11 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
>>>   - If you want to overwrite an existing snapshot, you could specify
>>>     the 'id' or the 'name' argument or both of them and also you will
>>>     have to use the 'force' argument
>>
>> But the argument made in this thread is that QMP should _not_ have a
>> force argument.  It should be a flat-out error in QMP to try to create a
>> snapshot with a conflicting name or tag; preferably with a distinct
>> error type.  Higher-level apps (HMP savevm -f) would try to create; if
>> -f is not specified, the error is good enough; if -f is specified and
>> that particular error is returned, then HMP calls delete and then
>> re-tries the create.  No 'force' argument needed at the QMP layer.
> 
> To avoid adding a new error class, the HMP command could query for the
> snapshot name and delete it if it exists before creating the snapshot.

Atomic collision detection is nicer than having to pre-query - fewer QMP
calls in the common case.  But you're right that none of the existing
ErrorClass categories fit the idea of "creation refused because name
would collide".

-- 
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 v4 00/11] convert savevm to use qapi and introduce qmp command

[Pavel Hrdina]

On 10.4.2013 14:49, Eric Blake wrote:
> On 04/10/2013 06:40 AM, Luiz Capitulino wrote:
>> On Wed, 10 Apr 2013 06:24:11 -0600
>> Eric Blake <eblake@redhat.com> wrote:
>>
>>>>    - If you want to overwrite an existing snapshot, you could specify
>>>>      the 'id' or the 'name' argument or both of them and also you will
>>>>      have to use the 'force' argument
>>>
>>> But the argument made in this thread is that QMP should _not_ have a
>>> force argument.  It should be a flat-out error in QMP to try to create a
>>> snapshot with a conflicting name or tag; preferably with a distinct
>>> error type.  Higher-level apps (HMP savevm -f) would try to create; if
>>> -f is not specified, the error is good enough; if -f is specified and
>>> that particular error is returned, then HMP calls delete and then
>>> re-tries the create.  No 'force' argument needed at the QMP layer.
>>
>> To avoid adding a new error class, the HMP command could query for the
>> snapshot name and delete it if it exists before creating the snapshot.
>
> Atomic collision detection is nicer than having to pre-query - fewer QMP
> calls in the common case.  But you're right that none of the existing
> ErrorClass categories fit the idea of "creation refused because name
> would collide".
>

We still could have the force parameter for HMP because the QMP command 
will return an error message that the snapshot already exist. The only 
drawback will be that the error message will not contain an information 
that you could use a '-f' parameter for override.

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

[Luiz Capitulino]

On Wed, 10 Apr 2013 15:22:49 +0200
Pavel Hrdina <phrdina@redhat.com> wrote:

> On 10.4.2013 14:49, Eric Blake wrote:
> > On 04/10/2013 06:40 AM, Luiz Capitulino wrote:
> >> On Wed, 10 Apr 2013 06:24:11 -0600
> >> Eric Blake <eblake@redhat.com> wrote:
> >>
> >>>>    - If you want to overwrite an existing snapshot, you could specify
> >>>>      the 'id' or the 'name' argument or both of them and also you will
> >>>>      have to use the 'force' argument
> >>>
> >>> But the argument made in this thread is that QMP should _not_ have a
> >>> force argument.  It should be a flat-out error in QMP to try to create a
> >>> snapshot with a conflicting name or tag; preferably with a distinct
> >>> error type.  Higher-level apps (HMP savevm -f) would try to create; if
> >>> -f is not specified, the error is good enough; if -f is specified and
> >>> that particular error is returned, then HMP calls delete and then
> >>> re-tries the create.  No 'force' argument needed at the QMP layer.
> >>
> >> To avoid adding a new error class, the HMP command could query for the
> >> snapshot name and delete it if it exists before creating the snapshot.
> >
> > Atomic collision detection is nicer than having to pre-query - fewer QMP
> > calls in the common case.  But you're right that none of the existing
> > ErrorClass categories fit the idea of "creation refused because name
> > would collide".

We can add a new error class if it's important to be atomic. I don't think
this is hugely important though, first because we don't support multiple
QMP connections (it should work, but we don't advertise it as supported) and
also because I don't think we have ever designed commands with this in mind.

> We still could have the force parameter for HMP because the QMP command 
> will return an error message that the snapshot already exist. The only 
> drawback will be that the error message will not contain an information 
> that you could use a '-f' parameter for override.

You're right, but we're discussing how the HMP command could detect that
an existing snapshot should be deleted.

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

[Pavel Hrdina]

On 10.4.2013 15:32, Luiz Capitulino wrote:
> On Wed, 10 Apr 2013 15:22:49 +0200
> Pavel Hrdina <phrdina@redhat.com> wrote:
>
>> On 10.4.2013 14:49, Eric Blake wrote:
>>> On 04/10/2013 06:40 AM, Luiz Capitulino wrote:
>>>> On Wed, 10 Apr 2013 06:24:11 -0600
>>>> Eric Blake <eblake@redhat.com> wrote:
>>>>
>>>>>>     - If you want to overwrite an existing snapshot, you could specify
>>>>>>       the 'id' or the 'name' argument or both of them and also you will
>>>>>>       have to use the 'force' argument
>>>>>
>>>>> But the argument made in this thread is that QMP should _not_ have a
>>>>> force argument.  It should be a flat-out error in QMP to try to create a
>>>>> snapshot with a conflicting name or tag; preferably with a distinct
>>>>> error type.  Higher-level apps (HMP savevm -f) would try to create; if
>>>>> -f is not specified, the error is good enough; if -f is specified and
>>>>> that particular error is returned, then HMP calls delete and then
>>>>> re-tries the create.  No 'force' argument needed at the QMP layer.
>>>>
>>>> To avoid adding a new error class, the HMP command could query for the
>>>> snapshot name and delete it if it exists before creating the snapshot.
>>>
>>> Atomic collision detection is nicer than having to pre-query - fewer QMP
>>> calls in the common case.  But you're right that none of the existing
>>> ErrorClass categories fit the idea of "creation refused because name
>>> would collide".
>
> We can add a new error class if it's important to be atomic. I don't think
> this is hugely important though, first because we don't support multiple
> QMP connections (it should work, but we don't advertise it as supported) and
> also because I don't think we have ever designed commands with this in mind.
>
>> We still could have the force parameter for HMP because the QMP command
>> will return an error message that the snapshot already exist. The only
>> drawback will be that the error message will not contain an information
>> that you could use a '-f' parameter for override.
>
> You're right, but we're discussing how the HMP command could detect that
> an existing snapshot should be deleted.
>

If you will get an error about existing snapshot then you could write
"savevm -f my_name" and the code could be something like this:

     if (force) {
         qmp_vm_snapshot_delete(!!name, name, false, 0, &local_err);
         if (error_is_set(&local_err) {
             hmp_handle_error(mon, &local_err);
             return;
         }
     }

     info = qmp_vm_snapshot_save(!!name, name, &local_err);

     if (info) {
         print_info
     }

     qapi_free_SnapshotInfo(info);
     hmp_handle_error(mon, &local_err);

The only requirement is that the qmp_vm_snapshot_delete cannot set an 
error if the snapshot doesn't exist. To get this information the 
qmp_vm_snapshot_delete will return nothing or SnapshotInfo of deleted 
snapshot.

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

[Pavel Hrdina]

Here is another proposal how to handle vm snapshots:

QMP vm-snapshot-save:
     - { 'command': 'vm-snapshot-save',
         'data': { 'name': 'str' },
         'returns': 'SnapshotInfo' }
     - vm-snapshot-save returns an error if there is an existing
       snapshot with the same name
     - you cannot provide an id for a new snapshot
     - on success all information about created snapshot will be returned

QMP vm-snapshot-load
     - { 'command': 'vm-snapshot-load',
         'data': { '*name': 'str', '*id': 'int' },
         'returns': 'SnapshotInfo' }
     - one of the name or id must be provided
     - if both are provided they will match only the snapshot with the
       same name and id
     - returns SnapshotInfo only if the snapshot exists.

QMP vm-snapshot-delete:
     - { 'command': 'vm-snapshot-delete',
         'data': { '*name': 'str', '*id': 'int' },
         'returns': 'SnapshotInfo' }
     - same rules as vm-snapshot-load

HMP savevm:
     - args_type = "force:-f,name:s?",
     - if the name is not provided the HMP command will generates new
       one for QMP command
     - if there is already a snapshot with provided or generated name
       it will fails
     - there will be an optional -f parameter to force saving requested
       snapshot and it will internally use vm-snapshot-delete and then
       vm-snapshot-save
     - all information about created snapshot will be printed

HMP loadvm:
     - args_type = "name:s?,id:i?",
     - follow the same behavior as the QMP command

HMP delvm:
     - args_type = "name:s?,id:i?"
     - same rules as loadvm

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

[Eric Blake]

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

On 04/10/2013 08:05 AM, Pavel Hrdina wrote:
> Here is another proposal how to handle vm snapshots:
> 
> QMP vm-snapshot-save:
>     - { 'command': 'vm-snapshot-save',
>         'data': { 'name': 'str' },
>         'returns': 'SnapshotInfo' }
>     - vm-snapshot-save returns an error if there is an existing
>       snapshot with the same name
>     - you cannot provide an id for a new snapshot
>     - on success all information about created snapshot will be returned
> 
> QMP vm-snapshot-load
>     - { 'command': 'vm-snapshot-load',
>         'data': { '*name': 'str', '*id': 'int' },
>         'returns': 'SnapshotInfo' }
>     - one of the name or id must be provided
>     - if both are provided they will match only the snapshot with the
>       same name and id
>     - returns SnapshotInfo only if the snapshot exists.
> 
> QMP vm-snapshot-delete:
>     - { 'command': 'vm-snapshot-delete',
>         'data': { '*name': 'str', '*id': 'int' },
>         'returns': 'SnapshotInfo' }
>     - same rules as vm-snapshot-load

Missing some form of query-snapshots to list all consistent snapshots
that can be loaded or deleted (or is that another series?)

Also, while load can only take a consistent snapshot, it might make
sense to expose two levels of delete - one that deletes consistent
snapshots, and one that deletes a snapshot for a given block device
regardless of whether it is consistent across all devices in use by the VM.

> 
> HMP savevm:
>     - args_type = "force:-f,name:s?",
>     - if the name is not provided the HMP command will generates new
>       one for QMP command
>     - if there is already a snapshot with provided or generated name
>       it will fails
>     - there will be an optional -f parameter to force saving requested
>       snapshot and it will internally use vm-snapshot-delete and then
>       vm-snapshot-save
>     - all information about created snapshot will be printed
> 
> HMP loadvm:
>     - args_type = "name:s?,id:i?",
>     - follow the same behavior as the QMP command

Except that with HMP, arguments are positional.  You can't provide 'id'
in isolation; by listing id:i? second, the parser requires that the
first argument encountered is a name.

I think a better interface might be:

HMP loadvm:
    - args_type = "id:-i,name:s?'

If just name is given, first do a tag lookup; if that fails do an id
lookup.  If the -i flag is given, skip the tag lookup just do an id lookup.

> 
> HMP delvm:
>     - args_type = "name:s?,id:i?"
>     - same rules as loadvm

Same comment as for loadvm.

-- 
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 v4 00/11] convert savevm to use qapi and introduce qmp command

[Pavel Hrdina]

On 10.4.2013 19:15, Eric Blake wrote:
> On 04/10/2013 08:05 AM, Pavel Hrdina wrote:
>> Here is another proposal how to handle vm snapshots:
>>
>> QMP vm-snapshot-save:
>>      - { 'command': 'vm-snapshot-save',
>>          'data': { 'name': 'str' },
>>          'returns': 'SnapshotInfo' }
>>      - vm-snapshot-save returns an error if there is an existing
>>        snapshot with the same name
>>      - you cannot provide an id for a new snapshot
>>      - on success all information about created snapshot will be returned
>>
>> QMP vm-snapshot-load
>>      - { 'command': 'vm-snapshot-load',
>>          'data': { '*name': 'str', '*id': 'int' },
>>          'returns': 'SnapshotInfo' }
>>      - one of the name or id must be provided
>>      - if both are provided they will match only the snapshot with the
>>        same name and id
>>      - returns SnapshotInfo only if the snapshot exists.
>>
>> QMP vm-snapshot-delete:
>>      - { 'command': 'vm-snapshot-delete',
>>          'data': { '*name': 'str', '*id': 'int' },
>>          'returns': 'SnapshotInfo' }
>>      - same rules as vm-snapshot-load
>
> Missing some form of query-snapshots to list all consistent snapshots
> that can be loaded or deleted (or is that another series?)

Yes, there is another series for that. 
http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg00196.html

>
> Also, while load can only take a consistent snapshot, it might make
> sense to expose two levels of delete - one that deletes consistent
> snapshots, and one that deletes a snapshot for a given block device
> regardless of whether it is consistent across all devices in use by the VM.

I think that Wenchao also works on it. He posted some patches for block 
snapshots about month ago.

>
>>
>> HMP savevm:
>>      - args_type = "force:-f,name:s?",
>>      - if the name is not provided the HMP command will generates new
>>        one for QMP command
>>      - if there is already a snapshot with provided or generated name
>>        it will fails
>>      - there will be an optional -f parameter to force saving requested
>>        snapshot and it will internally use vm-snapshot-delete and then
>>        vm-snapshot-save
>>      - all information about created snapshot will be printed
>>
>> HMP loadvm:
>>      - args_type = "name:s?,id:i?",
>>      - follow the same behavior as the QMP command
>
> Except that with HMP, arguments are positional.  You can't provide 'id'
> in isolation; by listing id:i? second, the parser requires that the
> first argument encountered is a name.
>
> I think a better interface might be:
>
> HMP loadvm:
>      - args_type = "id:-i,name:s?'
>
> If just name is given, first do a tag lookup; if that fails do an id
> lookup.  If the -i flag is given, skip the tag lookup just do an id lookup.

I didn't realize that, thanks for pointing this out.

>
>>
>> HMP delvm:
>>      - args_type = "name:s?,id:i?"
>>      - same rules as loadvm
>
> Same comment as for loadvm.
>

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

[Wenchao Xia]

于 2013-4-11 1:33, Pavel Hrdina 写道:
> On 10.4.2013 19:15, Eric Blake wrote:
>> On 04/10/2013 08:05 AM, Pavel Hrdina wrote:
>>> Here is another proposal how to handle vm snapshots:
>>>
>>> QMP vm-snapshot-save:
>>>      - { 'command': 'vm-snapshot-save',
>>>          'data': { 'name': 'str' },
>>>          'returns': 'SnapshotInfo' }
>>>      - vm-snapshot-save returns an error if there is an existing
>>>        snapshot with the same name
>>>      - you cannot provide an id for a new snapshot
>>>      - on success all information about created snapshot will be
>>> returned
>>>
>>> QMP vm-snapshot-load
>>>      - { 'command': 'vm-snapshot-load',
>>>          'data': { '*name': 'str', '*id': 'int' },
>>>          'returns': 'SnapshotInfo' }
>>>      - one of the name or id must be provided
>>>      - if both are provided they will match only the snapshot with the
>>>        same name and id
>>>      - returns SnapshotInfo only if the snapshot exists.
>>>
>>> QMP vm-snapshot-delete:
>>>      - { 'command': 'vm-snapshot-delete',
>>>          'data': { '*name': 'str', '*id': 'int' },
>>>          'returns': 'SnapshotInfo' }
>>>      - same rules as vm-snapshot-load
>>
>> Missing some form of query-snapshots to list all consistent snapshots
>> that can be loaded or deleted (or is that another series?)
>
> Yes, there is another series for that.
> http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg00196.html
>
>>
>> Also, while load can only take a consistent snapshot, it might make
>> sense to expose two levels of delete - one that deletes consistent
>> snapshots, and one that deletes a snapshot for a given block device
>> regardless of whether it is consistent across all devices in use by
>> the VM.
>
> I think that Wenchao also works on it. He posted some patches for block
> snapshots about month ago.
>
   yep, the latest version is at
http://lists.nongnu.org/archive/html/qemu-devel/2013-04/msg02525.html
which expose block'image info to management stack. Luckily the two
serial does not conflict each other in main part:),  only for
bdrv_snapshot_find() function.

>>
>>>
>>> HMP savevm:
>>>      - args_type = "force:-f,name:s?",
>>>      - if the name is not provided the HMP command will generates new
>>>        one for QMP command
>>>      - if there is already a snapshot with provided or generated name
>>>        it will fails
>>>      - there will be an optional -f parameter to force saving requested
>>>        snapshot and it will internally use vm-snapshot-delete and then
>>>        vm-snapshot-save
>>>      - all information about created snapshot will be printed
>>>
>>> HMP loadvm:
>>>      - args_type = "name:s?,id:i?",
>>>      - follow the same behavior as the QMP command
>>
>> Except that with HMP, arguments are positional.  You can't provide 'id'
>> in isolation; by listing id:i? second, the parser requires that the
>> first argument encountered is a name.
>>
>> I think a better interface might be:
>>
>> HMP loadvm:
>>      - args_type = "id:-i,name:s?'
>>
>> If just name is given, first do a tag lookup; if that fails do an id
>> lookup.  If the -i flag is given, skip the tag lookup just do an id
>> lookup.
>
> I didn't realize that, thanks for pointing this out.
>
>>
>>>
>>> HMP delvm:
>>>      - args_type = "name:s?,id:i?"
>>>      - same rules as loadvm
>>
>> Same comment as for loadvm.
>>
>
>


-- 
Best Regards

Wenchao Xia

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

[Markus Armbruster]

[Cc: Wenchao Xia because of overlap with his work]

Eric Blake <eblake@redhat.com> writes:

> On 04/10/2013 08:05 AM, Pavel Hrdina wrote:
>> Here is another proposal how to handle vm snapshots:
>> 
>> QMP vm-snapshot-save:
>>     - { 'command': 'vm-snapshot-save',
>>         'data': { 'name': 'str' },
>>         'returns': 'SnapshotInfo' }
>>     - vm-snapshot-save returns an error if there is an existing
>>       snapshot with the same name
>>     - you cannot provide an id for a new snapshot
>>     - on success all information about created snapshot will be returned
>> 
>> QMP vm-snapshot-load
>>     - { 'command': 'vm-snapshot-load',
>>         'data': { '*name': 'str', '*id': 'int' },
>>         'returns': 'SnapshotInfo' }
>>     - one of the name or id must be provided
>>     - if both are provided they will match only the snapshot with the
>>       same name and id
>>     - returns SnapshotInfo only if the snapshot exists.
>> 
>> QMP vm-snapshot-delete:
>>     - { 'command': 'vm-snapshot-delete',
>>         'data': { '*name': 'str', '*id': 'int' },
>>         'returns': 'SnapshotInfo' }
>>     - same rules as vm-snapshot-load
>
> Missing some form of query-snapshots to list all consistent snapshots
> that can be loaded or deleted (or is that another series?)
>
> Also, while load can only take a consistent snapshot, it might make
> sense to expose two levels of delete - one that deletes consistent
> snapshots, and one that deletes a snapshot for a given block device
> regardless of whether it is consistent across all devices in use by the VM.

I agree distinguishing between two levels (vm and device) make sense.

On the vm level, we consider only "consistent" snapshots, i.e. ones that
cover all devices and the VM state.  Semantics of save, load, delete and
query should all be obvious.

On the device level, query and delete make obvious sense.  Save and load
still have perfectly well-defined semantics (they apply just to the
device, and never include VM state), but may not be terribly useful in
practice.  Include them or not?  Not sure.

Wenchao Xia's work (pointed to by Pavel) deals with query on both
levels.

>> HMP savevm:
>>     - args_type = "force:-f,name:s?",
>>     - if the name is not provided the HMP command will generates new
>>       one for QMP command
>>     - if there is already a snapshot with provided or generated name
>>       it will fails
>>     - there will be an optional -f parameter to force saving requested
>>       snapshot and it will internally use vm-snapshot-delete and then
>>       vm-snapshot-save
>>     - all information about created snapshot will be printed
>> 
>> HMP loadvm:
>>     - args_type = "name:s?,id:i?",
>>     - follow the same behavior as the QMP command
>
> Except that with HMP, arguments are positional.  You can't provide 'id'
> in isolation; by listing id:i? second, the parser requires that the
> first argument encountered is a name.
>
> I think a better interface might be:
>
> HMP loadvm:
>     - args_type = "id:-i,name:s?'
>
> If just name is given, first do a tag lookup; if that fails do an id
> lookup.  If the -i flag is given, skip the tag lookup just do an id lookup.
>
>> 
>> HMP delvm:
>>     - args_type = "name:s?,id:i?"
>>     - same rules as loadvm
>
> Same comment as for loadvm.

Pavel's proposal together with Eric's corrections looks sane to me.

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

[Kevin Wolf]

Am 11.04.2013 um 11:20 hat Markus Armbruster geschrieben:
> [Cc: Wenchao Xia because of overlap with his work]
> 
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 04/10/2013 08:05 AM, Pavel Hrdina wrote:
> >> Here is another proposal how to handle vm snapshots:
> >> 
> >> QMP vm-snapshot-save:
> >>     - { 'command': 'vm-snapshot-save',
> >>         'data': { 'name': 'str' },
> >>         'returns': 'SnapshotInfo' }
> >>     - vm-snapshot-save returns an error if there is an existing
> >>       snapshot with the same name
> >>     - you cannot provide an id for a new snapshot
> >>     - on success all information about created snapshot will be returned

Would it make sense to (optionally) let the caller specify in which
image the VM state will be stored, or whether it should be stored at
all?

> >> QMP vm-snapshot-load
> >>     - { 'command': 'vm-snapshot-load',
> >>         'data': { '*name': 'str', '*id': 'int' },
> >>         'returns': 'SnapshotInfo' }
> >>     - one of the name or id must be provided
> >>     - if both are provided they will match only the snapshot with the
> >>       same name and id
> >>     - returns SnapshotInfo only if the snapshot exists.
> >> 
> >> QMP vm-snapshot-delete:
> >>     - { 'command': 'vm-snapshot-delete',
> >>         'data': { '*name': 'str', '*id': 'int' },
> >>         'returns': 'SnapshotInfo' }
> >>     - same rules as vm-snapshot-load
> >
> > Missing some form of query-snapshots to list all consistent snapshots
> > that can be loaded or deleted (or is that another series?)
> >
> > Also, while load can only take a consistent snapshot, it might make
> > sense to expose two levels of delete - one that deletes consistent
> > snapshots, and one that deletes a snapshot for a given block device
> > regardless of whether it is consistent across all devices in use by the VM.
> 
> I agree distinguishing between two levels (vm and device) make sense.
> 
> On the vm level, we consider only "consistent" snapshots, i.e. ones that
> cover all devices and the VM state.  Semantics of save, load, delete and
> query should all be obvious.
> 
> On the device level, query and delete make obvious sense.  Save and load
> still have perfectly well-defined semantics (they apply just to the
> device, and never include VM state), but may not be terribly useful in
> practice.  Include them or not?  Not sure.

We have a command for disk-only external snapshots of single disks, so
it would only be consistent to have the same for internal snapshots.

Kevin

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

[Eric Blake]

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

On 04/15/2013 06:10 AM, Kevin Wolf wrote:
> Am 11.04.2013 um 11:20 hat Markus Armbruster geschrieben:
>> [Cc: Wenchao Xia because of overlap with his work]
>>
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 04/10/2013 08:05 AM, Pavel Hrdina wrote:
>>>> Here is another proposal how to handle vm snapshots:
>>>>
>>>> QMP vm-snapshot-save:
>>>>     - { 'command': 'vm-snapshot-save',
>>>>         'data': { 'name': 'str' },
>>>>         'returns': 'SnapshotInfo' }
>>>>     - vm-snapshot-save returns an error if there is an existing
>>>>       snapshot with the same name
>>>>     - you cannot provide an id for a new snapshot
>>>>     - on success all information about created snapshot will be returned
> 
> Would it make sense to (optionally) let the caller specify in which
> image the VM state will be stored, or whether it should be stored at
> all?

Yes, please.  Libvirt already has the XML to express this, but can't
wire it up unless qemu exposes that level of control.

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