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

[Lukas Straub]

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

--

pgpjVSpy9aUAl.pgp
Description: OpenPGP digital signature