Re: [PATCH v6 07/13] migration: Add helpers to detect TLS capability

From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: qemu-devel@nongnu.org,
	Leonardo Bras Soares Passos <lsoaresp@redhat.com>,
	Juan Quintela <quintela@redhat.com>,
	Manish Mishra <manish.mishra@nutanix.com>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH v6 07/13] migration: Add helpers to detect TLS capability
Date: Wed, 18 May 2022 09:57:12 +0100	[thread overview]
Message-ID: <YoS06H6I0fZx/uAC@redhat.com> (raw)
In-Reply-To: <20220517195730.32312-8-peterx@redhat.com>

On Tue, May 17, 2022 at 03:57:24PM -0400, Peter Xu wrote:
> Add migrate_channel_requires_tls() to detect whether the specific channel
> requires TLS, leveraging the recently introduced migrate_use_tls().  No
> functional change intended.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/channel.c   | 9 ++-------
>  migration/migration.c | 1 +
>  migration/multifd.c   | 4 +---
>  migration/tls.c       | 9 +++++++++
>  migration/tls.h       | 4 ++++
>  5 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/channel.c b/migration/channel.c
> index a162d00fea..36e59eaeec 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -38,9 +38,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
>      trace_migration_set_incoming_channel(
>          ioc, object_get_typename(OBJECT(ioc)));
>  
> -    if (migrate_use_tls() &&
> -        !object_dynamic_cast(OBJECT(ioc),
> -                             TYPE_QIO_CHANNEL_TLS)) {
> +    if (migrate_channel_requires_tls(ioc)) {
>          migration_tls_channel_process_incoming(s, ioc, &local_err);
>      } else {
>          migration_ioc_register_yank(ioc);
> @@ -70,10 +68,7 @@ void migration_channel_connect(MigrationState *s,
>          ioc, object_get_typename(OBJECT(ioc)), hostname, error);
>  
>      if (!error) {
> -        if (s->parameters.tls_creds &&
> -            *s->parameters.tls_creds &&
> -            !object_dynamic_cast(OBJECT(ioc),
> -                                 TYPE_QIO_CHANNEL_TLS)) {
> +        if (migrate_channel_requires_tls(ioc)) {
>              migration_tls_channel_connect(s, ioc, hostname, &error);
>  
>              if (!error) {
> diff --git a/migration/migration.c b/migration/migration.c
> index f5f7a0f91f..d17f435d08 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -49,6 +49,7 @@
>  #include "trace.h"
>  #include "exec/target_page.h"
>  #include "io/channel-buffer.h"
> +#include "io/channel-tls.h"
>  #include "migration/colo.h"
>  #include "hw/boards.h"
>  #include "hw/qdev-properties.h"
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 9282ab6aa4..849c116ce4 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -831,9 +831,7 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>          migrate_get_current()->hostname, error);
>  
>      if (!error) {
> -        if (migrate_use_tls() &&
> -            !object_dynamic_cast(OBJECT(ioc),
> -                                 TYPE_QIO_CHANNEL_TLS)) {
> +        if (migrate_channel_requires_tls(ioc)) {
>              multifd_tls_channel_connect(p, ioc, &error);
>              if (!error) {
>                  /*
> diff --git a/migration/tls.c b/migration/tls.c
> index 32c384a8b6..1baa662489 100644
> --- a/migration/tls.c
> +++ b/migration/tls.c
> @@ -166,3 +166,12 @@ void migration_tls_channel_connect(MigrationState *s,
>                                NULL,
>                                NULL);
>  }
> +
> +bool migrate_channel_requires_tls(QIOChannel *ioc)
> +{
> +    if (!migrate_use_tls()) {
> +        return false;
> +    }
> +
> +    return !object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS);
> +}
> diff --git a/migration/tls.h b/migration/tls.h
> index de4fe2cafd..a54c1dcec7 100644
> --- a/migration/tls.h
> +++ b/migration/tls.h
> @@ -37,4 +37,8 @@ void migration_tls_channel_connect(MigrationState *s,
>                                     QIOChannel *ioc,
>                                     const char *hostname,
>                                     Error **errp);
> +
> +/* Whether the QIO channel requires further TLS handshake? */
> +bool migrate_channel_requires_tls(QIOChannel *ioc);

I find this name somewhat confusing, as 'requires tls' and
'uses tls' are just synonyms for the same thing IMHO.

What this method is actually checking is whether we still need
to upgrade the channel from plain text to TLS, by completing a
TLS handshake. So can we call this:

  migrate_channel_requires_tls_upgrade

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 :|