On Mon, Dec 29, 2025 at 06:14:52PM -0300, Fabiano Rosas wrote:
> Peter Xu <[email protected]> writes:
>
> > On Fri, Dec 26, 2025 at 06:19:24PM -0300, Fabiano Rosas wrote:
> >> Now that everything is in channel.c, it's easier to browse this code
> >> if it's all in the same place. It's also easier to grasp what the
> >> connection flow is if both sides of the connection are close together.
> >>
> >> Signed-off-by: Fabiano Rosas <[email protected]>
> >> ---
> >> migration/channel.c | 86 +++++++++++++++++++++++----------------------
> >> migration/channel.h | 14 ++++++--
> >> 2 files changed, 56 insertions(+), 44 deletions(-)
> >>
> >> diff --git a/migration/channel.c b/migration/channel.c
> >> index 042e01b224..ba9aa1c58b 100644
> >> --- a/migration/channel.c
> >> +++ b/migration/channel.c
> >> @@ -31,10 +31,11 @@
> >> #include "trace.h"
> >> #include "yank_functions.h"
> >>
> >> -bool migration_connect_outgoing(MigrationAddress *addr, Error **errp)
> >> +bool migration_connect(MigrationAddress *addr, bool out, Error **errp)
> >> {
> >> g_autoptr(QIOChannel) ioc = NULL;
> >> SocketAddress *saddr;
> >> + ERRP_GUARD();
> >>
> >> switch (addr->transport) {
> >> case MIGRATION_ADDRESS_TYPE_SOCKET:
> >> @@ -44,15 +45,24 @@ bool migration_connect_outgoing(MigrationAddress
> >> *addr, Error **errp)
> >> case SOCKET_ADDRESS_TYPE_INET:
> >> case SOCKET_ADDRESS_TYPE_UNIX:
> >> case SOCKET_ADDRESS_TYPE_VSOCK:
> >> - socket_connect_outgoing(saddr, errp);
> >> - /*
> >> - * async: after the socket is connected, calls
> >> - * migration_channel_connect_outgoing() directly.
> >> - */
> >> - return true;
> >> + if (out) {
> >
> > Personally I wouldn't suggest we merge the outgoing / incoming with
> > migration_connect() then split paths once more in this exact function.
> >
> > I got this conclusion when I started to count how many "if (out)" are
> > there.. When there're too much, it may imply we need to think more..
> >
>
> Well, compared to before, there 50% less "if (addr->transport == ...)",
> this is top level programming! =D
Yep, though that'll be the only part got deduplicated.
Reading migration_connect() will definitely break my flow of thoughts when
hitting so many "if (out)", and I'll be a bit puzzled on how the code runs.
I'm not sure if I'm the only one, though.
If we really want, we can introduce a MigrationAddressOps, providing one
ops for each type of MigrationAddress, differently on both sides:
/* Return true if success; when false errp will be set */
bool (*MigrationAddressOp)(MigrationAddress *addr, QIOChannel **channel, Error
**errp)
Then define:
MigrationAddressOps[MIGRATION_ADDRESS_TYPE__MAX] addr_ops_outgoing = { ... };
MigrationAddressOps[MIGRATION_ADDRESS_TYPE__MAX] addr_ops_incoming = { ... };
And use them.. but it may also be an overkill when we only have incoming /
outgoing anyway.. So IMHO the existing code (after you refactored many of
the rest!) looks still pretty decent to me.
>
> This part of the series is highly subjective, if there's a patch you
> don't like it we can drop it, let's not dwell on it.. Just read my words
> below on the previous patch, which I think you may be mistaken about.
Thanks, yes I was indeed mistaken and overlooked something. :)
>
> > This also answers part of my confusion when reading the previous patch - if
> > that was only paving way for this one, IMHO it may not be as worthwhile,
> > and I would tend to avoid both.
> >
>
> Patch 21 is just a cleanup after patch 19 moves the call to
> migration_channel_connect_outgoing from being inside the transport
> routines to this top level here at migration_connect(), which moves the
> places where MigrationState is used as well. So it removes unused
> passing of MigrationState along with the SocketConnectionData.
Now after I read it again, I agree with those removal of *s where they're
not used, like for:
fd_connect_outgoing()
exec_connect_outgoing()
file_connect_outgoing()
I think socket_connect_outgoing() should also be fine, but maybe better to
have a pre-requisite patch removing SocketConnectData?
For most of the rest, IMHO we don't get much benefit from removing *s from
the parameters, especially inside qmp_migrate()..
So IMHO you were right in the commit log there, that we should justify
every use of migrate_get_current() to deserve fetching from a global, and
we should avoid using it in new code whenever possible. IMHO we should
stick with that.
Imagine the old days we debug when *s can become null, and the more we
reference the global, the harder we fight those things (taking one refcount
from the very top caller would work for all the sub-callers OTOH, when we
justify one place thread-safe and justify all the rest). More referencing
globals normally will just make things harder for us. This rule applies to
all the globals.. not only *s.
>
> > Thoughts?
> >
> >> + socket_connect_outgoing(saddr, errp);
> >> + /*
> >> + * async: after the socket is connected, calls
> >> + * migration_channel_connect_outgoing() directly.
> >> + */
> >> + return true;
> >> + } else {
> >> + socket_connect_incoming(saddr, errp);
> >> + }
> >> +
> >> break;
> >> case SOCKET_ADDRESS_TYPE_FD:
> >> - ioc = fd_connect_outgoing(saddr->u.fd.str, errp);
> >> + if (out) {
> >> + ioc = fd_connect_outgoing(saddr->u.fd.str, errp);
> >> + } else {
> >> + fd_connect_incoming(saddr->u.fd.str, errp);
> >> + }
> >> break;
> >> default:
> >> g_assert_not_reached();
> >> @@ -62,16 +72,28 @@ bool migration_connect_outgoing(MigrationAddress
> >> *addr, Error **errp)
> >>
> >> #ifdef CONFIG_RDMA
> >> case MIGRATION_ADDRESS_TYPE_RDMA:
> >> - ioc = rdma_connect_outgoing(&addr->u.rdma, errp);
> >> + if (out) {
> >> + ioc = rdma_connect_outgoing(&addr->u.rdma, errp);
> >> + } else {
> >> + rdma_connect_incoming(&addr->u.rdma, errp);
> >> + }
> >> break;
> >> #endif
> >>
> >> case MIGRATION_ADDRESS_TYPE_EXEC:
> >> - ioc = exec_connect_outgoing(addr->u.exec.args, errp);
> >> + if (out) {
> >> + ioc = exec_connect_outgoing(addr->u.exec.args, errp);
> >> + } else {
> >> + exec_connect_incoming(addr->u.exec.args, errp);
> >> + }
> >> break;
> >>
> >> case MIGRATION_ADDRESS_TYPE_FILE:
> >> - ioc = file_connect_outgoing(&addr->u.file, errp);
> >> + if (out) {
> >> + ioc = file_connect_outgoing(&addr->u.file, errp);
> >> + } else {
> >> + file_connect_incoming(&addr->u.file, errp);
> >> + }
> >> break;
> >>
> >> default:
> >> @@ -79,42 +101,22 @@ bool migration_connect_outgoing(MigrationAddress
> >> *addr, Error **errp)
> >> break;
> >> }
> >>
> >> - if (!ioc) {
> >> - return false;
> >> - }
> >> -
> >> - migration_channel_connect_outgoing(ioc);
> >> - return true;
> >> -}
> >> -
> >> -void migration_connect_incoming(MigrationAddress *addr, Error **errp)
> >> -{
> >> - if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
> >> - SocketAddress *saddr = &addr->u.socket;
> >> - if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> >> - saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> >> - saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> >> - socket_connect_incoming(saddr, errp);
> >> - } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> >> - fd_connect_incoming(saddr->u.fd.str, errp);
> >> + if (out) {
> >> + if (!ioc) {
> >> + return false;
> >> }
> >> -#ifdef CONFIG_RDMA
> >> - } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
> >> - rdma_connect_incoming(&addr->u.rdma, errp);
> >> -#endif
> >> - } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> >> - exec_connect_incoming(addr->u.exec.args, errp);
> >> - } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) {
> >> - file_connect_incoming(&addr->u.file, errp);
> >> - } else {
> >> - error_setg(errp, "unknown migration protocol");
> >> +
> >> + migration_channel_connect_outgoing(ioc);
> >> + return true;
> >> }
> >>
> >> /*
> >> - * async: the above routines all wait for the incoming connection
> >> - * and call back to migration_channel_process_incoming() to start
> >> - * the migration.
> >> + * async: on the incoming side all of the transport routines above
> >> + * wait for the incoming connection and call back to
> >> + * migration_channel_process_incoming() to start the migration.
> >> */
> >> +
> >> + return !*errp;
> >> }
> >>
> >> bool migration_has_main_and_multifd_channels(void)
> >> diff --git a/migration/channel.h b/migration/channel.h
> >> index 8cf16bfda9..86934fee38 100644
> >> --- a/migration/channel.h
> >> +++ b/migration/channel.h
> >> @@ -39,6 +39,16 @@ int migration_channel_read_peek(QIOChannel *ioc,
> >> bool migration_has_main_and_multifd_channels(void);
> >> bool migration_has_all_channels(void);
> >>
> >> -bool migration_connect_outgoing(MigrationAddress *addr, Error **errp);
> >> -void migration_connect_incoming(MigrationAddress *addr, Error **errp);
> >> +bool migration_connect(MigrationAddress *addr, bool out, Error **errp);
> >> +static inline bool migration_connect_outgoing(MigrationAddress *addr,
> >> + Error **errp)
> >> +{
> >> + return migration_connect(addr, true, errp);
> >> +}
> >> +
> >> +static inline bool migration_connect_incoming(MigrationAddress *addr,
> >> + Error **errp)
> >> +{
> >> + return migration_connect(addr, false, errp);
> >> +}
> >> #endif
> >> --
> >> 2.51.0
> >>
>
--
Peter Xu
