[PATCH 0/3] migration: Make save/load_snapshot() return boolean

[PATCH 0/3] migration: Make save/load_snapshot() return boolean

[Philippe Mathieu-Daudé]

I had a pair of patches making save_snapshot/load_snapshot()
return a boolean (like Markus recent qdev/QOM cleanup), then
realized Daniel already has series changing migration/, so I
rebased my patches in the first part of his v6 series:
"migration: bring improved savevm/loadvm/delvm to QMP"
https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02158.html

I included patch #1/#3 from Daniel, #2 is my first patch,
and my second patch is squashed with Daniel's #3.

Daniel if you find these patches worthwhile, please consider them
while merging your series (or respining).

Regards,

Phil.

Daniel P. Berrangé (2):
  block: push error reporting into bdrv_all_*_snapshot functions
  migration: stop returning errno from load_snapshot()

Philippe Mathieu-Daudé (1):
  migration: Make save_snapshot() return bool, not 0/-1

 include/block/snapshot.h       | 14 +++----
 include/migration/snapshot.h   | 18 +++++++-
 block/monitor/block-hmp-cmds.c |  7 ++--
 block/snapshot.c               | 77 +++++++++++++++++-----------------
 migration/savevm.c             | 72 ++++++++++++-------------------
 monitor/hmp-cmds.c             |  9 +---
 replay/replay-debugging.c      |  6 +--
 replay/replay-snapshot.c       |  4 +-
 softmmu/vl.c                   |  2 +-
 tests/qemu-iotests/267.out     | 10 ++---
 10 files changed, 105 insertions(+), 114 deletions(-)

-- 
2.26.2

[PATCH 1/3] block: push error reporting into bdrv_all_*_snapshot functions

[Philippe Mathieu-Daudé]

From: Daniel P. Berrangé <berrange@redhat.com>

The bdrv_all_*_snapshot functions return a BlockDriverState pointer
for the invalid backend, which the callers then use to report an
error message. In some cases multiple callers are reporting the
same error message, but with slightly different text. In the future
there will be more error scenarios for some of these methods, which
will benefit from fine grained error message reporting. So it is
helpful to push error reporting down a level.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/block/snapshot.h       | 14 +++----
 block/monitor/block-hmp-cmds.c |  7 ++--
 block/snapshot.c               | 77 +++++++++++++++++-----------------
 migration/savevm.c             | 37 +++++-----------
 monitor/hmp-cmds.c             |  7 +---
 replay/replay-debugging.c      |  4 +-
 tests/qemu-iotests/267.out     | 10 ++---
 7 files changed, 67 insertions(+), 89 deletions(-)

diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index b0fe42993d..5cb2b696ad 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -77,17 +77,15 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
  * These functions will properly handle dataplane (take aio_context_acquire
  * when appropriate for appropriate block drivers */
 
-bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
-int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
-                             Error **errp);
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
-                           Error **errp);
-int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
+bool bdrv_all_can_snapshot(Error **errp);
+int bdrv_all_delete_snapshot(const char *name, Error **errp);
+int bdrv_all_goto_snapshot(const char *name, Error **errp);
+int bdrv_all_find_snapshot(const char *name, Error **errp);
 int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              BlockDriverState *vm_state_bs,
                              uint64_t vm_state_size,
-                             BlockDriverState **first_bad_bs);
+                             Error **errp);
 
-BlockDriverState *bdrv_all_find_vmstate_bs(void);
+BlockDriverState *bdrv_all_find_vmstate_bs(Error **errp);
 
 #endif
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index d15a2be827..548bb6b5e3 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -899,10 +899,11 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
 
     ImageEntry *image_entry, *next_ie;
     SnapshotEntry *snapshot_entry;
+    Error *err = NULL;
 
-    bs = bdrv_all_find_vmstate_bs();
+    bs = bdrv_all_find_vmstate_bs(&err);
     if (!bs) {
-        monitor_printf(mon, "No available block device supports snapshots\n");
+        error_report_err(err);
         return;
     }
     aio_context = bdrv_get_aio_context(bs);
@@ -952,7 +953,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
     total = 0;
     for (i = 0; i < nb_sns; i++) {
         SnapshotEntry *next_sn;
-        if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) {
+        if (bdrv_all_find_snapshot(sn_tab[i].name, NULL) == 0) {
             global_snapshots[total] = i;
             total++;
             QTAILQ_FOREACH(image_entry, &image_list, next) {
diff --git a/block/snapshot.c b/block/snapshot.c
index a2bf3a54eb..50e35bb9fa 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -462,14 +462,14 @@ static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
  * These functions will properly handle dataplane (take aio_context_acquire
  * when appropriate for appropriate block drivers) */
 
-bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
+bool bdrv_all_can_snapshot(Error **errp)
 {
-    bool ok = true;
     BlockDriverState *bs;
     BdrvNextIterator it;
 
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *ctx = bdrv_get_aio_context(bs);
+        bool ok;
 
         aio_context_acquire(ctx);
         if (bdrv_all_snapshots_includes_bs(bs)) {
@@ -477,26 +477,25 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
         }
         aio_context_release(ctx);
         if (!ok) {
+            error_setg(errp, "Device '%s' is writable but does not support "
+                       "snapshots", bdrv_get_device_or_node_name(bs));
             bdrv_next_cleanup(&it);
-            goto fail;
+            return false;
         }
     }
 
-fail:
-    *first_bad_bs = bs;
-    return ok;
+    return true;
 }
 
-int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
-                             Error **errp)
+int bdrv_all_delete_snapshot(const char *name, Error **errp)
 {
-    int ret = 0;
     BlockDriverState *bs;
     BdrvNextIterator it;
     QEMUSnapshotInfo sn1, *snapshot = &sn1;
 
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *ctx = bdrv_get_aio_context(bs);
+        int ret;
 
         aio_context_acquire(ctx);
         if (bdrv_all_snapshots_includes_bs(bs) &&
@@ -507,26 +506,25 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
         }
         aio_context_release(ctx);
         if (ret < 0) {
+            error_prepend(errp, "Could not delete snapshot '%s' on '%s': ",
+                          name, bdrv_get_device_or_node_name(bs));
             bdrv_next_cleanup(&it);
-            goto fail;
+            return -1;
         }
     }
 
-fail:
-    *first_bad_bs = bs;
-    return ret;
+    return 0;
 }
 
 
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
-                           Error **errp)
+int bdrv_all_goto_snapshot(const char *name, Error **errp)
 {
-    int ret = 0;
     BlockDriverState *bs;
     BdrvNextIterator it;
 
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *ctx = bdrv_get_aio_context(bs);
+        int ret;
 
         aio_context_acquire(ctx);
         if (bdrv_all_snapshots_includes_bs(bs)) {
@@ -534,75 +532,75 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
         }
         aio_context_release(ctx);
         if (ret < 0) {
+            error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
+                          name, bdrv_get_device_or_node_name(bs));
             bdrv_next_cleanup(&it);
-            goto fail;
+            return -1;
         }
     }
 
