Re: [PATCH v3 13/25] migration: Move setting of QEMUFile into migration_outgoing|incoming_setup

[Fabiano Rosas <farosas@suse.de>]

Prasad Pandit <ppandit@redhat.com> writes:

> On Fri, 9 Jan 2026 at 18:12, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Centralize, on both sides of migration, the setting of the to_src_file
>> and from_dst_file QEMUFiles. This will clean up the interface with
>> channel.c and rdma.c, allowing those files to stop dealing with
>> QEMUFile themselves.
>>
>> (multifd_recv_new_channel was changed to return bool+errp for
>> convenience)
>>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/channel.c   |  9 +----
>>  migration/migration.c | 84 ++++++++++++++++++++++++++-----------------
>>  migration/migration.h |  2 ++
>>  migration/multifd.c   |  8 +++--
>>  migration/multifd.h   |  2 +-
>>  5 files changed, 61 insertions(+), 44 deletions(-)
>>
>> diff --git a/migration/channel.c b/migration/channel.c
>> index 26cb7bf059..6acce7b2a2 100644
>> --- a/migration/channel.c
>> +++ b/migration/channel.c
>> @@ -14,7 +14,6 @@
>>  #include "channel.h"
>>  #include "tls.h"
>>  #include "migration.h"
>> -#include "qemu-file.h"
>>  #include "trace.h"
>>  #include "qapi/error.h"
>>  #include "io/channel-tls.h"
>> @@ -80,14 +79,8 @@ void migration_channel_connect(MigrationState *s, QIOChannel *ioc)
>>          return;
>>      }
>>
>> -    QEMUFile *f = qemu_file_new_output(ioc);
>> -
>>      migration_ioc_register_yank(ioc);
>> -
>> -    qemu_mutex_lock(&s->qemu_file_lock);
>> -    s->to_dst_file = f;
>> -    qemu_mutex_unlock(&s->qemu_file_lock);
>> -
>> +    migration_outgoing_setup(ioc);
>>      migration_connect(s);
>>  }
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 1ea6125454..b7367eb5cf 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -930,17 +930,56 @@ out:
>>      migrate_incoming_unref_outgoing_state();
>>  }
>>
>> -/**
>> - * migration_incoming_setup: Setup incoming migration
>> - * @f: file for main migration channel
>> +static bool migration_has_main_and_multifd_channels(void);
>> +
>> +/*
>> + * Returns whether all the necessary channels to proceed with the
>> + * incoming migration have been established without error.
>>   */
>> -static void migration_incoming_setup(QEMUFile *f)
>> +bool migration_incoming_setup(QIOChannel *ioc, uint8_t channel, Error **errp)
>>  {
>>      MigrationIncomingState *mis = migration_incoming_get_current();
>> +    QEMUFile *f;
>>
>> -    assert(!mis->from_src_file);
>> -    mis->from_src_file = f;
>> -    qemu_file_set_blocking(f, false, &error_abort);
>> +    if (multifd_recv_setup(errp) != 0) {
>> +        return false;
>> +    }
>> +
>> +    switch (channel) {
>> +    case CH_MAIN:
>> +        f = qemu_file_new_input(ioc);
>> +        assert(!mis->from_src_file);
>> +        mis->from_src_file = f;
>> +        qemu_file_set_blocking(f, false, &error_abort);
>> +        break;
>> +
>> +    case CH_MULTIFD:
>> +        if (!multifd_recv_new_channel(ioc, errp)) {
>> +            return false;
>> +        }
>> +        break;
>> +
>> +    case CH_POSTCOPY:
>> +        assert(!mis->postcopy_qemufile_dst);
>> +        f = qemu_file_new_input(ioc);
>> +        postcopy_preempt_new_channel(mis, f);
>> +        return false;
>> +
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    return migration_has_main_and_multifd_channels();
>> +}
>> +
>> +void migration_outgoing_setup(QIOChannel *ioc)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +    QEMUFile *f = qemu_file_new_output(ioc);
>> +
>> +    qemu_mutex_lock(&s->qemu_file_lock);
>> +    s->to_dst_file = f;
>> +    qemu_mutex_unlock(&s->qemu_file_lock);
>>  }
>
> * Shouldn't migration_outgoing_setup() also return bool, to be
> consistent with migration_incoming_setup() above?  OR make
> migration_incoming_setup() return void.
>

That would be a change simply to make the signatures consistent. It's
not indicative (granted, as it usually is) of a consistency in
design. The reason migration_incoming_setup() returns bool is to follow
the recommendation of returning whether errp is set. Here, there's no
errp, so no need.

(before multifd_send_setup moved to the migration thread, maybe we could
still make both functions more analogous, but I'm not sure how much
benefit that would have brought)

