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

[Fabiano Rosas <farosas@suse.de>]

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.


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.