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

[Fabiano Rosas <farosas@suse.de>]

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

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.

> 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.

> 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
>>