On Mon, Jan 05, 2026 at 04:06:30PM -0300, Fabiano Rosas 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) > > Signed-off-by: Fabiano Rosas <[email protected]>
Reviewed-by: Peter Xu <[email protected]> Some nitpicks inline. > --- > migration/channel.c | 9 +---- > migration/migration.c | 82 ++++++++++++++++++++++++++----------------- > migration/migration.h | 2 ++ > migration/multifd.c | 8 +++-- > migration/multifd.h | 2 +- > 5 files changed, 58 insertions(+), 45 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..dc8fe80cf8 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -930,17 +930,52 @@ out: > migrate_incoming_unref_outgoing_state(); > } > > -/** > - * migration_incoming_setup: Setup incoming migration > - * @f: file for main migration channel > - */ > -static void migration_incoming_setup(QEMUFile *f) > +static bool migration_has_main_and_multifd_channels(void); > + > +bool migration_incoming_setup(QIOChannel *ioc, uint8_t channel, Error **errp) Considering how widely used is "bool+errp" pattern across QEMU (but with different semantics), maybe we should add a comment to the retval here saying the bool is not reflecting "whether errp will be set", but "whether we should proceed with processing the incoming migration". > { > 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); > } > > /* Returns true if recovered from a paused migration, otherwise false */ > @@ -988,7 +1023,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); These are only a few lines of straightforward code, I think it's fine to open code them here. Said that, IMHO it's always one step back if your new patch will open-code some function more than once, when we used to have a helper for that. IMHO we can rename the old migration_incoming_setup() to migration_incoming_setup_main(), and use it in both places. > migration_incoming_process(); > } > > @@ -1011,8 +1050,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 +1103,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); > -- > 2.51.0 > -- Peter Xu
