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