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

[Lukas Straub]

On Wed, 26 May 2021 16:40:35 -0400
Peter Xu <peterx@redhat.com> wrote:

> On Wed, May 26, 2021 at 05:05:40PM -0300, Leonardo Bras wrote:
> > After yank feature was introduced, 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().
> > 
> > Fixes: 50186051f ("Introduce yank feature")
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1964326
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>  
> 
> Leo,
> 
> Thanks for looking into it!
> 
> So before looking int the fix... I do have a doubt on why we only enable yank
> on socket typed, as I think tls should also work with qio_channel_shutdown().
> 
> IIUC the confused thing here is we register only for qio-socket, however tls
> will actually call migration_channel_connect() twice, first with a qio-socket,
> then with the real tls-socket.  For tls I feel like we have registered with 
> the
> wrong channel - instead of the wrapper socket ioc, we should register to the
> final tls ioc?
> 
> Lukas, is there a reason?
> 

Hi,
There is no specific reason. Both ways work equally well in preventing
qemu from hanging. shutdown() for tls-channel just makes it abort a
little sooner (by not attempting to encrypt and send data anymore).

I don't lean either way. I guess registering it on the tls-channel
makes is a bit more explicit and clearer.

What do you think?

Regards,
Lukas Straub

--

pgplPQLZfuwVy.pgp
Description: OpenPGP digital signature