-fail:
-    *first_bad_bs = bs;
-    return ret;
+    return 0;
 }
 
-int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
+int bdrv_all_find_snapshot(const char *name, Error **errp)
 {
     QEMUSnapshotInfo sn;
-    int err = 0;
     BlockDriverState *bs;
     BdrvNextIterator it;
 
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *ctx = bdrv_get_aio_context(bs);
+        int ret;
 
         aio_context_acquire(ctx);
         if (bdrv_all_snapshots_includes_bs(bs)) {
-            err = bdrv_snapshot_find(bs, &sn, name);
+            ret = bdrv_snapshot_find(bs, &sn, name);
         }
         aio_context_release(ctx);
-        if (err < 0) {
+        if (ret < 0) {
+            error_setg(errp, "Could not find snapshot '%s' on '%s'",
+                       name, bdrv_get_device_or_node_name(bs));
             bdrv_next_cleanup(&it);
-            goto fail;
+            return -1;
         }
     }
 
-fail:
-    *first_bad_bs = bs;
-    return err;
+    return 0;
 }
 
 int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
                              BlockDriverState *vm_state_bs,
                              uint64_t vm_state_size,
-                             BlockDriverState **first_bad_bs)
+                             Error **errp)
 {
-    int err = 0;
     BlockDriverState *bs;
     BdrvNextIterator it;
 
     for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
         AioContext *ctx = bdrv_get_aio_context(bs);
+        int ret;
 
         aio_context_acquire(ctx);
         if (bs == vm_state_bs) {
             sn->vm_state_size = vm_state_size;
-            err = bdrv_snapshot_create(bs, sn);
+            ret = bdrv_snapshot_create(bs, sn);
         } else if (bdrv_all_snapshots_includes_bs(bs)) {
             sn->vm_state_size = 0;
-            err = bdrv_snapshot_create(bs, sn);
+            ret = bdrv_snapshot_create(bs, sn);
         }
         aio_context_release(ctx);
-        if (err < 0) {
+        if (ret < 0) {
+            error_setg(errp, "Could not create snapshot '%s' on '%s'",
+                       sn->name, bdrv_get_device_or_node_name(bs));
             bdrv_next_cleanup(&it);
-            goto fail;
+            return -1;
         }
     }
 
-fail:
-    *first_bad_bs = bs;
-    return err;
+    return 0;
 }
 
-BlockDriverState *bdrv_all_find_vmstate_bs(void)
+BlockDriverState *bdrv_all_find_vmstate_bs(Error **errp)
 {
     BlockDriverState *bs;
     BdrvNextIterator it;
@@ -620,5 +618,8 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
             break;
         }
     }
+    if (!bs) {
+        error_setg(errp, "No block device supports snapshots");
+    }
     return bs;
 }
diff --git a/migration/savevm.c b/migration/savevm.c
index d2e141f7b1..a52da440f4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2660,7 +2660,7 @@ int qemu_load_device_state(QEMUFile *f)
 
 int save_snapshot(const char *name, Error **errp)
 {
-    BlockDriverState *bs, *bs1;
+    BlockDriverState *bs;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
     int ret = -1, ret2;
     QEMUFile *f;
@@ -2680,25 +2680,19 @@ int save_snapshot(const char *name, Error **errp)
         return ret;
     }
 
