On Tue, Sep 12, 2023 at 04:40:14PM -0300, Fabiano Rosas wrote:
> 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.
Indeed, thanks for spotting this. Perhaps I should just drop the
error_report_err() if we've set the error already anyway.
>
>
> 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.
I'll read when it comes.
--
Peter Xu
