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