Peter Xu <pet...@redhat.com> writes: > migrate_set_error() used one error_copy() so it always copy an error. > However that's not the major use case - the major use case is one would > like to pass the error to migrate_set_error() without further touching the > error. > > It can be proved if we see most of the callers are freeing the error > explicitly right afterwards. There're a few outliers (only if when the > caller) where we can use error_copy() explicitly there. > > Reviewed-by: Fabiano Rosas <faro...@suse.de> > Signed-off-by: Peter Xu <pet...@redhat.com> > --- > migration/migration.h | 4 ++-- > migration/channel.c | 1 - > migration/migration.c | 22 ++++++++++++++++------ > migration/multifd.c | 10 ++++------ > migration/postcopy-ram.c | 1 - > migration/ram.c | 1 - > 6 files changed, 22 insertions(+), 17 deletions(-) > > diff --git a/migration/migration.h b/migration/migration.h > index 6eea18db36..76e35a5ecf 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -465,7 +465,7 @@ bool migration_has_all_channels(void); > > uint64_t migrate_max_downtime(void); > > -void migrate_set_error(MigrationState *s, const Error *error); > +void migrate_set_error(MigrationState *s, Error *error); > > void migrate_fd_connect(MigrationState *s, Error *error_in); > > @@ -510,7 +510,7 @@ int foreach_not_ignored_block(RAMBlockIterFunc func, void > *opaque); > void migration_make_urgent_request(void); > void migration_consume_urgent_request(void); > bool migration_rate_limit(void); > -void migration_cancel(const Error *error); > +void migration_cancel(Error *error); > > void populate_vfio_info(MigrationInfo *info); > void reset_vfio_bytes_transferred(void); > diff --git a/migration/channel.c b/migration/channel.c > index ca3319a309..48b3f6abd6 100644 > --- a/migration/channel.c > +++ b/migration/channel.c > @@ -90,7 +90,6 @@ void migration_channel_connect(MigrationState *s, > } > } > migrate_fd_connect(s, error); > - error_free(error); > } > > > diff --git a/migration/migration.c b/migration/migration.c > index c60064d48e..0f3ca168ed 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -162,7 +162,7 @@ void migration_object_init(void) > dirty_bitmap_mig_init(); > } > > -void migration_cancel(const Error *error) > +void migration_cancel(Error *error) > { > if (error) { > migrate_set_error(current_migration, error); > @@ -1218,11 +1218,22 @@ static void migrate_fd_cleanup_bh(void *opaque) > object_unref(OBJECT(s)); > } > > -void migrate_set_error(MigrationState *s, const Error *error) > +/* > + * Set error for current migration state. The `error' ownership will be > + * moved from the caller to MigrationState, so the caller doesn't need to > + * free the error. > + * > + * If the caller still needs to reference the `error' passed in, one should > + * use error_copy() explicitly. > + */ > +void migrate_set_error(MigrationState *s, Error *error) > { > QEMU_LOCK_GUARD(&s->error_mutex); > if (!s->error) { > - s->error = error_copy(error); > + /* Record the first error triggered */ > + s->error = error; > + } else { > + error_free(error);
This will conflict logically with 908927db28 ("migration: Update error description whenever migration fails") which does: + migrate_set_error(s, local_err); + error_report_err(local_err); both functions may now try to free the error. I'm working on top of this series to try to get rid of all of those qemu_file_set_error() we have. I'm trying to use migrate_set_error() whenever possible and only set f->last_error at the very bottom IO functions.