Re: [PATCH v2 05/10] migration: Make multifd_load_setup() get an Error parameter

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> We need to change the full chain to pass the Error parameter.
> 
> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  migration/migration.c | 10 +++++-----
>  migration/migration.h |  2 +-
>  migration/ram.c       |  2 +-
>  migration/ram.h       |  2 +-
>  migration/rdma.c      |  2 +-
>  5 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 5a56bd0c91..cf6cec5fb6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -518,11 +518,11 @@ fail:
>      exit(EXIT_FAILURE);
>  }
>  
> -static void migration_incoming_setup(QEMUFile *f)
> +static void migration_incoming_setup(QEMUFile *f, Error **errp)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>  
> -    if (multifd_load_setup() != 0) {
> +    if (multifd_load_setup(errp) != 0) {
>          /* We haven't been able to create multifd threads
>             nothing better to do */

But if you're taking an errp and the load fails, don't you want to
report the error before you exit? (with an error_get_pretty or
something?)

>          exit(EXIT_FAILURE);
> @@ -572,13 +572,13 @@ static bool postcopy_try_recover(QEMUFile *f)
>      return false;
>  }
>  
> -void migration_fd_process_incoming(QEMUFile *f)
> +void migration_fd_process_incoming(QEMUFile *f, Error **errp)
>  {
>      if (postcopy_try_recover(f)) {
>          return;
>      }
>  
> -    migration_incoming_setup(f);
> +    migration_incoming_setup(f, errp);
>      migration_incoming_process();

and if you're making incoming_setup able to fail, don't you need
to.... hmm, skip the incoming_process?

>  }
>  
> @@ -596,7 +596,7 @@ void migration_ioc_process_incoming(QIOChannel *ioc, 
> Error **errp)
>              return;
>          }
>  
> -        migration_incoming_setup(f);
> +        migration_incoming_setup(f, errp);

Don't you need to make that use a local_err and propagate, like in the
other half of the if/else?

>          /*
>           * Common migration only needs one channel, so we can start
> diff --git a/migration/migration.h b/migration/migration.h
> index 79b3dda146..545f283ae7 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -265,7 +265,7 @@ struct MigrationState
>  
>  void migrate_set_state(int *state, int old_state, int new_state);
>  
> -void migration_fd_process_incoming(QEMUFile *f);
> +void migration_fd_process_incoming(QEMUFile *f, Error **errp);
>  void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
>  void migration_incoming_process(void);
>  
> diff --git a/migration/ram.c b/migration/ram.c
> index 1f364cc23d..fcf50e648a 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1421,7 +1421,7 @@ static void *multifd_recv_thread(void *opaque)
>      return NULL;
>  }
>  
> -int multifd_load_setup(void)
> +int multifd_load_setup(Error **errp)
>  {
>      int thread_count;
>      uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size();
> diff --git a/migration/ram.h b/migration/ram.h
> index da22a417ea..42be471d52 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -43,7 +43,7 @@ uint64_t ram_bytes_total(void);
>  
>  int multifd_save_setup(Error **errp);
>  void multifd_save_cleanup(void);
> -int multifd_load_setup(void);
> +int multifd_load_setup(Error **errp);
>  int multifd_load_cleanup(Error **errp);
>  bool multifd_recv_all_channels_created(void);
>  bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
> diff --git a/migration/rdma.c b/migration/rdma.c
> index e241dcb992..2379b8345b 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -4004,7 +4004,7 @@ static void rdma_accept_incoming_migration(void *opaque)
>      }
>  
>      rdma->migration_started_on_destination = 1;
> -    migration_fd_process_incoming(f);
> +    migration_fd_process_incoming(f, errp);

Heck, the errp handling in rdma_accept_incoming_migration is very very
broken; I don't see how the errp ever gets reported or freed.
(But that's an existing problem)

>  }
>  
>  void rdma_start_incoming_migration(const char *host_port, Error **errp)
> -- 
> 2.23.0
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK