Re: [Qemu-devel] [PATCH v2 12/17] savevm: add error parameter to qemu_loadvm_state()

[Luiz Capitulino <lcapitulino@redhat.com>]

On Thu, 13 Dec 2012 16:40:46 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  migration.c |  8 ++++----
>  savevm.c    | 39 +++++++++++++++++++++++----------------
>  sysemu.h    |  3 ++-
>  3 files changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 3ae1db0..4c2d2d3 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -86,13 +86,13 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>  static void process_incoming_migration_co(void *opaque)
>  {
>      QEMUFile *f = opaque;
> -    int ret;
> +    Error **errp = NULL;
>  
> -    ret = qemu_loadvm_state(f);
> +    qemu_loadvm_state(f, errp);
>      qemu_set_fd_handler(qemu_get_fd(f), NULL, NULL, NULL);
>      qemu_fclose(f);
> -    if (ret < 0) {
> -        fprintf(stderr, "load of migration failed\n");
> +    if (error_is_set(errp)) {
> +        handle_error(errp);

You can just print to stderr here, as qemu is going to exit.

>          exit(0);
>      }
>      qemu_announce_self();
> diff --git a/savevm.c b/savevm.c
> index 71c7df8..2c63aad 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1962,7 +1962,8 @@ typedef struct LoadStateEntry {
>      int version_id;
>  } LoadStateEntry;
>  
> -int qemu_loadvm_state(QEMUFile *f)
> +int qemu_loadvm_state(QEMUFile *f,
> +                      Error **errp)
>  {
>      QLIST_HEAD(, LoadStateEntry) loadvm_handlers =
>          QLIST_HEAD_INITIALIZER(loadvm_handlers);
> @@ -1971,21 +1972,25 @@ int qemu_loadvm_state(QEMUFile *f)
>      unsigned int v;
>      int ret;
>  
> -    if (qemu_savevm_state_blocked(NULL)) {
> -        return -EINVAL;
> +    if (qemu_savevm_state_blocked(errp)) {
> +        return -ENOTSUP;

Why?

>      }
>  
>      v = qemu_get_be32(f);
> -    if (v != QEMU_VM_FILE_MAGIC)
> +    if (v != QEMU_VM_FILE_MAGIC) {
> +        error_setg(errp, "Unknown vmstate file magic");
>          return -EINVAL;
> +    }
>  
>      v = qemu_get_be32(f);
>      if (v == QEMU_VM_FILE_VERSION_COMPAT) {
> -        fprintf(stderr, "SaveVM v2 format is obsolete and don't work anymore\n");
> +        error_setg(errp, "Not supported.");

Will this new error message reach the terminal as fprintf() does? Also,
please keep the good error message.

>          return -ENOTSUP;
>      }
> -    if (v != QEMU_VM_FILE_VERSION)
> +    if (v != QEMU_VM_FILE_VERSION) {
> +        error_setg(errp, "Not supported.");

Please, improve the error message.

>          return -ENOTSUP;
> +    }
>  
>      while ((section_type = qemu_get_byte(f)) != QEMU_VM_EOF) {
>          uint32_t instance_id, version_id, section_id;
> @@ -2007,15 +2012,16 @@ int qemu_loadvm_state(QEMUFile *f)
>              /* Find savevm section */
>              se = find_se(idstr, instance_id);
>              if (se == NULL) {
> -                fprintf(stderr, "Unknown savevm section or instance '%s' %d\n", idstr, instance_id);
> +                error_setg(errp, "Unknown savevm section or instance '%s' %d",
> +                           idstr, instance_id);
>                  ret = -EINVAL;
>                  goto out;
>              }
>  
>              /* Validate version */
>              if (version_id > se->version_id) {
> -                fprintf(stderr, "savevm: unsupported version %d for '%s' v%d\n",
> -                        version_id, idstr, se->version_id);
> +                error_setg(errp, "savevm: unsupported version %d for '%s' v%d",
> +                           version_id, idstr, se->version_id);
>                  ret = -EINVAL;
>                  goto out;
>              }
> @@ -2030,8 +2036,7 @@ int qemu_loadvm_state(QEMUFile *f)
>  
>              ret = vmstate_load(f, le->se, le->version_id);
>              if (ret < 0) {
> -                fprintf(stderr, "qemu: warning: error while loading state for instance 0x%x of device '%s'\n",
> -                        instance_id, idstr);
> +                error_setg(errp, "Failed to load vmstate.");
>                  goto out;
>              }
>              break;
> @@ -2045,20 +2050,19 @@ int qemu_loadvm_state(QEMUFile *f)
>                  }
>              }
>              if (le == NULL) {
> -                fprintf(stderr, "Unknown savevm section %d\n", section_id);
> +                error_setg(errp, "Unknown savevm section %d", section_id);
>                  ret = -EINVAL;
>                  goto out;
>              }
>  
>              ret = vmstate_load(f, le->se, le->version_id);
>              if (ret < 0) {
> -                fprintf(stderr, "qemu: warning: error while loading state section id %d\n",
> -                        section_id);
> +                error_setg(errp, "Failed to load vmstate.");
>                  goto out;
>              }
>              break;
>          default:
> -            fprintf(stderr, "Unknown savevm section type %d\n", section_type);
> +            error_setg(errp, "Unknown savevm section type %d", section_type);
>              ret = -EINVAL;
>              goto out;
>          }
> @@ -2076,6 +2080,9 @@ out:
>  
>      if (ret == 0) {
>          ret = qemu_file_get_error(f);
> +        if (ret < 0) {
> +            error_set(errp, QERR_GENERIC_ERROR, ret);

No reason to sue QERR_ macros here.

> +        }
>      }
>  
>      return ret;
> @@ -2342,7 +2349,7 @@ int load_vmstate(const char *name)
>      }
>  
>      qemu_system_reset(VMRESET_SILENT);
> -    ret = qemu_loadvm_state(f);
> +    ret = qemu_loadvm_state(f, NULL);
>  
>      qemu_fclose(f);
>      if (ret < 0) {
> diff --git a/sysemu.h b/sysemu.h
> index 11a4560..c08f7a3 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -81,7 +81,8 @@ int qemu_savevm_state_iterate(QEMUFile *f,
>  int qemu_savevm_state_complete(QEMUFile *f,
>                                 Error **errp);
>  void qemu_savevm_state_cancel(QEMUFile *f);
> -int qemu_loadvm_state(QEMUFile *f);
> +int qemu_loadvm_state(QEMUFile *f,
> +                      Error **errp);
>  
>  /* SLIRP */
>  void do_info_slirp(Monitor *mon);