[PATCH 0/2] migration: cleanup TLS channel referencing

[PATCH 0/2] migration: cleanup TLS channel referencing

[peterx]

From: Peter Xu <peterx@redhat.com>

Based-on: <20240208030528.368214-1-peterx@redhat.com>

The patchset is based on the latest migration pull request.  This is a
small cleanup patchset to firstly cleanup tls iochannel deref on error
paths, then further remove one unused var on yank if the cleanup applies.

These are exactly the same patch I attached here in this email reply:

https://lore.kernel.org/r/ZcLrF5HN920rUTSw@x1n

It's just a formal post, collecting one R-b from Fabiano in patch 2.

Please feel free to have a look, thanks.

Peter Xu (2):
  migration/multifd: Cleanup TLS iochannel referencing
  migration/multifd: Drop registered_yank

 migration/multifd.h |  2 --
 migration/multifd.c | 44 ++++++++++++++++++++++++++------------------
 2 files changed, 26 insertions(+), 20 deletions(-)

-- 
2.43.0

[PATCH 1/2] migration/multifd: Cleanup TLS iochannel referencing

[peterx]

From: Peter Xu <peterx@redhat.com>

Commit a1af605bd5 ("migration/multifd: fix hangup with TLS-Multifd due to
blocking handshake") introduced a thread for TLS channels, which will
resolve the issue on blocking the main thread.  However in the same commit
p->c is slightly abused just to be able to pass over the pointer "p" into
the thread.

That's the major reason we'll need to conditionally free the io channel in
the fault paths.

To clean it up, using a separate structure to pass over both "p" and "tioc"
in the tls handshake thread.  Then we can make it a rule that p->c will
never be set until the channel is completely setup.  With that, we can drop
the tricky conditional unref of the io channel in the error path.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index adfe8c9a0a..4a85a6b7b3 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -873,16 +873,22 @@ out:
 
 static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque);
 
+typedef struct {
+    MultiFDSendParams *p;
+    QIOChannelTLS *tioc;
+} MultiFDTLSThreadArgs;
+
 static void *multifd_tls_handshake_thread(void *opaque)
 {
-    MultiFDSendParams *p = opaque;
-    QIOChannelTLS *tioc = QIO_CHANNEL_TLS(p->c);
+    MultiFDTLSThreadArgs *args = opaque;
 
-    qio_channel_tls_handshake(tioc,
+    qio_channel_tls_handshake(args->tioc,
                               multifd_new_send_channel_async,
-                              p,
+                              args->p,
                               NULL,
                               NULL);
+    g_free(args);
+
     return NULL;
 }
 
@@ -892,6 +898,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
 {
     MigrationState *s = migrate_get_current();
     const char *hostname = s->hostname;
+    MultiFDTLSThreadArgs *args;
     QIOChannelTLS *tioc;
 
     tioc = migration_tls_client_create(ioc, hostname, errp);
@@ -906,11 +913,14 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
     object_unref(OBJECT(ioc));
     trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
     qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
-    p->c = QIO_CHANNEL(tioc);
+
+    args = g_new0(MultiFDTLSThreadArgs, 1);
+    args->tioc = tioc;
+    args->p = p;
 
     p->tls_thread_created = true;
     qemu_thread_create(&p->tls_thread, "multifd-tls-handshake-worker",
-                       multifd_tls_handshake_thread, p,
+                       multifd_tls_handshake_thread, args,
                        QEMU_THREAD_JOINABLE);
     return true;
 }
@@ -923,6 +933,7 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
 
     migration_ioc_register_yank(ioc);
     p->registered_yank = true;
+    /* Setup p->c only if the channel is completely setup */
     p->c = ioc;
 
     p->thread_created = true;
@@ -976,14 +987,12 @@ out:
 
     trace_multifd_new_send_channel_async_error(p->id, local_err);
     multifd_send_set_error(local_err);
-    if (!p->c) {
-        /*
-         * If no channel has been created, drop the initial
-         * reference. Otherwise cleanup happens at
-         * multifd_send_channel_destroy()
-         */
-        object_unref(OBJECT(ioc));
-    }
+    /*
+     * For error cases (TLS or non-TLS), IO channel is always freed here
+     * rather than when cleanup multifd: since p->c is not set, multifd
+     * cleanup code doesn't even know its existance.
+     */
+    object_unref(OBJECT(ioc));
     error_free(local_err);
 }
 
