| Message ID | 20230518062308.90631-3-tejus.gk@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Update error description whenever migration fails | expand |
Tejus GK <tejus.gk@nutanix.com> wrote: > There are places outside of migration.c which eventually leads to a > migration failure, but the failure reason is never updated. Hence > libvirt doesn't know why the migration failed when it queries for it. > > Signed-off-by: Tejus GK <tejus.gk@nutanix.com> Reviewed-by: Juan Quintela <quintela@redhat.com> If you have to respin: > @@ -1456,6 +1460,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, > int vmdesc_len; > SaveStateEntry *se; > int ret; > + Error *local_err = NULL; You can declare this: > QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { > if (se->vmsd && se->vmsd->early_setup) { > @@ -1475,8 +1480,10 @@ 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) { here > - error_report("%s: bdrv_inactivate_all() failed (%d)", > - __func__, ret);
On 18/05/23 5:22 pm, Juan Quintela wrote: > Tejus GK <tejus.gk@nutanix.com> wrote: >> There are places outside of migration.c which eventually leads to a >> migration failure, but the failure reason is never updated. Hence >> libvirt doesn't know why the migration failed when it queries for it. >> >> Signed-off-by: Tejus GK <tejus.gk@nutanix.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> Thank you for the reviews Juan, but I believe that this particular patch shouldn't be approved yet. I have mentioned it in the RFC cover letter that the changes in this patch, in the file vmstate.c, end up breaking the build for a unit-test, eventually breaking the entire build. I was not sure how to implement the error reporting properly in such cases, and the aim of this patch was to receive advice on the same. > > > If you have to respin: > >> @@ -1456,6 +1460,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, >> int vmdesc_len; >> SaveStateEntry *se; >> int ret; >> + Error *local_err = NULL; > > You can declare this: > >> QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { >> if (se->vmsd && se->vmsd->early_setup) { >> @@ -1475,8 +1480,10 @@ 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) { > > here > >> - error_report("%s: bdrv_inactivate_all() failed (%d)", >> - __func__, ret); >
diff --git a/migration/savevm.c b/migration/savevm.c index e33788343a..39d4ecdd41 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -1068,10 +1068,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); return -1; } @@ -1456,6 +1460,7 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f, int vmdesc_len; SaveStateEntry *se; int ret; + Error *local_err = NULL; QTAILQ_FOREACH(se, &savevm_state.handlers, entry) { if (se->vmsd && se->vmsd->early_setup) { @@ -1475,8 +1480,10 @@ 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_setg(&local_err, "%s: bdrv_inactivate_all() failed (%d)", + __func__, ret); + migrate_set_error(ms, local_err); + error_report_err(local_err); qemu_file_set_error(f, ret); return ret; } diff --git a/migration/vmstate.c b/migration/vmstate.c index af01d54b6f..3a5770b925 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" @@ -323,6 +324,8 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd, { int ret = 0; const VMStateField *field = vmsd->fields; + MigrationState *ms = migrate_get_current(); + Error *local_err = NULL; trace_vmstate_save_state_top(vmsd->name); @@ -330,7 +333,9 @@ 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(&local_err, "pre-save failed: %s", vmsd->name); + migrate_set_error(ms, local_err); + error_report_err(local_err); return ret; } } @@ -383,8 +388,10 @@ 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(&local_err, "Save of field %s/%s failed", + vmsd->name, field->name); + migrate_set_error(ms, local_err); + error_report_err(local_err); if (vmsd->post_save) { vmsd->post_save(opaque); }
There are places outside of migration.c which eventually leads to a migration failure, but the failure reason is never updated. Hence libvirt doesn't know why the migration failed when it queries for it. Signed-off-by: Tejus GK <tejus.gk@nutanix.com> --- migration/savevm.c | 13 ++++++++++--- migration/vmstate.c | 13 ++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-)