We're currently leaking the resources of the TLS thread by not joining
it and also overwriting the p->thread pointer altogether.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 10 +++++++++-
 migration/multifd.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 409460684f..d632dbc095 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -525,6 +525,10 @@ void multifd_save_cleanup(void)
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
 
+        if (p->tls_thread) {
+            qemu_thread_join(p->tls_thread);
+        }
+
         if (p->running) {
             qemu_thread_join(&p->thread);
         }
@@ -552,6 +556,8 @@ void multifd_save_cleanup(void)
         p->iov = NULL;
         g_free(p->normal);
         p->normal = NULL;
+        g_free(p->tls_thread);
+        p->tls_thread = NULL;
         multifd_send_state->ops->send_cleanup(p, &local_err);
         if (local_err) {
             migrate_set_error(migrate_get_current(), local_err);
@@ -826,7 +832,9 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
     trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
     qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
     p->c = QIO_CHANNEL(tioc);
-    qemu_thread_create(&p->thread, "multifd-tls-handshake-worker",
+
+    p->tls_thread = g_new0(QemuThread, 1);
+    qemu_thread_create(p->tls_thread, "multifd-tls-handshake-worker",
                        multifd_tls_handshake_thread, p,
                        QEMU_THREAD_JOINABLE);
     return true;
diff --git a/migration/multifd.h b/migration/multifd.h
index a835643b48..4ff78e9863 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -75,6 +75,7 @@ typedef struct {
     char *name;
     /* channel thread id */
     QemuThread thread;
+    QemuThread *tls_thread;
     /* communication channel */
     QIOChannel *c;
     /* is the yank function registered */
-- 
2.35.3

On Fri, Nov 10, 2023 at 05:02:39PM -0300, Fabiano Rosas wrote:
> @@ -826,7 +832,9 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
>      trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
>      qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
>      p->c = QIO_CHANNEL(tioc);
> -    qemu_thread_create(&p->thread, "multifd-tls-handshake-worker",
> +
> +    p->tls_thread = g_new0(QemuThread, 1);
> +    qemu_thread_create(p->tls_thread, "multifd-tls-handshake-worker",
>                         multifd_tls_handshake_thread, p,
>                         QEMU_THREAD_JOINABLE);

I understand your way of doing this, but personally I prefer bool and make
QemuThread not a pointer but still statically allocated.

Same comment to the next patch.

IMHO we can even add support for QemuThread in general to remember the bool
itself:

        struct QemuThread {
                pthread_t thread;
                bool thread_created;
        };

Changing qemu_thread_create() to set the bool, and join() to skip the real
join if it's not even created; clearing the bool otherwise after join()ed.
I _think_ it'll work transparently to existing callers, but start to allow
join() to be bypassed if thread not even created.

Thanks,

-- 
Peter Xu