-    if (!bdrv_all_can_snapshot(&bs)) {
-        error_setg(errp, "Device '%s' is writable but does not support "
-                   "snapshots", bdrv_get_device_or_node_name(bs));
+    if (!bdrv_all_can_snapshot(errp)) {
         return ret;
     }
 
     /* Delete old snapshots of the same name */
     if (name) {
-        ret = bdrv_all_delete_snapshot(name, &bs1, errp);
-        if (ret < 0) {
-            error_prepend(errp, "Error while deleting snapshot on device "
-                          "'%s': ", bdrv_get_device_or_node_name(bs1));
+        if (bdrv_all_delete_snapshot(name, errp) < 0) {
             return ret;
         }
     }
 
-    bs = bdrv_all_find_vmstate_bs();
+    bs = bdrv_all_find_vmstate_bs(errp);
     if (bs == NULL) {
-        error_setg(errp, "No block device can accept snapshots");
         return ret;
     }
     aio_context = bdrv_get_aio_context(bs);
@@ -2768,10 +2762,8 @@ int save_snapshot(const char *name, Error **errp)
     aio_context_release(aio_context);
     aio_context = NULL;
 
-    ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
+    ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, errp);
     if (ret < 0) {
-        error_setg(errp, "Error while creating snapshot on '%s'",
-                   bdrv_get_device_or_node_name(bs));
         goto the_end;
     }
 
@@ -2874,30 +2866,23 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
 
 int load_snapshot(const char *name, Error **errp)
 {
-    BlockDriverState *bs, *bs_vm_state;
+    BlockDriverState *bs_vm_state;
     QEMUSnapshotInfo sn;
     QEMUFile *f;
     int ret;
     AioContext *aio_context;
     MigrationIncomingState *mis = migration_incoming_get_current();
 
-    if (!bdrv_all_can_snapshot(&bs)) {
-        error_setg(errp,
-                   "Device '%s' is writable but does not support snapshots",
-                   bdrv_get_device_or_node_name(bs));
+    if (!bdrv_all_can_snapshot(errp)) {
         return -ENOTSUP;
     }
-    ret = bdrv_all_find_snapshot(name, &bs);
+    ret = bdrv_all_find_snapshot(name, errp);
     if (ret < 0) {
-        error_setg(errp,
-                   "Device '%s' does not have the requested snapshot '%s'",
-                   bdrv_get_device_or_node_name(bs), name);
         return ret;
     }
 
-    bs_vm_state = bdrv_all_find_vmstate_bs();
+    bs_vm_state = bdrv_all_find_vmstate_bs(errp);
     if (!bs_vm_state) {
-        error_setg(errp, "No block device supports snapshots");
         return -ENOTSUP;
     }
     aio_context = bdrv_get_aio_context(bs_vm_state);
@@ -2923,10 +2908,8 @@ int load_snapshot(const char *name, Error **errp)
     /* Flush all IO requests so they don't interfere with the new state.  */
     bdrv_drain_all_begin();
 
-    ret = bdrv_all_goto_snapshot(name, &bs, errp);
+    ret = bdrv_all_goto_snapshot(name, errp);
     if (ret < 0) {
-        error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
-                      name, bdrv_get_device_or_node_name(bs));
         goto err_drain;
     }
 
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index dc0de39219..14848a3381 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1139,15 +1139,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
 
 void hmp_delvm(Monitor *mon, const QDict *qdict)
 {
-    BlockDriverState *bs;
     Error *err = NULL;
     const char *name = qdict_get_str(qdict, "name");
 
-    if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
-        error_prepend(&err,
-                      "deleting snapshot on device '%s': ",
-                      bdrv_get_device_name(bs));
-    }
+    bdrv_all_delete_snapshot(name, &err);
     hmp_handle_error(mon, err);
 }
 
diff --git a/replay/replay-debugging.c b/replay/replay-debugging.c
index ee9e86daa9..8785489c02 100644
--- a/replay/replay-debugging.c
+++ b/replay/replay-debugging.c
@@ -149,7 +149,7 @@ static char *replay_find_nearest_snapshot(int64_t icount,
 
     *snapshot_icount = -1;
 
-    bs = bdrv_all_find_vmstate_bs();
+    bs = bdrv_all_find_vmstate_bs(NULL);
     if (!bs) {
         goto fail;
     }
@@ -160,7 +160,7 @@ static char *replay_find_nearest_snapshot(int64_t icount,
     aio_context_release(aio_context);
 
     for (i = 0; i < nb_sns; i++) {
-        if (bdrv_all_find_snapshot(sn_tab[i].name, &bs) == 0) {
+        if (bdrv_all_find_snapshot(sn_tab[i].name, NULL) == 0) {
             if (sn_tab[i].icount != -1ULL
                 && sn_tab[i].icount <= icount
                 && (!nearest || nearest->icount < sn_tab[i].icount)) {
diff --git a/tests/qemu-iotests/267.out b/tests/qemu-iotests/267.out
index 27471ffae8..6149029b25 100644
--- a/tests/qemu-iotests/267.out
+++ b/tests/qemu-iotests/267.out
@@ -6,9 +6,9 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 Testing:
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
-Error: No block device can accept snapshots
+Error: No block device supports snapshots
 (qemu) info snapshots
-No available block device supports snapshots
+No block device supports snapshots
 (qemu) loadvm snap0
 Error: No block device supports snapshots
 (qemu) quit
@@ -22,7 +22,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
 Error: Device 'none0' is writable but does not support snapshots
 (qemu) info snapshots
-No available block device supports snapshots
+No block device supports snapshots
 (qemu) loadvm snap0
 Error: Device 'none0' is writable but does not support snapshots
 (qemu) quit
@@ -58,7 +58,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
 Error: Device 'virtio0' is writable but does not support snapshots
 (qemu) info snapshots
-No available block device supports snapshots
+No block device supports snapshots
 (qemu) loadvm snap0
 Error: Device 'virtio0' is writable but does not support snapshots
 (qemu) quit
@@ -83,7 +83,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm snap0
 Error: Device 'file' is writable but does not support snapshots
 (qemu) info snapshots
-No available block device supports snapshots
+No block device supports snapshots
 (qemu) loadvm snap0
 Error: Device 'file' is writable but does not support snapshots
 (qemu) quit
-- 
2.26.2

Re: [PATCH 1/3] block: push error reporting into bdrv_all_*_snapshot functions

[Max Reitz]

On 08.10.20 19:48, Philippe Mathieu-Daudé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> The bdrv_all_*_snapshot functions return a BlockDriverState pointer
> for the invalid backend, which the callers then use to report an
> error message. In some cases multiple callers are reporting the
> same error message, but with slightly different text. In the future
> there will be more error scenarios for some of these methods, which
> will benefit from fine grained error message reporting. So it is
> helpful to push error reporting down a level.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/block/snapshot.h       | 14 +++----
>  block/monitor/block-hmp-cmds.c |  7 ++--
>  block/snapshot.c               | 77 +++++++++++++++++-----------------
>  migration/savevm.c             | 37 +++++-----------
>  monitor/hmp-cmds.c             |  7 +---
>  replay/replay-debugging.c      |  4 +-
>  tests/qemu-iotests/267.out     | 10 ++---
>  7 files changed, 67 insertions(+), 89 deletions(-)

Looks good overall to me, but for some reason this patch pulls in the
@ok and @ret variables from the top scope of all concerned functions
into the inner scopes of the BDS loops, and drops their initialization.
 That’s wrong, because we only call the respective snapshotting
functions on some BDSs, so the return value stays uninitialized for all
other BDSs:

> diff --git a/block/snapshot.c b/block/snapshot.c
> index a2bf3a54eb..50e35bb9fa 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -462,14 +462,14 @@ static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
>   * These functions will properly handle dataplane (take aio_context_acquire
>   * when appropriate for appropriate block drivers) */
>  
> -bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
> +bool bdrv_all_can_snapshot(Error **errp)
>  {
> -    bool ok = true;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        bool ok;

So I think @ok must be initialized.

>  
>          aio_context_acquire(ctx);
>          if (bdrv_all_snapshots_includes_bs(bs)) {
> @@ -477,26 +477,25 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
>          }
>          aio_context_release(ctx);
>          if (!ok) {
> +            error_setg(errp, "Device '%s' is writable but does not support "
> +                       "snapshots", bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return false;
>          }
>      }
>  
> -fail:
> -    *first_bad_bs = bs;
> -    return ok;
> +    return true;
>  }
>  
> -int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
> -                             Error **errp)
> +int bdrv_all_delete_snapshot(const char *name, Error **errp)
>  {
> -    int ret = 0;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>      QEMUSnapshotInfo sn1, *snapshot = &sn1;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        int ret;

Same here, @ret must be initialized.

>  
>          aio_context_acquire(ctx);
>          if (bdrv_all_snapshots_includes_bs(bs) &&
> @@ -507,26 +506,25 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
>          }
>          aio_context_release(ctx);
>          if (ret < 0) {
> +            error_prepend(errp, "Could not delete snapshot '%s' on '%s': ",
> +                          name, bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return -1;
>          }
>      }
>  
> -fail:
> -    *first_bad_bs = bs;
> -    return ret;
> +    return 0;
>  }
>  
>  
> -int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
> -                           Error **errp)
> +int bdrv_all_goto_snapshot(const char *name, Error **errp)
>  {
> -    int ret = 0;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        int ret;

And again.

>  
>          aio_context_acquire(ctx);
>          if (bdrv_all_snapshots_includes_bs(bs)) {
> @@ -534,75 +532,75 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
>          }
>          aio_context_release(ctx);
>          if (ret < 0) {
> +            error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
> +                          name, bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return -1;
>          }
>      }
>  
> -fail:
> -    *first_bad_bs = bs;
> -    return ret;
> +    return 0;
>  }
>  
> -int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +int bdrv_all_find_snapshot(const char *name, Error **errp)
>  {
>      QEMUSnapshotInfo sn;
> -    int err = 0;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        int ret;

Again.

>  
>          aio_context_acquire(ctx);
>          if (bdrv_all_snapshots_includes_bs(bs)) {
> -            err = bdrv_snapshot_find(bs, &sn, name);
> +            ret = bdrv_snapshot_find(bs, &sn, name);
>          }
>          aio_context_release(ctx);
> -        if (err < 0) {
> +        if (ret < 0) {
> +            error_setg(errp, "Could not find snapshot '%s' on '%s'",
> +                       name, bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return -1;
>          }
>      }
>  
> -fail:
> -    *first_bad_bs = bs;
> -    return err;
> +    return 0;
>  }
>  
>  int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>                               BlockDriverState *vm_state_bs,
>                               uint64_t vm_state_size,
> -                             BlockDriverState **first_bad_bs)
> +                             Error **errp)
>  {
> -    int err = 0;
>      BlockDriverState *bs;
>      BdrvNextIterator it;
>  
>      for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>          AioContext *ctx = bdrv_get_aio_context(bs);
> +        int ret;

And one final time.

Max

>          aio_context_acquire(ctx);
>          if (bs == vm_state_bs) {
>              sn->vm_state_size = vm_state_size;
> -            err = bdrv_snapshot_create(bs, sn);
> +            ret = bdrv_snapshot_create(bs, sn);
>          } else if (bdrv_all_snapshots_includes_bs(bs)) {
>              sn->vm_state_size = 0;
> -            err = bdrv_snapshot_create(bs, sn);
> +            ret = bdrv_snapshot_create(bs, sn);
>          }
>          aio_context_release(ctx);
> -        if (err < 0) {
> +        if (ret < 0) {
> +            error_setg(errp, "Could not create snapshot '%s' on '%s'",
> +                       sn->name, bdrv_get_device_or_node_name(bs));
>              bdrv_next_cleanup(&it);
> -            goto fail;
> +            return -1;
>          }
>      }

Re: [PATCH 1/3] block: push error reporting into bdrv_all_*_snapshot functions

[Philippe Mathieu-Daudé]

On 10/12/20 12:07 PM, Max Reitz wrote:
> On 08.10.20 19:48, Philippe Mathieu-Daudé wrote:
>> From: Daniel P. Berrangé <berrange@redhat.com>
>>
>> The bdrv_all_*_snapshot functions return a BlockDriverState pointer
>> for the invalid backend, which the callers then use to report an
>> error message. In some cases multiple callers are reporting the
>> same error message, but with slightly different text. In the future
>> there will be more error scenarios for some of these methods, which
>> will benefit from fine grained error message reporting. So it is
>> helpful to push error reporting down a level.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>   include/block/snapshot.h       | 14 +++----
>>   block/monitor/block-hmp-cmds.c |  7 ++--
>>   block/snapshot.c               | 77 +++++++++++++++++-----------------
>>   migration/savevm.c             | 37 +++++-----------
>>   monitor/hmp-cmds.c             |  7 +---
>>   replay/replay-debugging.c      |  4 +-
>>   tests/qemu-iotests/267.out     | 10 ++---
>>   7 files changed, 67 insertions(+), 89 deletions(-)
> 
> Looks good overall to me, but for some reason this patch pulls in the
> @ok and @ret variables from the top scope of all concerned functions
> into the inner scopes of the BDS loops, and drops their initialization.
>   That’s wrong, because we only call the respective snapshotting
> functions on some BDSs, so the return value stays uninitialized for all
> other BDSs:

Indeed, thanks for catching that.

[...]
>>   int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>>                                BlockDriverState *vm_state_bs,
>>                                uint64_t vm_state_size,
>> -                             BlockDriverState **first_bad_bs)
>> +                             Error **errp)
>>   {
>> -    int err = 0;
>>       BlockDriverState *bs;
>>       BdrvNextIterator it;
>>   
>>       for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>>           AioContext *ctx = bdrv_get_aio_context(bs);
>> +        int ret;
> 
> And one final time.
> 
> Max
> 
>>           aio_context_acquire(ctx);
>>           if (bs == vm_state_bs) {
>>               sn->vm_state_size = vm_state_size;
>> -            err = bdrv_snapshot_create(bs, sn);
>> +            ret = bdrv_snapshot_create(bs, sn);
>>           } else if (bdrv_all_snapshots_includes_bs(bs)) {
>>               sn->vm_state_size = 0;
>> -            err = bdrv_snapshot_create(bs, sn);
>> +            ret = bdrv_snapshot_create(bs, sn);

This one is not needed.

>>           }
>>           aio_context_release(ctx);
>> -        if (err < 0) {
>> +        if (ret < 0) {
>> +            error_setg(errp, "Could not create snapshot '%s' on '%s'",
>> +                       sn->name, bdrv_get_device_or_node_name(bs));
>>               bdrv_next_cleanup(&it);
>> -            goto fail;
>> +            return -1;
>>           }
>>       }
> 

Re: [PATCH 1/3] block: push error reporting into bdrv_all_*_snapshot functions

[Max Reitz]

On 12.10.20 12:16, Philippe Mathieu-Daudé wrote:
> On 10/12/20 12:07 PM, Max Reitz wrote:
>> On 08.10.20 19:48, Philippe Mathieu-Daudé wrote:
>>> From: Daniel P. Berrangé <berrange@redhat.com>
>>>
>>> The bdrv_all_*_snapshot functions return a BlockDriverState pointer
>>> for the invalid backend, which the callers then use to report an
>>> error message. In some cases multiple callers are reporting the
>>> same error message, but with slightly different text. In the future
>>> there will be more error scenarios for some of these methods, which
>>> will benefit from fine grained error message reporting. So it is
>>> helpful to push error reporting down a level.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>   include/block/snapshot.h       | 14 +++----
>>>   block/monitor/block-hmp-cmds.c |  7 ++--
>>>   block/snapshot.c               | 77 +++++++++++++++++-----------------
>>>   migration/savevm.c             | 37 +++++-----------
>>>   monitor/hmp-cmds.c             |  7 +---
>>>   replay/replay-debugging.c      |  4 +-
>>>   tests/qemu-iotests/267.out     | 10 ++---
>>>   7 files changed, 67 insertions(+), 89 deletions(-)
>>
>> Looks good overall to me, but for some reason this patch pulls in the
>> @ok and @ret variables from the top scope of all concerned functions
>> into the inner scopes of the BDS loops, and drops their initialization.
>>   That’s wrong, because we only call the respective snapshotting
>> functions on some BDSs, so the return value stays uninitialized for all
>> other BDSs:
> 
> Indeed, thanks for catching that.
> 
> [...]
>>>   int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>>>                                BlockDriverState *vm_state_bs,
>>>                                uint64_t vm_state_size,
>>> -                             BlockDriverState **first_bad_bs)
>>> +                             Error **errp)
>>>   {
>>> -    int err = 0;
>>>       BlockDriverState *bs;
>>>       BdrvNextIterator it;
>>>         for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>>>           AioContext *ctx = bdrv_get_aio_context(bs);
>>> +        int ret;
>>
>> And one final time.
>>
>> Max
>>
>>>           aio_context_acquire(ctx);
>>>           if (bs == vm_state_bs) {
>>>               sn->vm_state_size = vm_state_size;
>>> -            err = bdrv_snapshot_create(bs, sn);
>>> +            ret = bdrv_snapshot_create(bs, sn);
>>>           } else if (bdrv_all_snapshots_includes_bs(bs)) {
>>>               sn->vm_state_size = 0;
>>> -            err = bdrv_snapshot_create(bs, sn);
>>> +            ret = bdrv_snapshot_create(bs, sn);
> 
> This one is not needed.

Why not?  Is bdrv_all_snapshots_includes_bs(bs) guaranteed to be true?
(I don’t see any a plain “else” branch, or where ret would be set
outside of these two “if” blocks.)

Max

>>>           }
>>>           aio_context_release(ctx);
>>> -        if (err < 0) {
>>> +        if (ret < 0) {
>>> +            error_setg(errp, "Could not create snapshot '%s' on '%s'",
>>> +                       sn->name, bdrv_get_device_or_node_name(bs));
>>>               bdrv_next_cleanup(&it);
>>> -            goto fail;
>>> +            return -1;
>>>           }
>>>       }
>>
> 

Re: [PATCH 1/3] block: push error reporting into bdrv_all_*_snapshot functions

[Philippe Mathieu-Daudé]

On 10/12/20 1:09 PM, Max Reitz wrote:
> On 12.10.20 12:16, Philippe Mathieu-Daudé wrote:
>> On 10/12/20 12:07 PM, Max Reitz wrote:
>>> On 08.10.20 19:48, Philippe Mathieu-Daudé wrote:
>>>> From: Daniel P. Berrangé <berrange@redhat.com>
>>>>
>>>> The bdrv_all_*_snapshot functions return a BlockDriverState pointer
>>>> for the invalid backend, which the callers then use to report an
>>>> error message. In some cases multiple callers are reporting the
>>>> same error message, but with slightly different text. In the future
>>>> there will be more error scenarios for some of these methods, which
>>>> will benefit from fine grained error message reporting. So it is
>>>> helpful to push error reporting down a level.
>>>>
>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>>> ---
>>>>    include/block/snapshot.h       | 14 +++----
>>>>    block/monitor/block-hmp-cmds.c |  7 ++--
>>>>    block/snapshot.c               | 77 +++++++++++++++++-----------------
>>>>    migration/savevm.c             | 37 +++++-----------
>>>>    monitor/hmp-cmds.c             |  7 +---
>>>>    replay/replay-debugging.c      |  4 +-
>>>>    tests/qemu-iotests/267.out     | 10 ++---
>>>>    7 files changed, 67 insertions(+), 89 deletions(-)
>>>
>>> Looks good overall to me, but for some reason this patch pulls in the
>>> @ok and @ret variables from the top scope of all concerned functions
>>> into the inner scopes of the BDS loops, and drops their initialization.
>>>    That’s wrong, because we only call the respective snapshotting
>>> functions on some BDSs, so the return value stays uninitialized for all
>>> other BDSs:
>>
>> Indeed, thanks for catching that.
>>
>> [...]
>>>>    int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
>>>>                                 BlockDriverState *vm_state_bs,
>>>>                                 uint64_t vm_state_size,
>>>> -                             BlockDriverState **first_bad_bs)
>>>> +                             Error **errp)
>>>>    {
>>>> -    int err = 0;
>>>>        BlockDriverState *bs;
>>>>        BdrvNextIterator it;
>>>>          for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>>>>            AioContext *ctx = bdrv_get_aio_context(bs);
>>>> +        int ret;
>>>
>>> And one final time.
>>>
>>> Max
>>>
>>>>            aio_context_acquire(ctx);
>>>>            if (bs == vm_state_bs) {
>>>>                sn->vm_state_size = vm_state_size;
>>>> -            err = bdrv_snapshot_create(bs, sn);
>>>> +            ret = bdrv_snapshot_create(bs, sn);
>>>>            } else if (bdrv_all_snapshots_includes_bs(bs)) {
>>>>                sn->vm_state_size = 0;
>>>> -            err = bdrv_snapshot_create(bs, sn);
>>>> +            ret = bdrv_snapshot_create(bs, sn);
>>
>> This one is not needed.
> 
> Why not?  Is bdrv_all_snapshots_includes_bs(bs) guaranteed to be true?
> (I don’t see any a plain “else” branch, or where ret would be set
> outside of these two “if” blocks.)

Oops, I misread the 'else' branch, you are right again :)

> 
> Max
> 
>>>>            }
>>>>            aio_context_release(ctx);
>>>> -        if (err < 0) {
>>>> +        if (ret < 0) {
>>>> +            error_setg(errp, "Could not create snapshot '%s' on '%s'",
>>>> +                       sn->name, bdrv_get_device_or_node_name(bs));
>>>>                bdrv_next_cleanup(&it);
>>>> -            goto fail;
>>>> +            return -1;
>>>>            }
>>>>        }
>>>
>>
> 

[PATCH 2/3] migration: Make save_snapshot() return bool, not 0/-1

[Philippe Mathieu-Daudé]

Just for consistency, following the example documented since
commit e3fe3988d7 ("error: Document Error API usage rules").
Document the function.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/migration/snapshot.h |  9 ++++++++-
 migration/savevm.c           | 16 ++++++++--------
 replay/replay-debugging.c    |  2 +-
 replay/replay-snapshot.c     |  2 +-
 4 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
index c85b6ec75b..a40c34307b 100644
--- a/include/migration/snapshot.h
+++ b/include/migration/snapshot.h
@@ -15,7 +15,14 @@
 #ifndef QEMU_MIGRATION_SNAPSHOT_H
 #define QEMU_MIGRATION_SNAPSHOT_H
 
-int save_snapshot(const char *name, Error **errp);
+/**
+ * save_snapshot: Save a snapshot.
+ * @name: path to snapshot
+ * @errp: pointer to error object
+ * On success, return %true.
+ * On failure, store an error through @errp and return %false.
+ */
+bool save_snapshot(const char *name, Error **errp);
 int load_snapshot(const char *name, Error **errp);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index a52da440f4..fd2e5e8b66 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2658,7 +2658,7 @@ int qemu_load_device_state(QEMUFile *f)
     return 0;
 }
 
