Re: [PATCH] migration/multifd/tls: Cleanup BYE message processing on sender side

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> This patch is a trivial cleanup to the BYE messages on the multifd sender
> side.  It could also be a fix, but since we do not have a solid clue,
> taking this as a cleanup only.
>
> One trivial concern is, migration_tls_channel_end() might be unsafe to be
> invoked in the migration thread if migration is not successful, because
> when failed / cancelled we do not know whether the multifd sender threads
> can be writting to the channels, while GnuTLS library (when it's a TLS
> channel) logically doesn't support concurrent writes.
>
> When at it, cleanup on a few things.  What changed:
>
>   - Introduce a helper to do graceful shutdowns with rich comment, hiding
>     the details
>
>   - Only send bye() iff migration succeeded, skip if it failed / cancelled
>
>   - Detect TLS channel using channel type rather than thread created flags
>
>   - Move the loop into the existing one that will close the channels, but
>     do graceful shutdowns before channel shutdowns
>
>   - local_err seems to have been leaked if set, fix it along the way
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/multifd.c | 65 ++++++++++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index b255778855..98873cee74 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -439,6 +439,39 @@ static void multifd_send_set_error(Error *err)
>      }
>  }
>  
> +/*
> + * Gracefully shutdown IOChannels. Only needed for successful migrations on
> + * top of TLS channels.  Otherwise it is same to qio_channel_shutdown().
> + *
> + * A successful migration also guarantees multifd sender threads are

s/also//

> + * properly flushed and halted.  It is only safe to send BYE in the
> + * migration thread here when we know there's no other thread writting to

s/writting/writing/

> + * the channel, because GnuTLS doesn't support concurrent writers.
> + */
> +static void migration_ioc_shutdown_gracefully(QIOChannel *ioc)

Reviewed-by: Fabiano Rosas <farosas@suse.de>