| Message ID | 20240207133347.1115903-3-clg@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Improve error reporting | expand |
On 7/2/24 14:33, Cédric Le Goater wrote: > This will be useful to report errors at a higher level, mostly in VFIO > today. > > Signed-off-by: Cédric Le Goater <clg@redhat.com> > --- > include/migration/register.h | 2 +- > hw/vfio/migration.c | 2 +- > migration/ram.c | 2 +- > migration/savevm.c | 10 ++++++---- > 4 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/include/migration/register.h b/include/migration/register.h > index 831600a00eae4efd0464b60925d65de4d9dbcff8..e6bc226c98b27c1fb0f9e2b56d8aff491aa14d65 100644 > --- a/include/migration/register.h > +++ b/include/migration/register.h > @@ -72,7 +72,7 @@ typedef struct SaveVMHandlers { > void (*state_pending_exact)(void *opaque, uint64_t *must_precopy, > uint64_t *can_postcopy); > LoadStateHandler *load_state; > - int (*load_setup)(QEMUFile *f, void *opaque); > + int (*load_setup)(QEMUFile *f, void *opaque, Error **errp); Please document this prototype. Otherwise: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On Wed, Feb 07, 2024 at 02:33:35PM +0100, Cédric Le Goater wrote: > diff --git a/migration/ram.c b/migration/ram.c > index 136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void) > * @f: QEMUFile where to receive the data > * @opaque: RAMState pointer Another one may need touch up.. > */ > -static int ram_load_setup(QEMUFile *f, void *opaque) > +static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp) > { > xbzrle_load_setup(); > ramblock_recv_map_init(); > diff --git a/migration/savevm.c b/migration/savevm.c > index f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -2737,7 +2737,7 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis) > trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num); > } > > -static int qemu_loadvm_state_setup(QEMUFile *f) > +static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp) > { > SaveStateEntry *se; > int ret; > @@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f) > } > } > > - ret = se->ops->load_setup(f, se->opaque); > + ret = se->ops->load_setup(f, se->opaque, errp); > if (ret < 0) { > + error_prepend(errp, "Load state of device %s failed: ", > + se->idstr); > qemu_file_set_error(f, ret); Do we also want to switch to _set_error_obj()? Or even use migrate_set_error() (the latter may apply to previous patch too if it works)? > - error_report("Load state of device %s failed", se->idstr); > return ret; > } > } > @@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f) > return ret; > } > > - if (qemu_loadvm_state_setup(f) != 0) { > + if (qemu_loadvm_state_setup(f, &local_err) != 0) { > + error_report_err(local_err); > return -EINVAL; > } > > -- > 2.43.0 > >
On 2/8/24 05:30, Peter Xu wrote: > On Wed, Feb 07, 2024 at 02:33:35PM +0100, Cédric Le Goater wrote: >> diff --git a/migration/ram.c b/migration/ram.c >> index 136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906 100644 >> --- a/migration/ram.c >> +++ b/migration/ram.c >> @@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void) >> * @f: QEMUFile where to receive the data >> * @opaque: RAMState pointer > > Another one may need touch up.. > >> */ >> -static int ram_load_setup(QEMUFile *f, void *opaque) >> +static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp) >> { >> xbzrle_load_setup(); >> ramblock_recv_map_init(); >> diff --git a/migration/savevm.c b/migration/savevm.c >> index f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -2737,7 +2737,7 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis) >> trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num); >> } >> >> -static int qemu_loadvm_state_setup(QEMUFile *f) >> +static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp) >> { >> SaveStateEntry *se; >> int ret; >> @@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f) >> } >> } >> >> - ret = se->ops->load_setup(f, se->opaque); >> + ret = se->ops->load_setup(f, se->opaque, errp); >> if (ret < 0) { >> + error_prepend(errp, "Load state of device %s failed: ", >> + se->idstr); >> qemu_file_set_error(f, ret); > > Do we also want to switch to _set_error_obj()? yes. possible. > Or even use migrate_set_error() It seems so and may be even remove it completely. What we could do first is add an Errp ** argument to qemu_loadvm_state() which would improve qmp_xen_load_devices_state() and load_snapshot(). It is less obvious for process_incoming_migration_co(). > (the latter may apply to previous patch too if it works)? It seems safe to use migrate_set_error for both migration_thread() and bg_migration_thread() because migration_detect_error() is called after calling qemu_savevm_state_setup(). However, qemu_savevm_state() relies only on qemu_file_get_error() and there would be a problem there I think. Thanks, C. > >> - error_report("Load state of device %s failed", se->idstr); >> return ret; >> } >> } >> @@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f) >> return ret; >> } >> >> - if (qemu_loadvm_state_setup(f) != 0) { >> + if (qemu_loadvm_state_setup(f, &local_err) != 0) { >> + error_report_err(local_err); >> return -EINVAL; >> } >> >> -- >> 2.43.0 >> >> >
diff --git a/include/migration/register.h b/include/migration/register.h index 831600a00eae4efd0464b60925d65de4d9dbcff8..e6bc226c98b27c1fb0f9e2b56d8aff491aa14d65 100644 --- a/include/migration/register.h +++ b/include/migration/register.h @@ -72,7 +72,7 @@ typedef struct SaveVMHandlers { void (*state_pending_exact)(void *opaque, uint64_t *must_precopy, uint64_t *can_postcopy); LoadStateHandler *load_state; - int (*load_setup)(QEMUFile *f, void *opaque); + int (*load_setup)(QEMUFile *f, void *opaque, Error **errp); int (*load_cleanup)(void *opaque); /* Called when postcopy migration wants to resume from failure */ int (*resume_prepare)(MigrationState *s, void *opaque); diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 8bcb4bc73cd5ba5338e3ffa4d907d0e6bfbb9485..2dfbe671f6f45aa530c7341177bb532d8292cecd 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -580,7 +580,7 @@ static void vfio_save_state(QEMUFile *f, void *opaque) } } -static int vfio_load_setup(QEMUFile *f, void *opaque) +static int vfio_load_setup(QEMUFile *f, void *opaque, Error **errp) { VFIODevice *vbasedev = opaque; diff --git a/migration/ram.c b/migration/ram.c index 136c237f4079f68d4e578cf1c72eec2efc815bc8..8dac9bac2fe8b8c19e102c771a7ef6e976252906 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3498,7 +3498,7 @@ void colo_release_ram_cache(void) * @f: QEMUFile where to receive the data * @opaque: RAMState pointer */ -static int ram_load_setup(QEMUFile *f, void *opaque) +static int ram_load_setup(QEMUFile *f, void *opaque, Error **errp) { xbzrle_load_setup(); ramblock_recv_map_init(); diff --git a/migration/savevm.c b/migration/savevm.c index f2ae799bad13e631bccf733a34c3a8fd22e8dd48..990f4249a26d28117ee365d8b20fc5bbca0d43d6 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2737,7 +2737,7 @@ static void qemu_loadvm_state_switchover_ack_needed(MigrationIncomingState *mis) trace_loadvm_state_switchover_ack_needed(mis->switchover_ack_pending_num); } -static int qemu_loadvm_state_setup(QEMUFile *f) +static int qemu_loadvm_state_setup(QEMUFile *f, Error **errp) { SaveStateEntry *se; int ret; @@ -2753,10 +2753,11 @@ static int qemu_loadvm_state_setup(QEMUFile *f) } } - ret = se->ops->load_setup(f, se->opaque); + ret = se->ops->load_setup(f, se->opaque, errp); if (ret < 0) { + error_prepend(errp, "Load state of device %s failed: ", + se->idstr); qemu_file_set_error(f, ret); - error_report("Load state of device %s failed", se->idstr); return ret; } } @@ -2937,7 +2938,8 @@ int qemu_loadvm_state(QEMUFile *f) return ret; } - if (qemu_loadvm_state_setup(f) != 0) { + if (qemu_loadvm_state_setup(f, &local_err) != 0) { + error_report_err(local_err); return -EINVAL; }
This will be useful to report errors at a higher level, mostly in VFIO today. Signed-off-by: Cédric Le Goater <clg@redhat.com> --- include/migration/register.h | 2 +- hw/vfio/migration.c | 2 +- migration/ram.c | 2 +- migration/savevm.c | 10 ++++++---- 4 files changed, 9 insertions(+), 7 deletions(-)