Re: [RFC PATCH 22/25] migration/channel: Merge both sides of the connection initiation code

[Peter Xu <peterx@redhat.com>]

On Mon, Dec 29, 2025 at 06:14:52PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> 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 <farosas@suse.de>
> >> ---
> >>  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