Re: [PATCH v5 2/2] migration: Update error description outside migration.c

[Juan Quintela <quintela@redhat.com>]

Tejus GK <tejus.gk@nutanix.com> wrote:
> A few code paths exist in the source code,where a migration is
> marked as failed via MIGRATION_STATUS_FAILED, but the failure happens
> outside	of migration.c
>
> In such	cases, an error_report() call is made, however the current
> MigrationState is never	updated	with the error description, and	hence
> clients	like libvirt never know	the actual reason for the failure.
>
> This patch covers such cases outside of	migration.c and	updates	the
> error description at the appropriate places.
>
> Acked-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Tejus GK <tejus.gk@nutanix.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Queued.

But I wonder.

> index 1f65294bf4..60eec7c31f 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -979,6 +979,8 @@ static void save_section_footer(QEMUFile *f, SaveStateEntry *se)
>  static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
>  {
>      int ret;
> +    Error *local_err = NULL;
> +    MigrationState *s = migrate_get_current();
>  
>      if ((!se->ops || !se->ops->save_state) && !se->vmsd) {
>          return 0;
> @@ -1002,6 +1004,8 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
>      } else {
>          ret = vmstate_save_state_with_err(f, se->vmsd, se->opaque, vmdesc, &local_err);
>          if (ret) {
> +            migrate_set_error(s, local_err);
> +            error_report_err(local_err);

We are setting the error and reporting it.

>              return ret;
>          }
>      }
> @@ -1068,10 +1072,14 @@ void qemu_savevm_send_open_return_path(QEMUFile *f)
>  int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len)
>  {
>      uint32_t tmp;
> +    MigrationState *ms = migrate_get_current();
> +    Error *local_err = NULL;
>  
>      if (len > MAX_VM_CMD_PACKAGED_SIZE) {
> -        error_report("%s: Unreasonably large packaged state: %zu",
> +        error_setg(&local_err, "%s: Unreasonably large packaged state: %zu",
>                       __func__, len);
> +        migrate_set_error(ms, local_err);
> +        error_report_err(local_err);

Again we set the error and we report it.

>          return -1;
>      }
>  
> @@ -1499,8 +1507,11 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>           * bdrv_activate_all() on the other end won't fail. */
>          ret = bdrv_inactivate_all();
>          if (ret) {
> -            error_report("%s: bdrv_inactivate_all() failed (%d)",
> -                         __func__, ret);
> +            Error *local_err = NULL;
> +            error_setg(&local_err, "%s: bdrv_inactivate_all() failed (%d)",
> +                       __func__, ret);
> +            migrate_set_error(ms, local_err);
> +            error_report_err(local_err);

Again.

>              qemu_file_set_error(f, ret);

And we still have qemu_file_set_error() here, ouch.

>              return ret;
>          }
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index dd9c76dbeb..4cde30bf2d 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -14,6 +14,7 @@
>  #include "migration.h"
>  #include "migration/vmstate.h"
>  #include "savevm.h"
> +#include "qapi/error.h"
>  #include "qapi/qmp/json-writer.h"
>  #include "qemu-file.h"
>  #include "qemu/bitops.h"
> @@ -336,7 +337,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>          ret = vmsd->pre_save(opaque);
>          trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
>          if (ret) {
> -            error_report("pre-save failed: %s", vmsd->name);
> +            error_setg(errp, "pre-save failed: %s", vmsd->name);

Here we only set the error

>              return ret;
>          }
>      }
> @@ -389,8 +390,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                                       vmdesc_loop);
>                  }
>                  if (ret) {
> -                    error_report("Save of field %s/%s failed",
> -                                 vmsd->name, field->name);
> +                    error_setg(errp, "Save of field %s/%s failed",
> +                                vmsd->name, field->name);

Same here.

>                      if (vmsd->post_save) {
>                          vmsd->post_save(opaque);
>                      }


So, I am wondering if it could be better to just report the error in a
single place for migration, and set it whenever we need it?

That is independent of this patch, though.

Later, Juan.