-- 
2.43.0

Re: [PATCH 1/2] migration/multifd: Cleanup TLS iochannel referencing

[Fabiano Rosas]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> Commit a1af605bd5 ("migration/multifd: fix hangup with TLS-Multifd due to
> blocking handshake") introduced a thread for TLS channels, which will
> resolve the issue on blocking the main thread.  However in the same commit
> p->c is slightly abused just to be able to pass over the pointer "p" into
> the thread.
>
> That's the major reason we'll need to conditionally free the io channel in
> the fault paths.
>
> To clean it up, using a separate structure to pass over both "p" and "tioc"
> in the tls handshake thread.  Then we can make it a rule that p->c will
> never be set until the channel is completely setup.  With that, we can drop
> the tricky conditional unref of the io channel in the error path.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Ok, I'm convinced after reading your reply on the other thread.

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 1/2] migration/multifd: Cleanup TLS iochannel referencing

[Avihai Horon]

On 08/02/2024 5:51, peterx@redhat.com wrote:
> External email: Use caution opening links or attachments
>
>
> From: Peter Xu <peterx@redhat.com>
>
> Commit a1af605bd5 ("migration/multifd: fix hangup with TLS-Multifd due to
> blocking handshake") introduced a thread for TLS channels, which will
> resolve the issue on blocking the main thread.  However in the same commit
> p->c is slightly abused just to be able to pass over the pointer "p" into
> the thread.
>
> That's the major reason we'll need to conditionally free the io channel in
> the fault paths.
>
> To clean it up, using a separate structure to pass over both "p" and "tioc"
> in the tls handshake thread.  Then we can make it a rule that p->c will
> never be set until the channel is completely setup.  With that, we can drop
> the tricky conditional unref of the io channel in the error path.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   migration/multifd.c | 37 +++++++++++++++++++++++--------------
>   1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index adfe8c9a0a..4a85a6b7b3 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -873,16 +873,22 @@ out:
>
>   static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque);
>
> +typedef struct {
> +    MultiFDSendParams *p;
> +    QIOChannelTLS *tioc;
> +} MultiFDTLSThreadArgs;
> +
>   static void *multifd_tls_handshake_thread(void *opaque)
>   {
> -    MultiFDSendParams *p = opaque;
> -    QIOChannelTLS *tioc = QIO_CHANNEL_TLS(p->c);
> +    MultiFDTLSThreadArgs *args = opaque;
>
> -    qio_channel_tls_handshake(tioc,
> +    qio_channel_tls_handshake(args->tioc,
>                                 multifd_new_send_channel_async,
> -                              p,
> +                              args->p,
>                                 NULL,
>                                 NULL);
> +    g_free(args);
> +
>       return NULL;
>   }
>
> @@ -892,6 +898,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
>   {
>       MigrationState *s = migrate_get_current();
>       const char *hostname = s->hostname;
> +    MultiFDTLSThreadArgs *args;
>       QIOChannelTLS *tioc;
>
>       tioc = migration_tls_client_create(ioc, hostname, errp);
> @@ -906,11 +913,14 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
>       object_unref(OBJECT(ioc));
>       trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
>       qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
> -    p->c = QIO_CHANNEL(tioc);
> +
> +    args = g_new0(MultiFDTLSThreadArgs, 1);
> +    args->tioc = tioc;
> +    args->p = p;
>
>       p->tls_thread_created = true;
>       qemu_thread_create(&p->tls_thread, "multifd-tls-handshake-worker",
> -                       multifd_tls_handshake_thread, p,
> +                       multifd_tls_handshake_thread, args,
>                          QEMU_THREAD_JOINABLE);
>       return true;
>   }
> @@ -923,6 +933,7 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>
>       migration_ioc_register_yank(ioc);
>       p->registered_yank = true;
> +    /* Setup p->c only if the channel is completely setup */
>       p->c = ioc;
>
>       p->thread_created = true;
> @@ -976,14 +987,12 @@ out:
>
>       trace_multifd_new_send_channel_async_error(p->id, local_err);
>       multifd_send_set_error(local_err);
> -    if (!p->c) {
> -        /*
> -         * If no channel has been created, drop the initial
> -         * reference. Otherwise cleanup happens at
> -         * multifd_send_channel_destroy()
> -         */
> -        object_unref(OBJECT(ioc));
> -    }
> +    /*
> +     * For error cases (TLS or non-TLS), IO channel is always freed here
> +     * rather than when cleanup multifd: since p->c is not set, multifd
> +     * cleanup code doesn't even know its existance.

Small nit:
s/existance/existence

BTW, I just noticed that multifd_channel_connect() can't fail, probably 
would be good to turn it into a void function.

Thanks.

> +     */
> +    object_unref(OBJECT(ioc));
>       error_free(local_err);
>   }
>
> --
> 2.43.0
>