>>  /* Returns true if recovered from a paused migration, otherwise false */
>> @@ -988,7 +1027,11 @@ void migration_incoming_process(void)
>>
>>  void migration_fd_process_incoming(QEMUFile *f)
>>  {
>> -    migration_incoming_setup(f);
>> +    MigrationIncomingState *mis = migration_incoming_get_current();
>> +
>> +    assert(!mis->from_src_file);
>> +    mis->from_src_file = f;
>> +    qemu_file_set_blocking(f, false, &error_abort);
>>      migration_incoming_process();
>>  }
>
> * Is it possible to move above mis->from_src_file = f part to
> migration_incoming_setup()?
>

Yes, patch 14.

>
>> @@ -1011,8 +1054,6 @@ static bool migration_has_main_and_multifd_channels(void)
>>  void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>>  {
>>      MigrationIncomingState *mis = migration_incoming_get_current();
>> -    Error *local_err = NULL;
>> -    QEMUFile *f;
>>      uint8_t channel;
>>      uint32_t channel_magic = 0;
>>      int ret = 0;
>> @@ -1066,28 +1107,7 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>>          channel = CH_POSTCOPY;
>>      }
>>
>> -    if (multifd_recv_setup(errp) != 0) {
>> -        return;
>> -    }
>> -
>> -    if (channel == CH_MAIN) {
>> -        f = qemu_file_new_input(ioc);
>> -        migration_incoming_setup(f);
>> -    } else if (channel == CH_MULTIFD) {
>> -        /* Multiple connections */
>> -        multifd_recv_new_channel(ioc, &local_err);
>> -        if (local_err) {
>> -            error_propagate(errp, local_err);
>> -            return;
>> -        }
>> -    } else if (channel == CH_POSTCOPY) {
>> -        assert(!mis->postcopy_qemufile_dst);
>> -        f = qemu_file_new_input(ioc);
>> -        postcopy_preempt_new_channel(mis, f);
>> -        return;
>> -    }
>> -
>> -    if (migration_has_main_and_multifd_channels()) {
>> +    if (migration_incoming_setup(ioc, channel, errp)) {
>>          migration_incoming_process();
>>      }
>>  }
>> diff --git a/migration/migration.h b/migration/migration.h
>> index d134881eaf..4dcf299719 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -530,6 +530,8 @@ void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
>>  void migration_fd_process_incoming(QEMUFile *f);
>>  void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
>>  void migration_incoming_process(void);
>> +bool migration_incoming_setup(QIOChannel *ioc, uint8_t channel, Error **errp);
>> +void migration_outgoing_setup(QIOChannel *ioc);
>>
>>  bool  migration_has_all_channels(void);
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index 3fb1a07ba9..4980ed4f04 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -1521,7 +1521,7 @@ bool multifd_recv_all_channels_created(void)
>>   * Try to receive all multifd channels to get ready for the migration.
>>   * Sets @errp when failing to receive the current channel.
>>   */
>> -void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
>> +bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
>>  {
>>      MultiFDRecvParams *p;
>>      Error *local_err = NULL;
>> @@ -1536,7 +1536,7 @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
>>                                      "failed to receive packet"
>>                                      " via multifd channel %d: ",
>>                                      qatomic_read(&multifd_recv_state->count));
>> -            return;
>> +            return false;
>>          }
>>          trace_multifd_recv_new_channel(id);
>>      } else {
>> @@ -1549,7 +1549,7 @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
>>                     id);
>>          multifd_recv_terminate_threads(error_copy(local_err));
>>          error_propagate(errp, local_err);
>> -        return;
>> +        return false;
>>      }
>>      p->c = ioc;
>>      object_ref(OBJECT(ioc));
>> @@ -1558,4 +1558,6 @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
>>      qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
>>                         QEMU_THREAD_JOINABLE);
>>      qatomic_inc(&multifd_recv_state->count);
>> +
>> +    return true;
>>  }
>> diff --git a/migration/multifd.h b/migration/multifd.h
>> index 9b6d81e7ed..89a395aef2 100644
>> --- a/migration/multifd.h
>> +++ b/migration/multifd.h
>> @@ -42,7 +42,7 @@ int multifd_recv_setup(Error **errp);
>>  void multifd_recv_cleanup(void);
>>  void multifd_recv_shutdown(void);
>>  bool multifd_recv_all_channels_created(void);
>> -void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
>> +bool multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
>>  void multifd_recv_sync_main(void);
>>  int multifd_send_sync_main(MultiFDSyncReq req);
>>  bool multifd_queue_page(RAMBlock *block, ram_addr_t offset);
>> --
>
> * Change looks okay.
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>
> Thank you.
> ---
>   - Prasad