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

Philippe Mathieu-Daudé posted 3 patches 5 years, 1 month ago
There is a newer version of this series
[PATCH 2/3] migration: Make save_snapshot() return bool, not 0/-1
Posted by Philippe Mathieu-Daudé 5 years, 1 month ago
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
Posted by Pavel Dovgalyuk 5 years, 1 month ago
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
Posted by Philippe Mathieu-Daudé 5 years, 1 month ago
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);
>