Re: [PATCH 1/2] migration/multifd: Cleanup TLS iochannel referencing

[Peter Xu]

On Thu, Feb 08, 2024 at 04:10:58PM +0200, Avihai Horon wrote:
> 
> On 08/02/2024 5:51, peterx@redhat.com wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > From: Peter Xu <peterx@redhat.com>
> > 
> > Commit a1af605bd5 ("migration/multifd: fix hangup with TLS-Multifd due to
> > blocking handshake") introduced a thread for TLS channels, which will
> > resolve the issue on blocking the main thread.  However in the same commit
> > p->c is slightly abused just to be able to pass over the pointer "p" into
> > the thread.
> > 
> > That's the major reason we'll need to conditionally free the io channel in
> > the fault paths.
> > 
> > To clean it up, using a separate structure to pass over both "p" and "tioc"
> > in the tls handshake thread.  Then we can make it a rule that p->c will
> > never be set until the channel is completely setup.  With that, we can drop
> > the tricky conditional unref of the io channel in the error path.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   migration/multifd.c | 37 +++++++++++++++++++++++--------------
> >   1 file changed, 23 insertions(+), 14 deletions(-)
> > 
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index adfe8c9a0a..4a85a6b7b3 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -873,16 +873,22 @@ out:
> > 
> >   static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque);
> > 
> > +typedef struct {
> > +    MultiFDSendParams *p;
> > +    QIOChannelTLS *tioc;
> > +} MultiFDTLSThreadArgs;
> > +
> >   static void *multifd_tls_handshake_thread(void *opaque)
> >   {
> > -    MultiFDSendParams *p = opaque;
> > -    QIOChannelTLS *tioc = QIO_CHANNEL_TLS(p->c);
> > +    MultiFDTLSThreadArgs *args = opaque;
> > 
> > -    qio_channel_tls_handshake(tioc,
> > +    qio_channel_tls_handshake(args->tioc,
> >                                 multifd_new_send_channel_async,
> > -                              p,
> > +                              args->p,
> >                                 NULL,
> >                                 NULL);
> > +    g_free(args);
> > +
> >       return NULL;
> >   }
> > 
> > @@ -892,6 +898,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
> >   {
> >       MigrationState *s = migrate_get_current();
> >       const char *hostname = s->hostname;
> > +    MultiFDTLSThreadArgs *args;
> >       QIOChannelTLS *tioc;
> > 
> >       tioc = migration_tls_client_create(ioc, hostname, errp);
> > @@ -906,11 +913,14 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
> >       object_unref(OBJECT(ioc));
> >       trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
> >       qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
> > -    p->c = QIO_CHANNEL(tioc);
> > +
> > +    args = g_new0(MultiFDTLSThreadArgs, 1);
> > +    args->tioc = tioc;
> > +    args->p = p;
> > 
> >       p->tls_thread_created = true;
> >       qemu_thread_create(&p->tls_thread, "multifd-tls-handshake-worker",
> > -                       multifd_tls_handshake_thread, p,
> > +                       multifd_tls_handshake_thread, args,
> >                          QEMU_THREAD_JOINABLE);
> >       return true;
> >   }
> > @@ -923,6 +933,7 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
> > 
> >       migration_ioc_register_yank(ioc);
> >       p->registered_yank = true;
> > +    /* Setup p->c only if the channel is completely setup */
> >       p->c = ioc;
> > 
> >       p->thread_created = true;
> > @@ -976,14 +987,12 @@ out:
> > 
> >       trace_multifd_new_send_channel_async_error(p->id, local_err);
> >       multifd_send_set_error(local_err);
> > -    if (!p->c) {
> > -        /*
> > -         * If no channel has been created, drop the initial
> > -         * reference. Otherwise cleanup happens at
> > -         * multifd_send_channel_destroy()
> > -         */
> > -        object_unref(OBJECT(ioc));
> > -    }
> > +    /*
> > +     * For error cases (TLS or non-TLS), IO channel is always freed here
> > +     * rather than when cleanup multifd: since p->c is not set, multifd
> > +     * cleanup code doesn't even know its existance.
> 
> Small nit:
> s/existance/existence
> 
> BTW, I just noticed that multifd_channel_connect() can't fail, probably
> would be good to turn it into a void function.

Yes we can.  I'll add a patch and fix the spell, thanks.

-- 
Peter Xu

[PATCH 2/2] migration/multifd: Drop registered_yank

[peterx]

From: Peter Xu <peterx@redhat.com>

With a clear definition of p->c protocol, where we only set it up if the
channel is fully established (TLS or non-TLS), registered_yank boolean will
have equal meaning of "p->c != NULL".

Drop registered_yank by checking p->c instead.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h | 2 --
 migration/multifd.c | 7 +++----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index 8a1cad0996..b3fe27ae93 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -78,8 +78,6 @@ typedef struct {
     bool tls_thread_created;
     /* communication channel */
     QIOChannel *c;
-    /* is the yank function registered */
-    bool registered_yank;
     /* packet allocated len */
     uint32_t packet_len;
     /* guest page size */
diff --git a/migration/multifd.c b/migration/multifd.c
index 4a85a6b7b3..278453cf84 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send)
 
 static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
 {
-    if (p->registered_yank) {
+    if (p->c) {
         migration_ioc_unregister_yank(p->c);
+        multifd_send_channel_destroy(p->c);
+        p->c = NULL;
     }
-    multifd_send_channel_destroy(p->c);
-    p->c = NULL;
     qemu_sem_destroy(&p->sem);
     qemu_sem_destroy(&p->sem_sync);
     g_free(p->name);
@@ -932,7 +932,6 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
     qio_channel_set_delay(ioc, false);
 
     migration_ioc_register_yank(ioc);
-    p->registered_yank = true;
     /* Setup p->c only if the channel is completely setup */
     p->c = ioc;
 
-- 
2.43.0

Re: [PATCH 2/2] migration/multifd: Drop registered_yank

[Fabiano Rosas]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> With a clear definition of p->c protocol, where we only set it up if the
> channel is fully established (TLS or non-TLS), registered_yank boolean will
> have equal meaning of "p->c != NULL".
>
> Drop registered_yank by checking p->c instead.
>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/multifd.h | 2 --
>  migration/multifd.c | 7 +++----
>  2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 8a1cad0996..b3fe27ae93 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -78,8 +78,6 @@ typedef struct {
>      bool tls_thread_created;
>      /* communication channel */
>      QIOChannel *c;
> -    /* is the yank function registered */
> -    bool registered_yank;
>      /* packet allocated len */
>      uint32_t packet_len;
>      /* guest page size */
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 4a85a6b7b3..278453cf84 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send)
>  
>  static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>  {
> -    if (p->registered_yank) {
> +    if (p->c) {
>          migration_ioc_unregister_yank(p->c);
> +        multifd_send_channel_destroy(p->c);

At socket_send_channel_destroy the clean up of outgoing_args.saddr will
now be skipped. The failure at multifd_new_send_channel_async might have
been due to TLS, in which case all of plain socket setup will have
happened properly.

> +        p->c = NULL;
>      }
> -    multifd_send_channel_destroy(p->c);
> -    p->c = NULL;
>      qemu_sem_destroy(&p->sem);
>      qemu_sem_destroy(&p->sem_sync);
>      g_free(p->name);
> @@ -932,7 +932,6 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>      qio_channel_set_delay(ioc, false);
>  
>      migration_ioc_register_yank(ioc);
> -    p->registered_yank = true;
>      /* Setup p->c only if the channel is completely setup */
>      p->c = ioc;

Re: [PATCH 2/2] migration/multifd: Drop registered_yank

[Peter Xu]

On Thu, Feb 08, 2024 at 09:48:33AM -0300, Fabiano Rosas wrote:
> peterx@redhat.com writes:
> 
> > From: Peter Xu <peterx@redhat.com>
> >
> > With a clear definition of p->c protocol, where we only set it up if the
> > channel is fully established (TLS or non-TLS), registered_yank boolean will
> > have equal meaning of "p->c != NULL".
> >
> > Drop registered_yank by checking p->c instead.
> >
> > Reviewed-by: Fabiano Rosas <farosas@suse.de>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/multifd.h | 2 --
> >  migration/multifd.c | 7 +++----
> >  2 files changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/migration/multifd.h b/migration/multifd.h
> > index 8a1cad0996..b3fe27ae93 100644
> > --- a/migration/multifd.h
> > +++ b/migration/multifd.h
> > @@ -78,8 +78,6 @@ typedef struct {
> >      bool tls_thread_created;
> >      /* communication channel */
> >      QIOChannel *c;
> > -    /* is the yank function registered */
> > -    bool registered_yank;
> >      /* packet allocated len */
> >      uint32_t packet_len;
> >      /* guest page size */
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 4a85a6b7b3..278453cf84 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send)
> >  
> >  static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
> >  {
> > -    if (p->registered_yank) {
> > +    if (p->c) {
> >          migration_ioc_unregister_yank(p->c);
> > +        multifd_send_channel_destroy(p->c);
> 
> At socket_send_channel_destroy the clean up of outgoing_args.saddr will
> now be skipped. The failure at multifd_new_send_channel_async might have
> been due to TLS, in which case all of plain socket setup will have
> happened properly.

Right, IMHO it's a hack to free globals in a per-channel helper.  We should
have moved:

    if (outgoing_args.saddr) {
        qapi_free_SocketAddress(outgoing_args.saddr);
        outgoing_args.saddr = NULL;
    }

Outside irrelevant of that..

That could be done later I guess, because we have one more guard:

socket_start_outgoing_migration():
    /* in case previous migration leaked it */
    qapi_free_SocketAddress(outgoing_args.saddr);
    outgoing_args.saddr = addr;

If you think proper, I can add one more patch to do that cleanup, IOW, move
above free() into multifd_send_cleanup_state().

Thanks,

-- 
Peter Xu

Re: [PATCH 2/2] migration/multifd: Drop registered_yank

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Feb 08, 2024 at 09:48:33AM -0300, Fabiano Rosas wrote:
>> peterx@redhat.com writes:
>> 
>> > From: Peter Xu <peterx@redhat.com>
>> >
>> > With a clear definition of p->c protocol, where we only set it up if the
>> > channel is fully established (TLS or non-TLS), registered_yank boolean will
>> > have equal meaning of "p->c != NULL".
>> >
>> > Drop registered_yank by checking p->c instead.
>> >
>> > Reviewed-by: Fabiano Rosas <farosas@suse.de>
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  migration/multifd.h | 2 --
>> >  migration/multifd.c | 7 +++----
>> >  2 files changed, 3 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/migration/multifd.h b/migration/multifd.h
>> > index 8a1cad0996..b3fe27ae93 100644
>> > --- a/migration/multifd.h
>> > +++ b/migration/multifd.h
>> > @@ -78,8 +78,6 @@ typedef struct {
>> >      bool tls_thread_created;
>> >      /* communication channel */
>> >      QIOChannel *c;
>> > -    /* is the yank function registered */
>> > -    bool registered_yank;
>> >      /* packet allocated len */
>> >      uint32_t packet_len;
>> >      /* guest page size */
>> > diff --git a/migration/multifd.c b/migration/multifd.c
>> > index 4a85a6b7b3..278453cf84 100644
>> > --- a/migration/multifd.c
>> > +++ b/migration/multifd.c
>> > @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send)
>> >  
>> >  static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>> >  {
>> > -    if (p->registered_yank) {
>> > +    if (p->c) {
>> >          migration_ioc_unregister_yank(p->c);
>> > +        multifd_send_channel_destroy(p->c);
>> 
>> At socket_send_channel_destroy the clean up of outgoing_args.saddr will
>> now be skipped. The failure at multifd_new_send_channel_async might have
>> been due to TLS, in which case all of plain socket setup will have
>> happened properly.
>
> Right, IMHO it's a hack to free globals in a per-channel helper.  We should
> have moved:
>
>     if (outgoing_args.saddr) {
>         qapi_free_SocketAddress(outgoing_args.saddr);
>         outgoing_args.saddr = NULL;
>     }
>
> Outside irrelevant of that..
>
> That could be done later I guess, because we have one more guard:
>
> socket_start_outgoing_migration():
>     /* in case previous migration leaked it */
>     qapi_free_SocketAddress(outgoing_args.saddr);
>     outgoing_args.saddr = addr;
>
> If you think proper, I can add one more patch to do that cleanup, IOW, move
> above free() into multifd_send_cleanup_state().

Sounds good.

>
> Thanks,