Re: [PATCH v9 02/11] migration: Make save_snapshot() return bool, not 0/-1

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> 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.
> 
> Acked-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@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..0eaf1ba0b1 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 an internal snapshot.
> + * @name: name of internal 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 4a7237337e..ef7963f6c9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2729,7 +2729,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;
> @@ -2742,29 +2742,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);
>  
> @@ -2773,7 +2773,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);
>  
> @@ -2845,7 +2845,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 3a9b609e62..8e0050915d 100644
> --- a/replay/replay-debugging.c
> +++ b/replay/replay-debugging.c
> @@ -323,7 +323,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.29.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK