Tejus GK <tejus...@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 <pet...@redhat.com>
> Signed-off-by: Tejus GK <tejus...@nutanix.com>
Reviewed-by: Juan Quintela <quint...@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.