-int save_snapshot(const char *name, Error **errp)
+bool save_snapshot(const char *name, Error **errp)
 {
     BlockDriverState *bs;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2671,29 +2671,29 @@ int save_snapshot(const char *name, Error **errp)
     AioContext *aio_context;
 
     if (migration_is_blocked(errp)) {
-        return ret;
+        return false;
     }
 
     if (!replay_can_snapshot()) {
         error_setg(errp, "Record/replay does not allow making snapshot "
                    "right now. Try once more later.");
-        return ret;
+        return false;
     }
 
     if (!bdrv_all_can_snapshot(errp)) {
-        return ret;
+        return false;
     }
 
     /* Delete old snapshots of the same name */
     if (name) {
         if (bdrv_all_delete_snapshot(name, errp) < 0) {
-            return ret;
+            return false;
         }
     }
 
     bs = bdrv_all_find_vmstate_bs(errp);
     if (bs == NULL) {
-        return ret;
+        return false;
     }
     aio_context = bdrv_get_aio_context(bs);
 
@@ -2702,7 +2702,7 @@ int save_snapshot(const char *name, Error **errp)
     ret = global_state_store();
     if (ret) {
         error_setg(errp, "Error saving global state");
-        return ret;
+        return false;
     }
     vm_stop(RUN_STATE_SAVE_VM);
 
@@ -2779,7 +2779,7 @@ int save_snapshot(const char *name, Error **errp)
     if (saved_vm_running) {
         vm_start();
     }
-    return ret;
+    return ret == 0;
 }
 
 void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
diff --git a/replay/replay-debugging.c b/replay/replay-debugging.c
index 8785489c02..5458a73fce 100644
--- a/replay/replay-debugging.c
+++ b/replay/replay-debugging.c
@@ -327,7 +327,7 @@ void replay_gdb_attached(void)
      */
     if (replay_mode == REPLAY_MODE_PLAY
         && !replay_snapshot) {
-        if (save_snapshot("start_debugging", NULL) != 0) {
+        if (!save_snapshot("start_debugging", NULL)) {
             /* Can't create the snapshot. Continue conventional debugging. */
         }
     }
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index e26fa4c892..4f2560d156 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -77,7 +77,7 @@ void replay_vmstate_init(void)
 
     if (replay_snapshot) {
         if (replay_mode == REPLAY_MODE_RECORD) {
-            if (save_snapshot(replay_snapshot, &err) != 0) {
+            if (!save_snapshot(replay_snapshot, &err)) {
                 error_report_err(err);
                 error_report("Could not create snapshot for icount record");
                 exit(1);
-- 
2.26.2

Re: [PATCH 2/3] migration: Make save_snapshot() return bool, not 0/-1

[Pavel Dovgalyuk]

On 08.10.2020 20:48, Philippe Mathieu-Daudé wrote:
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules").
> Document the function.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Acked-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>

> ---
>   include/migration/snapshot.h |  9 ++++++++-
>   migration/savevm.c           | 16 ++++++++--------
>   replay/replay-debugging.c    |  2 +-
>   replay/replay-snapshot.c     |  2 +-
>   4 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
> index c85b6ec75b..a40c34307b 100644
> --- a/include/migration/snapshot.h
> +++ b/include/migration/snapshot.h
> @@ -15,7 +15,14 @@
>   #ifndef QEMU_MIGRATION_SNAPSHOT_H
>   #define QEMU_MIGRATION_SNAPSHOT_H
>   
> -int save_snapshot(const char *name, Error **errp);
> +/**
> + * save_snapshot: Save a snapshot.
> + * @name: path to snapshot
> + * @errp: pointer to error object
> + * On success, return %true.
> + * On failure, store an error through @errp and return %false.
> + */
> +bool save_snapshot(const char *name, Error **errp);
>   int load_snapshot(const char *name, Error **errp);
>   
>   #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index a52da440f4..fd2e5e8b66 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2658,7 +2658,7 @@ int qemu_load_device_state(QEMUFile *f)
>       return 0;
>   }
>   
> -int save_snapshot(const char *name, Error **errp)
> +bool save_snapshot(const char *name, Error **errp)
>   {
>       BlockDriverState *bs;
>       QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -2671,29 +2671,29 @@ int save_snapshot(const char *name, Error **errp)
>       AioContext *aio_context;
>   
>       if (migration_is_blocked(errp)) {
> -        return ret;
> +        return false;
>       }
>   
>       if (!replay_can_snapshot()) {
>           error_setg(errp, "Record/replay does not allow making snapshot "
>                      "right now. Try once more later.");
> -        return ret;
> +        return false;
>       }
>   
>       if (!bdrv_all_can_snapshot(errp)) {
> -        return ret;
> +        return false;
>       }
>   
>       /* Delete old snapshots of the same name */
>       if (name) {
>           if (bdrv_all_delete_snapshot(name, errp) < 0) {
> -            return ret;
> +            return false;
>           }
>       }
>   
>       bs = bdrv_all_find_vmstate_bs(errp);
>       if (bs == NULL) {
> -        return ret;
> +        return false;
>       }
>       aio_context = bdrv_get_aio_context(bs);
>   
> @@ -2702,7 +2702,7 @@ int save_snapshot(const char *name, Error **errp)
>       ret = global_state_store();
>       if (ret) {
>           error_setg(errp, "Error saving global state");
> -        return ret;
> +        return false;
>       }
>       vm_stop(RUN_STATE_SAVE_VM);
>   
> @@ -2779,7 +2779,7 @@ int save_snapshot(const char *name, Error **errp)
>       if (saved_vm_running) {
>           vm_start();
>       }
> -    return ret;
> +    return ret == 0;
>   }
>   
>   void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
> diff --git a/replay/replay-debugging.c b/replay/replay-debugging.c
> index 8785489c02..5458a73fce 100644
> --- a/replay/replay-debugging.c
> +++ b/replay/replay-debugging.c
> @@ -327,7 +327,7 @@ void replay_gdb_attached(void)
>        */
>       if (replay_mode == REPLAY_MODE_PLAY
>           && !replay_snapshot) {
> -        if (save_snapshot("start_debugging", NULL) != 0) {
> +        if (!save_snapshot("start_debugging", NULL)) {
>               /* Can't create the snapshot. Continue conventional debugging. */
>           }
>       }
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index e26fa4c892..4f2560d156 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -77,7 +77,7 @@ void replay_vmstate_init(void)
>   
>       if (replay_snapshot) {
>           if (replay_mode == REPLAY_MODE_RECORD) {
> -            if (save_snapshot(replay_snapshot, &err) != 0) {
> +            if (!save_snapshot(replay_snapshot, &err)) {
>                   error_report_err(err);
>                   error_report("Could not create snapshot for icount record");
>                   exit(1);
> 

Re: [PATCH 2/3] migration: Make save_snapshot() return bool, not 0/-1

[Philippe Mathieu-Daudé]

On 10/8/20 7:48 PM, Philippe Mathieu-Daudé wrote:
> Just for consistency, following the example documented since
> commit e3fe3988d7 ("error: Document Error API usage rules").

I meant to use this description:

   Just for consistency, following the example documented since
   commit e3fe3988d7 ("error: Document Error API usage rules"),
   return a boolean value indicating an error is set or not.

> Document the function.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   include/migration/snapshot.h |  9 ++++++++-
>   migration/savevm.c           | 16 ++++++++--------
>   replay/replay-debugging.c    |  2 +-
>   replay/replay-snapshot.c     |  2 +-
>   4 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
> index c85b6ec75b..a40c34307b 100644
> --- a/include/migration/snapshot.h
> +++ b/include/migration/snapshot.h
> @@ -15,7 +15,14 @@
>   #ifndef QEMU_MIGRATION_SNAPSHOT_H
>   #define QEMU_MIGRATION_SNAPSHOT_H
>   
> -int save_snapshot(const char *name, Error **errp);
> +/**
> + * save_snapshot: Save a snapshot.
> + * @name: path to snapshot
> + * @errp: pointer to error object
> + * On success, return %true.
> + * On failure, store an error through @errp and return %false.
> + */
> +bool save_snapshot(const char *name, Error **errp);
>   int load_snapshot(const char *name, Error **errp);
>   
>   #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index a52da440f4..fd2e5e8b66 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2658,7 +2658,7 @@ int qemu_load_device_state(QEMUFile *f)
>       return 0;
>   }
>   
> -int save_snapshot(const char *name, Error **errp)
> +bool save_snapshot(const char *name, Error **errp)
>   {
>       BlockDriverState *bs;
>       QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -2671,29 +2671,29 @@ int save_snapshot(const char *name, Error **errp)
>       AioContext *aio_context;
>   
>       if (migration_is_blocked(errp)) {
> -        return ret;
> +        return false;
>       }
>   
>       if (!replay_can_snapshot()) {
>           error_setg(errp, "Record/replay does not allow making snapshot "
>                      "right now. Try once more later.");
> -        return ret;
> +        return false;
>       }
>   
>       if (!bdrv_all_can_snapshot(errp)) {
> -        return ret;
> +        return false;
>       }
>   
>       /* Delete old snapshots of the same name */
>       if (name) {
>           if (bdrv_all_delete_snapshot(name, errp) < 0) {
> -            return ret;
> +            return false;
>           }
>       }
>   
>       bs = bdrv_all_find_vmstate_bs(errp);
>       if (bs == NULL) {
> -        return ret;
> +        return false;
>       }
>       aio_context = bdrv_get_aio_context(bs);
>   
> @@ -2702,7 +2702,7 @@ int save_snapshot(const char *name, Error **errp)
>       ret = global_state_store();
>       if (ret) {
>           error_setg(errp, "Error saving global state");
> -        return ret;
> +        return false;
>       }
>       vm_stop(RUN_STATE_SAVE_VM);
>   
> @@ -2779,7 +2779,7 @@ int save_snapshot(const char *name, Error **errp)
>       if (saved_vm_running) {
>           vm_start();
>       }
> -    return ret;
> +    return ret == 0;
>   }
>   
>   void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
> diff --git a/replay/replay-debugging.c b/replay/replay-debugging.c
> index 8785489c02..5458a73fce 100644
> --- a/replay/replay-debugging.c
> +++ b/replay/replay-debugging.c
> @@ -327,7 +327,7 @@ void replay_gdb_attached(void)
>        */
>       if (replay_mode == REPLAY_MODE_PLAY
>           && !replay_snapshot) {
> -        if (save_snapshot("start_debugging", NULL) != 0) {
> +        if (!save_snapshot("start_debugging", NULL)) {
>               /* Can't create the snapshot. Continue conventional debugging. */
>           }
>       }
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index e26fa4c892..4f2560d156 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -77,7 +77,7 @@ void replay_vmstate_init(void)
>   
>       if (replay_snapshot) {
>           if (replay_mode == REPLAY_MODE_RECORD) {
> -            if (save_snapshot(replay_snapshot, &err) != 0) {
> +            if (!save_snapshot(replay_snapshot, &err)) {
>                   error_report_err(err);
>                   error_report("Could not create snapshot for icount record");
>                   exit(1);
> 

[PATCH 3/3] migration: stop returning errno from load_snapshot()

[Philippe Mathieu-Daudé]

From: Daniel P. Berrangé <berrange@redhat.com>

None of the callers care about the errno value since there is a full
Error object populated. This gives consistency with save_snapshot()
which already just returns a boolean value.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
[PMD: Return false/true instead of -1/0, document function]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/migration/snapshot.h |  9 ++++++++-
 migration/savevm.c           | 19 +++++++++----------
 monitor/hmp-cmds.c           |  2 +-
 replay/replay-snapshot.c     |  2 +-
 softmmu/vl.c                 |  2 +-
 5 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
index a40c34307b..9bc989a6b4 100644
--- a/include/migration/snapshot.h
+++ b/include/migration/snapshot.h
@@ -23,6 +23,13 @@
  * On failure, store an error through @errp and return %false.
  */
 bool save_snapshot(const char *name, Error **errp);
-int load_snapshot(const char *name, Error **errp);
+/**
+ * save_snapshot: Load a snapshot.
+ * @name: path to snapshot
+ * @errp: pointer to error object
+ * On success, return %true.
+ * On failure, store an error through @errp and return %false.
+ */
+bool load_snapshot(const char *name, Error **errp);
 
 #endif
diff --git a/migration/savevm.c b/migration/savevm.c
index fd2e5e8b66..531bb2eca1 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2864,7 +2864,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
     migration_incoming_state_destroy();
 }
 
-int load_snapshot(const char *name, Error **errp)
+bool load_snapshot(const char *name, Error **errp)
 {
     BlockDriverState *bs_vm_state;
     QEMUSnapshotInfo sn;
@@ -2874,16 +2874,16 @@ int load_snapshot(const char *name, Error **errp)
     MigrationIncomingState *mis = migration_incoming_get_current();
 
     if (!bdrv_all_can_snapshot(errp)) {
-        return -ENOTSUP;
+        return false;
     }
     ret = bdrv_all_find_snapshot(name, errp);
     if (ret < 0) {
-        return ret;
+        return false;
     }
 
     bs_vm_state = bdrv_all_find_vmstate_bs(errp);
     if (!bs_vm_state) {
-        return -ENOTSUP;
+        return false;
     }
     aio_context = bdrv_get_aio_context(bs_vm_state);
 
@@ -2892,11 +2892,11 @@ int load_snapshot(const char *name, Error **errp)
     ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
     aio_context_release(aio_context);
     if (ret < 0) {
-        return ret;
+        return false;
     } else if (sn.vm_state_size == 0) {
         error_setg(errp, "This is a disk-only snapshot. Revert to it "
                    " offline using qemu-img");
-        return -EINVAL;
+        return false;
     }
 
     /*
@@ -2917,7 +2917,6 @@ int load_snapshot(const char *name, Error **errp)
     f = qemu_fopen_bdrv(bs_vm_state, 0);
     if (!f) {
         error_setg(errp, "Could not open VM state file");
-        ret = -EINVAL;
         goto err_drain;
     }
 
@@ -2933,14 +2932,14 @@ int load_snapshot(const char *name, Error **errp)
 
     if (ret < 0) {
         error_setg(errp, "Error %d while loading VM state", ret);
-        return ret;
+        return false;
     }
 
-    return 0;
+    return true;
 
 err_drain:
     bdrv_drain_all_end();
-    return ret;
+    return false;
 }
 
 void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 14848a3381..ff0e3df8a3 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1123,7 +1123,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
 
     vm_stop(RUN_STATE_RESTORE_VM);
 
-    if (load_snapshot(name, &err) == 0 && saved_vm_running) {
+    if (!load_snapshot(name, &err) && saved_vm_running) {
         vm_start();
     }
     hmp_handle_error(mon, err);
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index 4f2560d156..b289365937 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -83,7 +83,7 @@ void replay_vmstate_init(void)
                 exit(1);
             }
         } else if (replay_mode == REPLAY_MODE_PLAY) {
-            if (load_snapshot(replay_snapshot, &err) != 0) {
+            if (!load_snapshot(replay_snapshot, &err)) {
                 error_report_err(err);
                 error_report("Could not load snapshot for icount replay");
                 exit(1);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5a11a62f78..6eaa6b3a09 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -4478,7 +4478,7 @@ void qemu_init(int argc, char **argv, char **envp)
     register_global_state();
     if (loadvm) {
         Error *local_err = NULL;
-        if (load_snapshot(loadvm, &local_err) < 0) {
+        if (!load_snapshot(loadvm, &local_err)) {
             error_report_err(local_err);
             autostart = 0;
             exit(1);
-- 
2.26.2

Re: [PATCH 3/3] migration: stop returning errno from load_snapshot()

[Pavel Dovgalyuk]

On 08.10.2020 20:48, Philippe Mathieu-Daudé wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> None of the callers care about the errno value since there is a full
> Error object populated. This gives consistency with save_snapshot()
> which already just returns a boolean value.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> [PMD: Return false/true instead of -1/0, document function]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Acked-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>

> ---
>   include/migration/snapshot.h |  9 ++++++++-
>   migration/savevm.c           | 19 +++++++++----------
>   monitor/hmp-cmds.c           |  2 +-
>   replay/replay-snapshot.c     |  2 +-
>   softmmu/vl.c                 |  2 +-
>   5 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
> index a40c34307b..9bc989a6b4 100644
> --- a/include/migration/snapshot.h
> +++ b/include/migration/snapshot.h
> @@ -23,6 +23,13 @@
>    * On failure, store an error through @errp and return %false.
>    */
>   bool save_snapshot(const char *name, Error **errp);
> -int load_snapshot(const char *name, Error **errp);
> +/**
> + * save_snapshot: Load a snapshot.
> + * @name: path to snapshot
> + * @errp: pointer to error object
> + * On success, return %true.
> + * On failure, store an error through @errp and return %false.
> + */
> +bool load_snapshot(const char *name, Error **errp);
>   
>   #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index fd2e5e8b66..531bb2eca1 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2864,7 +2864,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
>       migration_incoming_state_destroy();
>   }
>   
> -int load_snapshot(const char *name, Error **errp)
> +bool load_snapshot(const char *name, Error **errp)
>   {
>       BlockDriverState *bs_vm_state;
>       QEMUSnapshotInfo sn;
> @@ -2874,16 +2874,16 @@ int load_snapshot(const char *name, Error **errp)
>       MigrationIncomingState *mis = migration_incoming_get_current();
>   
>       if (!bdrv_all_can_snapshot(errp)) {
> -        return -ENOTSUP;
> +        return false;
>       }
>       ret = bdrv_all_find_snapshot(name, errp);
>       if (ret < 0) {
> -        return ret;
> +        return false;
>       }
>   
>       bs_vm_state = bdrv_all_find_vmstate_bs(errp);
>       if (!bs_vm_state) {
> -        return -ENOTSUP;
> +        return false;
>       }
>       aio_context = bdrv_get_aio_context(bs_vm_state);
>   
> @@ -2892,11 +2892,11 @@ int load_snapshot(const char *name, Error **errp)
>       ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
>       aio_context_release(aio_context);
>       if (ret < 0) {
> -        return ret;
> +        return false;
>       } else if (sn.vm_state_size == 0) {
>           error_setg(errp, "This is a disk-only snapshot. Revert to it "
>                      " offline using qemu-img");
> -        return -EINVAL;
> +        return false;
>       }
>   
>       /*
> @@ -2917,7 +2917,6 @@ int load_snapshot(const char *name, Error **errp)
>       f = qemu_fopen_bdrv(bs_vm_state, 0);
>       if (!f) {
>           error_setg(errp, "Could not open VM state file");
> -        ret = -EINVAL;
>           goto err_drain;
>       }
>   
> @@ -2933,14 +2932,14 @@ int load_snapshot(const char *name, Error **errp)
>   
>       if (ret < 0) {
>           error_setg(errp, "Error %d while loading VM state", ret);
> -        return ret;
> +        return false;
>       }
>   
> -    return 0;
> +    return true;
>   
>   err_drain:
>       bdrv_drain_all_end();
> -    return ret;
> +    return false;
>   }
>   
>   void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 14848a3381..ff0e3df8a3 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1123,7 +1123,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
>   
>       vm_stop(RUN_STATE_RESTORE_VM);
>   
> -    if (load_snapshot(name, &err) == 0 && saved_vm_running) {
> +    if (!load_snapshot(name, &err) && saved_vm_running) {
>           vm_start();
>       }
>       hmp_handle_error(mon, err);
> diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
> index 4f2560d156..b289365937 100644
> --- a/replay/replay-snapshot.c
> +++ b/replay/replay-snapshot.c
> @@ -83,7 +83,7 @@ void replay_vmstate_init(void)
>                   exit(1);
>               }
>           } else if (replay_mode == REPLAY_MODE_PLAY) {
> -            if (load_snapshot(replay_snapshot, &err) != 0) {
> +            if (!load_snapshot(replay_snapshot, &err)) {
>                   error_report_err(err);
>                   error_report("Could not load snapshot for icount replay");
>                   exit(1);
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 5a11a62f78..6eaa6b3a09 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -4478,7 +4478,7 @@ void qemu_init(int argc, char **argv, char **envp)
>       register_global_state();
>       if (loadvm) {
>           Error *local_err = NULL;
> -        if (load_snapshot(loadvm, &local_err) < 0) {
> +        if (!load_snapshot(loadvm, &local_err)) {
>               error_report_err(local_err);
>               autostart = 0;
>               exit(1);
>