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) {
+ 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.0On 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..
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.
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
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
>>
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
© 2016 - 2026 Red Hat, Inc.