Re: [PATCH v11 04/27] migration: push Error **errp into vmstate_load()

[Fabiano Rosas <farosas@suse.de>]

Arun Menon <armenon@redhat.com> writes:

> This is an incremental step in converting vmstate loading
> code to report error via Error objects instead of directly
> printing it to console/monitor.
> It is ensured that vmstate_load() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0c445a957fc99f826e6753ed3795bcdd51f1e3f5..7f79461844105bf672314c3325caee9cdb654c27 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -963,14 +963,20 @@ void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
>      }
>  }
>  
> -static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
> +static int vmstate_load(QEMUFile *f, SaveStateEntry *se, Error **errp)
>  {
> +    int ret;
>      trace_vmstate_load(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
>      if (!se->vmsd) {         /* Old style */
> -        return se->ops->load_state(f, se->opaque, se->load_version_id);
> +        ret = se->ops->load_state(f, se->opaque, se->load_version_id);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load VM version_id: %d, ret: %d",

"VM" is ambiguous. I'd use "vmstate".

> +                       se->load_version_id, ret);
> +        }
> +        return ret;
>      }
>      return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> -                              &error_fatal);
> +                              errp);
>  }
>  
>  static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> @@ -2692,6 +2698,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>      SaveStateEntry *se;
>      char idstr[256];
>      int ret;
> +    Error *local_err = NULL;
>  
>      /* Read section start */
>      section_id = qemu_get_be32(f);
> @@ -2741,10 +2748,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>      }
>  
> -    ret = vmstate_load(f, se);
> +    ret = vmstate_load(f, se, &local_err);
>      if (ret < 0) {
>          error_report("error while loading state for instance 0x%"PRIx32" of"
>                       " device '%s'", instance_id, idstr);
> +        warn_report_err(local_err);

I was about to ask why you're using a warning here, but I see you remove
it futher in the series. That's fine, but this kind of thing should be
mentioned in the commit message, otherwise it gets confusing to review.

>          return ret;
>      }
>  
> @@ -2769,6 +2777,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>      uint32_t section_id;
>      SaveStateEntry *se;
>      int ret;
> +    Error *local_err = NULL;
>  
>      section_id = qemu_get_be32(f);
>  
> @@ -2794,10 +2803,11 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>      }
>  
> -    ret = vmstate_load(f, se);
> +    ret = vmstate_load(f, se, &local_err);
>      if (ret < 0) {
>          error_report("error while loading state section id %d(%s)",
>                       section_id, se->idstr);
> +        warn_report_err(local_err);
>          return ret;
>      }