Re: [PATCH v4 16/19] migration: Enable TLS for preempt channel

[Daniel P . Berrangé]

On Thu, Mar 31, 2022 at 11:08:54AM -0400, Peter Xu wrote:
> This patch is based on the async preempt channel creation.  It continues
> wiring up the new channel with TLS handshake to destionation when enabled.
> 
> Note that only the src QEMU needs such operation; the dest QEMU does not
> need any change for TLS support due to the fact that all channels are
> established synchronously there, so all the TLS magic is already properly
> handled by migration_tls_channel_process_incoming().
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/postcopy-ram.c | 60 +++++++++++++++++++++++++++++++++++-----
>  migration/trace-events   |  1 +
>  2 files changed, 54 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index ab2a50cf45..f5ba176862 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -36,6 +36,7 @@
>  #include "socket.h"
>  #include "qemu-file-channel.h"
>  #include "yank_functions.h"
> +#include "tls.h"
>  
>  /* Arbitrary limit on size of each discard command,
>   * keeps them around ~200 bytes
> @@ -1552,15 +1553,15 @@ bool 
> postcopy_preempt_new_channel(MigrationIncomingState *mis, QEMUFile *file)
>      return true;
>  }
>  
> +/*
> + * Setup the postcopy preempt channel with the IOC.  If ERROR is specified,
> + * setup the error instead.  This helper will free the ERROR if specified.
> + */
>  static void
> -postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque)
> +postcopy_preempt_send_channel_done(MigrationState *s,
> +                                   QIOChannel *ioc, Error *local_err)
>  {
> -    MigrationState *s = opaque;
> -    QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
> -    Error *local_err = NULL;
> -
> -    if (qio_task_propagate_error(task, &local_err)) {
> -        /* Something wrong happened.. */
> +    if (local_err) {
>          migrate_set_error(s, local_err);
>          error_free(local_err);
>      } else {
> @@ -1574,6 +1575,51 @@ postcopy_preempt_send_channel_new(QIOTask *task, 
> gpointer opaque)
>       * postcopy_qemufile_src to know whether it failed or not.
>       */
>      qemu_sem_post(&s->postcopy_qemufile_src_sem);
> +}
> +
> +static void
> +postcopy_preempt_tls_handshake(QIOTask *task, gpointer opaque)
> +{
> +    MigrationState *s = opaque;
> +    QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));

If using g_autoptr(QIOChannel) ioc = ...

> +    Error *err = NULL;

local_err is normal naming 

> +
> +    qio_task_propagate_error(task, &err);
> +    postcopy_preempt_send_channel_done(s, ioc, err);
> +    object_unref(OBJECT(ioc));

...not needed with g_autoptr

> +}
> +
> +static void
> +postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque)
> +{
> +    MigrationState *s = opaque;
> +    QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));

If you use g_autoptr(QIOChannel)

> +    QIOChannelTLS *tioc;
> +    Error *local_err = NULL;
> +
> +    if (qio_task_propagate_error(task, &local_err)) {
> +        assert(local_err);

I don't think we really need to add these asserts everywhere we
handle a failure path do we ?

> +        goto out;
> +    }
> +
> +    if (migrate_channel_requires_tls(ioc)) {
> +        tioc = migration_tls_client_create(s, ioc, s->hostname, &local_err);
> +        if (!tioc) {
> +            assert(local_err);
> +            goto out;
> +        }
> +        trace_postcopy_preempt_tls_handshake();
> +        qio_channel_set_name(QIO_CHANNEL(tioc), "migration-tls-preempt");
> +        qio_channel_tls_handshake(tioc, postcopy_preempt_tls_handshake,
> +                                  s, NULL, NULL);
> +        /* Setup the channel until TLS handshake finished */
> +        object_unref(OBJECT(ioc));

...not needed with g_autoptr

> +        return;
> +    }
> +
> +out:
> +    /* This handles both good and error cases */
> +    postcopy_preempt_send_channel_done(s, ioc, local_err);
>      object_unref(OBJECT(ioc));

...also not needed with g_autoptr

>  }
>  
> diff --git a/migration/trace-events b/migration/trace-events
> index b21d5f371f..00ab2e1b96 100644
> --- a/migration/trace-events
> +++ b/migration/trace-events
> @@ -287,6 +287,7 @@ postcopy_request_shared_page(const char *sharer, const 
> char *rb, uint64_t rb_off
>  postcopy_request_shared_page_present(const char *sharer, const char *rb, 
> uint64_t rb_offset) "%s already %s offset 0x%"PRIx64
>  postcopy_wake_shared(uint64_t client_addr, const char *rb) "at 0x%"PRIx64" 
> in %s"
>  postcopy_page_req_del(void *addr, int count) "resolved page req %p total %d"
> +postcopy_preempt_tls_handshake(void) ""
>  postcopy_preempt_new_channel(void) ""
>  postcopy_preempt_thread_entry(void) ""
>  postcopy_preempt_thread_exit(void) ""
> -- 
> 2.32.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|