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

[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 qemu_loadvm_section_part_end() 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 | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 77408347c1f1ca7eb3a04f8f130c20a5a81f6db2..ff2e4f75e070d0f452414f28435905928b1480a7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2806,21 +2806,20 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
>  }
>  
>  static int
> -qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> +qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp)
>  {
> +    ERRP_GUARD();
>      bool trace_downtime = (type == QEMU_VM_SECTION_END);
>      int64_t start_ts, end_ts;
>      uint32_t section_id;
>      SaveStateEntry *se;
>      int ret;
> -    Error *local_err = NULL;
>  
>      section_id = qemu_get_be32(f);
>  
>      ret = qemu_file_get_error(f);
>      if (ret) {
> -        error_report("%s: Failed to read section ID: %d",
> -                     __func__, ret);
> +        error_setg(errp, "Failed to read section ID: %d", ret);
>          return ret;
>      }
>  
> @@ -2831,7 +2830,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>          }
>      }
>      if (se == NULL) {
> -        error_report("Unknown savevm section %d", section_id);
> +        error_setg(errp, "Unknown savevm section %d", section_id);

Drop "savevm" please.

>          return -EINVAL;
>      }
>  
> @@ -2839,11 +2838,10 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>      }
>  
> -    ret = vmstate_load(f, se, &local_err);
> +    ret = vmstate_load(f, se, errp);
>      if (ret < 0) {
> -        error_report("error while loading state section id %d(%s)",
> -                     section_id, se->idstr);
> -        warn_report_err(local_err);
> +        error_prepend(errp, "error while loading state section id %d(%s): ",
> +                      section_id, se->idstr);
>          return ret;
>      }
>  
> @@ -2854,6 +2852,8 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>      }
>  
>      if (!check_section_footer(f, se)) {
> +        error_setg(errp, "Check section footer error, section_id: %d",

This became not very grammatical, maybe drop the "Check" word.

> +                   section_id);
>          return -EINVAL;
>      }
>  
> @@ -3112,7 +3112,7 @@ retry:
>              break;
>          case QEMU_VM_SECTION_PART:
>          case QEMU_VM_SECTION_END:
> -            ret = qemu_loadvm_section_part_end(f, section_type);
> +            ret = qemu_loadvm_section_part_end(f, section_type, errp);
>              if (ret < 0) {
>                  goto out;
>              }
> @@ -3136,9 +3136,6 @@ retry:
>  
>  out:
>      if (ret < 0) {
> -        if (*errp == NULL) {
> -            error_setg(errp, "Loading VM state failed: %d", ret);
> -        }

Good. Could have been mentioned in that patch's commit message, or even
a /* temporary */ comment in the code.

>          qemu_file_set_error(f, ret);
>  
>          /* Cancel bitmaps incoming regardless of recovery */