Re: [PATCH 2/9] migration: Let migrate_set_error() take ownership

[Peter Xu <peterx@redhat.com>]

On Tue, Sep 12, 2023 at 04:40:14PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@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 <farosas@suse.de>
> > Signed-off-by: Peter Xu <peterx@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