Re: [PATCH v2] migration/multifd: close TLS channel before socket finalize

[Zheng Chuan]

On 2020/11/10 18:12, Daniel P. Berrangé wrote:
> On Fri, Nov 06, 2020 at 06:54:54PM +0800, Chuan Zheng wrote:
>> Since we now support tls multifd, when we cancel migration, the TLS
>> sockets will be left as CLOSE-WAIT On Src which results in socket
>> leak.
>> Fix it by closing TLS channel before socket finalize.
>>
>> Signed-off-by: Chuan Zheng <zhengchuan@huawei.com>
>> ---
>>  migration/multifd.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index 68b171f..a6838dc 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -523,6 +523,19 @@ static void multifd_send_terminate_threads(Error *err)
>>      }
>>  }
>>  
>> +static void multifd_tls_socket_close(QIOChannel *ioc, Error *err)
>> +{
>> +    if (ioc &&
>> +        object_dynamic_cast(OBJECT(ioc),
>> +                            TYPE_QIO_CHANNEL_TLS)) {
>> +        /*
>> +         * TLS channel is special, we need close it before
>> +         * socket finalize.
>> +         */
>> +        qio_channel_close(ioc, &err);
>> +    }
>> +}
> 
> This doesn't feel quite right to me.  Calling qio_channel_close will close
> both the TLS layer, and the underlying QIOChannelSocket. If the latter
> is safe to do, then we don't need the object_dynamic_cast() check, we can
> do it unconditionally whether we're using TLS or not.
> 
> Having said that, I'm not sure if we actually want to be using
> qio_channel_close or not ?
> 
> I would have expected that there is already code somewhere else in the
> migration layer that is closing these multifd channels, but I can't
> actually find where that happens right now.  Assuming that code does
> exist though, qio_channel_shutdown(ioc, BOTH) feels like the right
> answer to unblock waiting I/O ops.
> 
Hi, Daniel.
Actually, I have tried to use qio_channel_shutdown at the same place, but it 
seems not work right.
the socket connection is closed by observing through 'ss' command but the 
socket fds in /proc/$(qemu pid)/fd are still
residual.

The underlying QIOChannelSocket will be closed by qio_channel_socket_finalize() 
through object_unref(QIOChannel) later in socket_send_channel_destroy(),
does that means it is safe to close both of TLS and tcp socket?


>> +
>>  void multifd_save_cleanup(void)
>>  {
>>      int i;
>> @@ -542,6 +555,7 @@ void multifd_save_cleanup(void)
>>          MultiFDSendParams *p = &multifd_send_state->params[i];
>>          Error *local_err = NULL;
>>  
>> +        multifd_tls_socket_close(p->c, NULL);
>>          socket_send_channel_destroy(p->c);
>>          p->c = NULL;
>>          qemu_mutex_destroy(&p->mutex);
>> -- 
>> 1.8.3.1
>>
> 
> Regards,
> Daniel
> 

-- 
Regards.
Chuan