[PATCH v3 1/1] yank: Unregister function when using TLS migration

[PATCH v3 1/1] yank: Unregister function when using TLS migration

[Leonardo Bras]

After yank feature was introduced in migration, whenever migration
is started using TLS, the following error happens in both source and
destination hosts:

(qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
Assertion `QLIST_EMPTY(&entry->yankfns)' failed.

This happens because of a missing yank_unregister_function() when using
qio-channel-tls.

Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform
yank_unregister_function() in channel_close() and multifd_load_cleanup().

Also, inside migration_channel_connect() and
migration_channel_process_incoming() move yank_register_function() so
it only runs once on a TLS migration.

Fixes: b5eea99ec2f ("migration: Add yank feature", 2021-01-13)
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
Signed-off-by: Leonardo Bras <leobras.c@gmail.com>

--
Changes since v2:
- Dropped all references to ioc->master
- yank_register_function() and yank_unregister_function() now only run
  once in a TLS migration.

Changes since v1:
- Cast p->c to QIOChannelTLS into multifd_load_cleanup()
---
 migration/channel.c           | 26 ++++++++++++++------------
 migration/multifd.c           |  3 ++-
 migration/qemu-file-channel.c |  4 +++-
 3 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/migration/channel.c b/migration/channel.c
index c9ee902021..01275a9162 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -38,18 +38,19 @@ void migration_channel_process_incoming(QIOChannel *ioc)
     trace_migration_set_incoming_channel(
         ioc, object_get_typename(OBJECT(ioc)));
 
-    if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
-        yank_register_function(MIGRATION_YANK_INSTANCE,
-                               migration_yank_iochannel,
-                               QIO_CHANNEL(ioc));
-    }
-
     if (s->parameters.tls_creds &&
         *s->parameters.tls_creds &&
         !object_dynamic_cast(OBJECT(ioc),
                              TYPE_QIO_CHANNEL_TLS)) {
         migration_tls_channel_process_incoming(s, ioc, &local_err);
     } else {
+        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
+            object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
+            yank_register_function(MIGRATION_YANK_INSTANCE,
+                                   migration_yank_iochannel,
+                                   QIO_CHANNEL(ioc));
+        }
+
         migration_ioc_process_incoming(ioc, &local_err);
     }
 
@@ -76,12 +77,6 @@ void migration_channel_connect(MigrationState *s,
         ioc, object_get_typename(OBJECT(ioc)), hostname, error);
 
     if (!error) {
-        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
-            yank_register_function(MIGRATION_YANK_INSTANCE,
-                                   migration_yank_iochannel,
-                                   QIO_CHANNEL(ioc));
-        }
-
         if (s->parameters.tls_creds &&
             *s->parameters.tls_creds &&
             !object_dynamic_cast(OBJECT(ioc),
@@ -99,6 +94,13 @@ void migration_channel_connect(MigrationState *s,
         } else {
             QEMUFile *f = qemu_fopen_channel_output(ioc);
 
+            if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
+                object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
+                yank_register_function(MIGRATION_YANK_INSTANCE,
+                                       migration_yank_iochannel,
+                                       QIO_CHANNEL(ioc));
+            }
+
             qemu_mutex_lock(&s->qemu_file_lock);
             s->to_dst_file = f;
             qemu_mutex_unlock(&s->qemu_file_lock);
diff --git a/migration/multifd.c b/migration/multifd.c
index 0a4803cfcc..2e8f001bc0 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -987,7 +987,8 @@ int multifd_load_cleanup(Error **errp)
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
 
-        if (object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET)
+        if ((object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET) ||
+             object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_TLS))
             && OBJECT(p->c)->ref == 1) {
             yank_unregister_function(MIGRATION_YANK_INSTANCE,
                                      migration_yank_iochannel,
diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
index 876d05a540..fad340ea7a 100644
--- a/migration/qemu-file-channel.c
+++ b/migration/qemu-file-channel.c
@@ -26,6 +26,7 @@
 #include "qemu-file-channel.h"
 #include "qemu-file.h"
 #include "io/channel-socket.h"
+#include "io/channel-tls.h"
 #include "qemu/iov.h"
 #include "qemu/yank.h"
 #include "yank_functions.h"
@@ -106,7 +107,8 @@ static int channel_close(void *opaque, Error **errp)
     int ret;
     QIOChannel *ioc = QIO_CHANNEL(opaque);
     ret = qio_channel_close(ioc, errp);
-    if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)
+    if ((object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
+         object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS))
         && OBJECT(ioc)->ref == 1) {
         yank_unregister_function(MIGRATION_YANK_INSTANCE,
                                  migration_yank_iochannel,
-- 
2.31.1

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 5866 bytes --]

On Tue,  1 Jun 2021 02:40:31 -0300
Leonardo Bras <leobras.c@gmail.com> wrote:

> After yank feature was introduced in migration, whenever migration
> is started using TLS, the following error happens in both source and
> destination hosts:
> 
> (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> 
> This happens because of a missing yank_unregister_function() when using
> qio-channel-tls.
> 
> Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform
> yank_unregister_function() in channel_close() and multifd_load_cleanup().
> 
> Also, inside migration_channel_connect() and
> migration_channel_process_incoming() move yank_register_function() so
> it only runs once on a TLS migration.
> 
> Fixes: b5eea99ec2f ("migration: Add yank feature", 2021-01-13)
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
>

Looks good to me,
Reviewed-by: Lukas Straub <lukasstraub2@web.de>

> --
> Changes since v2:
> - Dropped all references to ioc->master
> - yank_register_function() and yank_unregister_function() now only run
>   once in a TLS migration.
> 
> Changes since v1:
> - Cast p->c to QIOChannelTLS into multifd_load_cleanup()
> ---
>  migration/channel.c           | 26 ++++++++++++++------------
>  migration/multifd.c           |  3 ++-
>  migration/qemu-file-channel.c |  4 +++-
>  3 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index c9ee902021..01275a9162 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -38,18 +38,19 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>      trace_migration_set_incoming_channel(
>          ioc, object_get_typename(OBJECT(ioc)));
>  
> -    if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
> -        yank_register_function(MIGRATION_YANK_INSTANCE,
> -                               migration_yank_iochannel,
> -                               QIO_CHANNEL(ioc));
> -    }
> -
>      if (s->parameters.tls_creds &&
>          *s->parameters.tls_creds &&
>          !object_dynamic_cast(OBJECT(ioc),
>                               TYPE_QIO_CHANNEL_TLS)) {
>          migration_tls_channel_process_incoming(s, ioc, &local_err);
>      } else {
> +        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +            object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> +            yank_register_function(MIGRATION_YANK_INSTANCE,
> +                                   migration_yank_iochannel,
> +                                   QIO_CHANNEL(ioc));
> +        }
> +
>          migration_ioc_process_incoming(ioc, &local_err);
>      }
>  
> @@ -76,12 +77,6 @@ void migration_channel_connect(MigrationState *s,
>          ioc, object_get_typename(OBJECT(ioc)), hostname, error);
>  
>      if (!error) {
> -        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
> -            yank_register_function(MIGRATION_YANK_INSTANCE,
> -                                   migration_yank_iochannel,
> -                                   QIO_CHANNEL(ioc));
> -        }
> -
>          if (s->parameters.tls_creds &&
>              *s->parameters.tls_creds &&
>              !object_dynamic_cast(OBJECT(ioc),
> @@ -99,6 +94,13 @@ void migration_channel_connect(MigrationState *s,
>          } else {
>              QEMUFile *f = qemu_fopen_channel_output(ioc);
>  
> +            if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +                object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> +                yank_register_function(MIGRATION_YANK_INSTANCE,
> +                                       migration_yank_iochannel,
> +                                       QIO_CHANNEL(ioc));
> +            }
> +
>              qemu_mutex_lock(&s->qemu_file_lock);
>              s->to_dst_file = f;
>              qemu_mutex_unlock(&s->qemu_file_lock);
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 0a4803cfcc..2e8f001bc0 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -987,7 +987,8 @@ int multifd_load_cleanup(Error **errp)
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDRecvParams *p = &multifd_recv_state->params[i];
>  
> -        if (object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET)
> +        if ((object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET) ||
> +             object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_TLS))
>              && OBJECT(p->c)->ref == 1) {
>              yank_unregister_function(MIGRATION_YANK_INSTANCE,
>                                       migration_yank_iochannel,
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 876d05a540..fad340ea7a 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -26,6 +26,7 @@
>  #include "qemu-file-channel.h"
>  #include "qemu-file.h"
>  #include "io/channel-socket.h"
> +#include "io/channel-tls.h"
>  #include "qemu/iov.h"
>  #include "qemu/yank.h"
>  #include "yank_functions.h"
> @@ -106,7 +107,8 @@ static int channel_close(void *opaque, Error **errp)
>      int ret;
>      QIOChannel *ioc = QIO_CHANNEL(opaque);
>      ret = qio_channel_close(ioc, errp);
> -    if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)
> +    if ((object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +         object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS))
>          && OBJECT(ioc)->ref == 1) {
>          yank_unregister_function(MIGRATION_YANK_INSTANCE,
>                                   migration_yank_iochannel,



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Peter Xu]

On Tue, Jun 01, 2021 at 02:40:31AM -0300, Leonardo Bras wrote:
> After yank feature was introduced in migration, whenever migration
> is started using TLS, the following error happens in both source and
> destination hosts:
> 
> (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> 
> This happens because of a missing yank_unregister_function() when using
> qio-channel-tls.
> 
> Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform
> yank_unregister_function() in channel_close() and multifd_load_cleanup().
> 
> Also, inside migration_channel_connect() and
> migration_channel_process_incoming() move yank_register_function() so
> it only runs once on a TLS migration.

Slightly inaccurate I guess, as it was run once too before this patch, but not
paired for tls?

IIUC when we call the 2nd time at below chunk we won't register again:

        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
            yank_register_function(MIGRATION_YANK_INSTANCE,
                                   migration_yank_iochannel,
                                   QIO_CHANNEL(ioc));
        }

Because the 2nd call will be TYPE_QIO_CHANNEL_TLS, so object_dynamic_cast()
will return with a failure, I think (note, TYPE_QIO_CHANNEL_TLS's parent is
TYPE_QIO_CHANNEL, not TYPE_QIO_CHANNEL_SOCKET).

> 
> Fixes: b5eea99ec2f ("migration: Add yank feature", 2021-01-13)
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks Leo!

I have one pure question not directly related to Leo's patch (probably for
Lukas?): we check OBJECT(ioc)->ref == 1 when unregister each function.  In what
case will the ref be not one?

-- 
Peter Xu

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

On Tue, 1 Jun 2021 10:50:24 -0400
Peter Xu <peterx@redhat.com> wrote:

> On Tue, Jun 01, 2021 at 02:40:31AM -0300, Leonardo Bras wrote:
> > After yank feature was introduced in migration, whenever migration
> > is started using TLS, the following error happens in both source and
> > destination hosts:
> > 
> > (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> > Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> > 
> > This happens because of a missing yank_unregister_function() when using
> > qio-channel-tls.
> > 
> > Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform
> > yank_unregister_function() in channel_close() and multifd_load_cleanup().
> > 
> > Also, inside migration_channel_connect() and
> > migration_channel_process_incoming() move yank_register_function() so
> > it only runs once on a TLS migration.  
> 
> Slightly inaccurate I guess, as it was run once too before this patch, but not
> paired for tls?
> 
> IIUC when we call the 2nd time at below chunk we won't register again:
> 
>         if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
>             yank_register_function(MIGRATION_YANK_INSTANCE,
>                                    migration_yank_iochannel,
>                                    QIO_CHANNEL(ioc));
>         }
> 
> Because the 2nd call will be TYPE_QIO_CHANNEL_TLS, so object_dynamic_cast()
> will return with a failure, I think (note, TYPE_QIO_CHANNEL_TLS's parent is
> TYPE_QIO_CHANNEL, not TYPE_QIO_CHANNEL_SOCKET).
> 
> > 
> > Fixes: b5eea99ec2f ("migration: Add yank feature", 2021-01-13)
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>  
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> Thanks Leo!
> 
> I have one pure question not directly related to Leo's patch (probably for
> Lukas?): we check OBJECT(ioc)->ref == 1 when unregister each function.  In what
> case will the ref be not one?
> 

If a return path is opened with qemu_file_get_return_path(), it will
take additional references:

qemu_file_get_return_path() (qemu-file.c)
 f->ops->get_return_path() -> channel_get_input_return_path() (qemu-file-channel.c)
  qemu_fopen_channel_input() (qemu-file-channel.c)
   object_ref(OBJECT(ioc))

Regards,
Lukas Straub

-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Peter Xu]

On Tue, Jun 01, 2021 at 05:32:33PM +0200, Lukas Straub wrote:
> > I have one pure question not directly related to Leo's patch (probably for
> > Lukas?): we check OBJECT(ioc)->ref == 1 when unregister each function.  In what
> > case will the ref be not one?
> > 
> 
> If a return path is opened with qemu_file_get_return_path(), it will
> take additional references:
> 
> qemu_file_get_return_path() (qemu-file.c)
>  f->ops->get_return_path() -> channel_get_input_return_path() (qemu-file-channel.c)
>   qemu_fopen_channel_input() (qemu-file-channel.c)
>    object_ref(OBJECT(ioc))

Makes sense.

Wondering whether it's better to unregister in migrate_fd_cleanup() rather than
channel_close(), as we used to register in migration code rather than in qemu
file wrapper layer.  I feel like we can drop the ref==1 check if we move, as
the return path should have been closed when reaching migrate_fd_cleanup()
(similar doubt to multifd_load_cleanup() calls to yank_unregister_function()).
No strong opinion, though..

Thanks,

-- 
Peter Xu

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Leonardo Brás]

On Tue, 2021-06-01 at 13:00 +0200, Lukas Straub wrote:
> [...]
> Looks good to me,
> Reviewed-by: Lukas Straub <lukasstraub2@web.de>
> 

Thanks!

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Leonardo Brás]

On Tue, 2021-06-01 at 10:50 -0400, Peter Xu wrote:
> On Tue, Jun 01, 2021 at 02:40:31AM -0300, Leonardo Bras wrote:
> > [...]
> >  inside migration_channel_connect() and
> > migration_channel_process_incoming() move yank_register_function()
> > so
> > it only runs once on a TLS migration.
> 
> Slightly inaccurate I guess, as it was run once too before this
> patch, but not
> paired for tls?

Yeah, I think I got confused over this:
I remembered having multiple instances of yank_register_function()
during TLS migration on qemu.org/master, but after re-testing it, I
think it was in fact something to do with V1 and V2.

So, yes, that's correct, in qemu.org/master there was a single
yank_register_function() and no pairing yank_unregister_function()
during TLS migration. 

> 
> IIUC when we call the 2nd time at below chunk we won't register
> again:
> 
>         if (object_dynamic_cast(OBJECT(ioc),
> TYPE_QIO_CHANNEL_SOCKET)) {
>             yank_register_function(MIGRATION_YANK_INSTANCE,
>                                    migration_yank_iochannel,
>                                    QIO_CHANNEL(ioc));
>         }
> 
> Because the 2nd call will be TYPE_QIO_CHANNEL_TLS, so
> object_dynamic_cast()
> will return with a failure, I think (note, TYPE_QIO_CHANNEL_TLS's
> parent is
> TYPE_QIO_CHANNEL, not TYPE_QIO_CHANNEL_SOCKET).
> 
> > 
> > Fixes: b5eea99ec2f ("migration: Add yank feature", 2021-01-13)
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> Thanks Leo!

Thank you Peter!

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Dr. David Alan Gilbert]

* Leonardo Bras (leobras.c@gmail.com) wrote:
> After yank feature was introduced in migration, whenever migration
> is started using TLS, the following error happens in both source and
> destination hosts:
> 
> (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> 
> This happens because of a missing yank_unregister_function() when using
> qio-channel-tls.
> 
> Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform
> yank_unregister_function() in channel_close() and multifd_load_cleanup().
> 
> Also, inside migration_channel_connect() and
> migration_channel_process_incoming() move yank_register_function() so
> it only runs once on a TLS migration.
> 
> Fixes: b5eea99ec2f ("migration: Add yank feature", 2021-01-13)
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>

Queued; thank you!

> --
> Changes since v2:
> - Dropped all references to ioc->master
> - yank_register_function() and yank_unregister_function() now only run
>   once in a TLS migration.
> 
> Changes since v1:
> - Cast p->c to QIOChannelTLS into multifd_load_cleanup()
> ---
>  migration/channel.c           | 26 ++++++++++++++------------
>  migration/multifd.c           |  3 ++-
>  migration/qemu-file-channel.c |  4 +++-
>  3 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index c9ee902021..01275a9162 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -38,18 +38,19 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>      trace_migration_set_incoming_channel(
>          ioc, object_get_typename(OBJECT(ioc)));
>  
> -    if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
> -        yank_register_function(MIGRATION_YANK_INSTANCE,
> -                               migration_yank_iochannel,
> -                               QIO_CHANNEL(ioc));
> -    }
> -
>      if (s->parameters.tls_creds &&
>          *s->parameters.tls_creds &&
>          !object_dynamic_cast(OBJECT(ioc),
>                               TYPE_QIO_CHANNEL_TLS)) {
>          migration_tls_channel_process_incoming(s, ioc, &local_err);
>      } else {
> +        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +            object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> +            yank_register_function(MIGRATION_YANK_INSTANCE,
> +                                   migration_yank_iochannel,
> +                                   QIO_CHANNEL(ioc));
> +        }
> +
>          migration_ioc_process_incoming(ioc, &local_err);
>      }
>  
> @@ -76,12 +77,6 @@ void migration_channel_connect(MigrationState *s,
>          ioc, object_get_typename(OBJECT(ioc)), hostname, error);
>  
>      if (!error) {
> -        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
> -            yank_register_function(MIGRATION_YANK_INSTANCE,
> -                                   migration_yank_iochannel,
> -                                   QIO_CHANNEL(ioc));
> -        }
> -
>          if (s->parameters.tls_creds &&
>              *s->parameters.tls_creds &&
>              !object_dynamic_cast(OBJECT(ioc),
> @@ -99,6 +94,13 @@ void migration_channel_connect(MigrationState *s,
>          } else {
>              QEMUFile *f = qemu_fopen_channel_output(ioc);
>  
> +            if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +                object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> +                yank_register_function(MIGRATION_YANK_INSTANCE,
> +                                       migration_yank_iochannel,
> +                                       QIO_CHANNEL(ioc));
> +            }
> +
>              qemu_mutex_lock(&s->qemu_file_lock);
>              s->to_dst_file = f;
>              qemu_mutex_unlock(&s->qemu_file_lock);
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 0a4803cfcc..2e8f001bc0 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -987,7 +987,8 @@ int multifd_load_cleanup(Error **errp)
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDRecvParams *p = &multifd_recv_state->params[i];
>  
> -        if (object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET)
> +        if ((object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET) ||
> +             object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_TLS))
>              && OBJECT(p->c)->ref == 1) {
>              yank_unregister_function(MIGRATION_YANK_INSTANCE,
>                                       migration_yank_iochannel,
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 876d05a540..fad340ea7a 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -26,6 +26,7 @@
>  #include "qemu-file-channel.h"
>  #include "qemu-file.h"
>  #include "io/channel-socket.h"
> +#include "io/channel-tls.h"
>  #include "qemu/iov.h"
>  #include "qemu/yank.h"
>  #include "yank_functions.h"
> @@ -106,7 +107,8 @@ static int channel_close(void *opaque, Error **errp)
>      int ret;
>      QIOChannel *ioc = QIO_CHANNEL(opaque);
>      ret = qio_channel_close(ioc, errp);
> -    if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)
> +    if ((object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +         object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS))
>          && OBJECT(ioc)->ref == 1) {
>          yank_unregister_function(MIGRATION_YANK_INSTANCE,
>                                   migration_yank_iochannel,
> -- 
> 2.31.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Dr. David Alan Gilbert]

cc'ing in qemu-stable - I think we'd probably want this on 6.0
(It's currently merged as 7de2e8565335c13fb3516cddbe2e40e366cce273 ).
Although you'll probably also want the missing dependency fix
Philippe is working (See: 
Mathieu- ( 42) [RFC PATCH] migration: Add missing dependency on GNUTLS )

Dave

* Leonardo Bras (leobras.c@gmail.com) wrote:
> After yank feature was introduced in migration, whenever migration
> is started using TLS, the following error happens in both source and
> destination hosts:
> 
> (qemu) qemu-kvm: ../util/yank.c:107: yank_unregister_instance:
> Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> 
> This happens because of a missing yank_unregister_function() when using
> qio-channel-tls.
> 
> Fix this by also allowing TYPE_QIO_CHANNEL_TLS object type to perform
> yank_unregister_function() in channel_close() and multifd_load_cleanup().
> 
> Also, inside migration_channel_connect() and
> migration_channel_process_incoming() move yank_register_function() so
> it only runs once on a TLS migration.
> 
> Fixes: b5eea99ec2f ("migration: Add yank feature", 2021-01-13)
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> 
> --
> Changes since v2:
> - Dropped all references to ioc->master
> - yank_register_function() and yank_unregister_function() now only run
>   once in a TLS migration.
> 
> Changes since v1:
> - Cast p->c to QIOChannelTLS into multifd_load_cleanup()
> ---
>  migration/channel.c           | 26 ++++++++++++++------------
>  migration/multifd.c           |  3 ++-
>  migration/qemu-file-channel.c |  4 +++-
>  3 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index c9ee902021..01275a9162 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -38,18 +38,19 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>      trace_migration_set_incoming_channel(
>          ioc, object_get_typename(OBJECT(ioc)));
>  
> -    if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
> -        yank_register_function(MIGRATION_YANK_INSTANCE,
> -                               migration_yank_iochannel,
> -                               QIO_CHANNEL(ioc));
> -    }
> -
>      if (s->parameters.tls_creds &&
>          *s->parameters.tls_creds &&
>          !object_dynamic_cast(OBJECT(ioc),
>                               TYPE_QIO_CHANNEL_TLS)) {
>          migration_tls_channel_process_incoming(s, ioc, &local_err);
>      } else {
> +        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +            object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> +            yank_register_function(MIGRATION_YANK_INSTANCE,
> +                                   migration_yank_iochannel,
> +                                   QIO_CHANNEL(ioc));
> +        }
> +
>          migration_ioc_process_incoming(ioc, &local_err);
>      }
>  
> @@ -76,12 +77,6 @@ void migration_channel_connect(MigrationState *s,
>          ioc, object_get_typename(OBJECT(ioc)), hostname, error);
>  
>      if (!error) {
> -        if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)) {
> -            yank_register_function(MIGRATION_YANK_INSTANCE,
> -                                   migration_yank_iochannel,
> -                                   QIO_CHANNEL(ioc));
> -        }
> -
>          if (s->parameters.tls_creds &&
>              *s->parameters.tls_creds &&
>              !object_dynamic_cast(OBJECT(ioc),
> @@ -99,6 +94,13 @@ void migration_channel_connect(MigrationState *s,
>          } else {
>              QEMUFile *f = qemu_fopen_channel_output(ioc);
>  
> +            if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +                object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) {
> +                yank_register_function(MIGRATION_YANK_INSTANCE,
> +                                       migration_yank_iochannel,
> +                                       QIO_CHANNEL(ioc));
> +            }
> +
>              qemu_mutex_lock(&s->qemu_file_lock);
>              s->to_dst_file = f;
>              qemu_mutex_unlock(&s->qemu_file_lock);
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 0a4803cfcc..2e8f001bc0 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -987,7 +987,8 @@ int multifd_load_cleanup(Error **errp)
>      for (i = 0; i < migrate_multifd_channels(); i++) {
>          MultiFDRecvParams *p = &multifd_recv_state->params[i];
>  
> -        if (object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET)
> +        if ((object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_SOCKET) ||
> +             object_dynamic_cast(OBJECT(p->c), TYPE_QIO_CHANNEL_TLS))
>              && OBJECT(p->c)->ref == 1) {
>              yank_unregister_function(MIGRATION_YANK_INSTANCE,
>                                       migration_yank_iochannel,
> diff --git a/migration/qemu-file-channel.c b/migration/qemu-file-channel.c
> index 876d05a540..fad340ea7a 100644
> --- a/migration/qemu-file-channel.c
> +++ b/migration/qemu-file-channel.c
> @@ -26,6 +26,7 @@
>  #include "qemu-file-channel.h"
>  #include "qemu-file.h"
>  #include "io/channel-socket.h"
> +#include "io/channel-tls.h"
>  #include "qemu/iov.h"
>  #include "qemu/yank.h"
>  #include "yank_functions.h"
> @@ -106,7 +107,8 @@ static int channel_close(void *opaque, Error **errp)
>      int ret;
>      QIOChannel *ioc = QIO_CHANNEL(opaque);
>      ret = qio_channel_close(ioc, errp);
> -    if (object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET)
> +    if ((object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_SOCKET) ||
> +         object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS))
>          && OBJECT(ioc)->ref == 1) {
>          yank_unregister_function(MIGRATION_YANK_INSTANCE,
>                                   migration_yank_iochannel,
> -- 
> 2.31.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Alexander Graf]

On 14.06.21 13:57, Dr. David Alan Gilbert wrote:
> cc'ing in qemu-stable - I think we'd probably want this on 6.0
> (It's currently merged as 7de2e8565335c13fb3516cddbe2e40e366cce273 ).
> Although you'll probably also want the missing dependency fix
> Philippe is working (See: 
> Mathieu- ( 42) [RFC PATCH] migration: Add missing dependency on GNUTLS )


Current master does not compile for me anymore (on macOS) due to this
change. Can we please either disable yank support and revert this patch,
pick the GNUTLS dependency patch you refer to quickly and work on a real
fix afterwards or get the proposed fix in the "missing dependency on
GNUTLS" discussion done quickly?

Having a broken tree is going to make bisection super painful later.


Alex

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Dr. David Alan Gilbert]

* Alexander Graf (agraf@csgraf.de) wrote:
> 
> On 14.06.21 13:57, Dr. David Alan Gilbert wrote:
> > cc'ing in qemu-stable - I think we'd probably want this on 6.0
> > (It's currently merged as 7de2e8565335c13fb3516cddbe2e40e366cce273 ).
> > Although you'll probably also want the missing dependency fix
> > Philippe is working (See: 
> > Mathieu- ( 42) [RFC PATCH] migration: Add missing dependency on GNUTLS )
> 
> 
> Current master does not compile for me anymore (on macOS) due to this
> change. Can we please either disable yank support and revert this patch,
> pick the GNUTLS dependency patch you refer to quickly and work on a real
> fix afterwards or get the proposed fix in the "missing dependency on
> GNUTLS" discussion done quickly?
> 
> Having a broken tree is going to make bisection super painful later.

There was Phil's dependency fix;
https://lore.kernel.org/qemu-devel/YMcTpO2SlVSc%2FHCR@redhat.com/t/

but there still seemed to be some argument if this was the right way to
do it.

Dave

> 
> Alex
> 
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Alexander Graf]

On 28.06.21 13:28, Dr. David Alan Gilbert wrote:
> * Alexander Graf (agraf@csgraf.de) wrote:
>> On 14.06.21 13:57, Dr. David Alan Gilbert wrote:
>>> cc'ing in qemu-stable - I think we'd probably want this on 6.0
>>> (It's currently merged as 7de2e8565335c13fb3516cddbe2e40e366cce273 ).
>>> Although you'll probably also want the missing dependency fix
>>> Philippe is working (See: 
>>> Mathieu- ( 42) [RFC PATCH] migration: Add missing dependency on GNUTLS )
>>
>> Current master does not compile for me anymore (on macOS) due to this
>> change. Can we please either disable yank support and revert this patch,
>> pick the GNUTLS dependency patch you refer to quickly and work on a real
>> fix afterwards or get the proposed fix in the "missing dependency on
>> GNUTLS" discussion done quickly?
>>
>> Having a broken tree is going to make bisection super painful later.
> There was Phil's dependency fix;
> https://lore.kernel.org/qemu-devel/YMcTpO2SlVSc%2FHCR@redhat.com/t/
>
> but there still seemed to be some argument if this was the right way to
> do it.


Perfect is the enemy of good :). Can we please fix the tree first, then
do "the right way"? I agree that not relying on the GNUTLS header in
that file is sensible, but it's been almost 2 weeks since that simple
one-liner, working fix was posted without anyone following up with a
more complete solution.


Alex

Re: [PATCH v3 1/1] yank: Unregister function when using TLS migration

[Daniel P. Berrangé]

On Mon, Jun 28, 2021 at 03:12:33PM +0200, Alexander Graf wrote:
> 
> On 28.06.21 13:28, Dr. David Alan Gilbert wrote:
> > * Alexander Graf (agraf@csgraf.de) wrote:
> >> On 14.06.21 13:57, Dr. David Alan Gilbert wrote:
> >>> cc'ing in qemu-stable - I think we'd probably want this on 6.0
> >>> (It's currently merged as 7de2e8565335c13fb3516cddbe2e40e366cce273 ).
> >>> Although you'll probably also want the missing dependency fix
> >>> Philippe is working (See: 
> >>> Mathieu- ( 42) [RFC PATCH] migration: Add missing dependency on GNUTLS )
> >>
> >> Current master does not compile for me anymore (on macOS) due to this
> >> change. Can we please either disable yank support and revert this patch,
> >> pick the GNUTLS dependency patch you refer to quickly and work on a real
> >> fix afterwards or get the proposed fix in the "missing dependency on
> >> GNUTLS" discussion done quickly?
> >>
> >> Having a broken tree is going to make bisection super painful later.
> > There was Phil's dependency fix;
> > https://lore.kernel.org/qemu-devel/YMcTpO2SlVSc%2FHCR@redhat.com/t/
> >
> > but there still seemed to be some argument if this was the right way to
> > do it.
> 
> 
> Perfect is the enemy of good :). Can we please fix the tree first, then
> do "the right way"? I agree that not relying on the GNUTLS header in
> that file is sensible, but it's been almost 2 weeks since that simple
> one-liner, working fix was posted without anyone following up with a
> more complete solution.

Phillipe has provided a update of the series that eliminates the
compile time dependancy on gnutls from other QEMU headers. Once
i've tested that I'll ready a pull request.

I'm happy if someone wants to merge that quick one-liner in the
meantime as a workaround.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|