Re: [PATCH v4 14/19] migration: Add helpers to detect TLS capability

[Peter Xu]

On Wed, Apr 20, 2022 at 12:10:14PM +0100, Daniel P. Berrangé wrote:
> On Thu, Mar 31, 2022 at 11:08:52AM -0400, Peter Xu wrote:
> > Add migrate_tls_enabled() to detect whether TLS is configured.
> > 
> > Add migrate_channel_requires_tls() to detect whether the specific channel
> > requires TLS.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/channel.c   | 10 ++--------
> >  migration/migration.c | 17 +++++++++++++++++
> >  migration/migration.h |  4 ++++
> >  migration/multifd.c   |  7 +------
> >  4 files changed, 24 insertions(+), 14 deletions(-)
> > 
> > diff --git a/migration/channel.c b/migration/channel.c
> > index c6a8dcf1d7..36e59eaeec 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -38,10 +38,7 @@ void migration_channel_process_incoming(QIOChannel *ioc)
> >      trace_migration_set_incoming_channel(
> >          ioc, object_get_typename(OBJECT(ioc)));
> >  
> > -    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_process_incoming(s, ioc, &local_err);
> >      } else {
> >          migration_ioc_register_yank(ioc);
> > @@ -71,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 ee3df9e229..899084f993 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"
> > @@ -4251,6 +4252,22 @@ void migration_global_dump(Monitor *mon)
> >                     ms->clear_bitmap_shift);
> >  }
> >  
> > +bool migrate_tls_enabled(void)
> > +{
> > +    MigrationState *s = migrate_get_current();
> > +
> > +    return s->parameters.tls_creds && *s->parameters.tls_creds;
> > +}
> > +
> > +bool migrate_channel_requires_tls(QIOChannel *ioc)
> > +{
> > +    if (!migrate_tls_enabled()) {
> 
> This is the only place migrate_tls_enabled is called. Does it
> really need to exist as an exported method, as opposed to
> inlining it here ?

IMHO the helper could help code readers to easier understand when TLS is
enabled, and it's not super obvious as TLS doesn't have a capability bit
bound to it.  No strong opinions, though.

> 
> > +        return false;
> > +    }
> > +
> > +    return !object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS);
> > +}
> > +
> >  #define DEFINE_PROP_MIG_CAP(name, x)             \
> >      DEFINE_PROP_BOOL(name, MigrationState, enabled_capabilities[x], false)
> >  
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 6ee520642f..8b9ad7fe31 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -436,6 +436,10 @@ bool migrate_use_events(void);
> >  bool migrate_postcopy_blocktime(void);
> >  bool migrate_background_snapshot(void);
> >  bool migrate_postcopy_preempt(void);
> > +/* Whether TLS is enabled for migration? */
> > +bool migrate_tls_enabled(void);
> > +/* Whether the QIO channel requires further TLS handshake? */
> > +bool migrate_channel_requires_tls(QIOChannel *ioc);
> 
> How about having it in tls.{c,h} as  'migration_tls_channel_enabled()' ?

I can do the movement, but the new name can be confusing when we read it in
the codes, it'll look like:

  if (migration_tls_channel_enabled(ioc)) {
    /* create the tls channel */
    ...
  }

The thing is migration_tls_channel_enabled() on a TLS channel will return
false.. which seems to be against the gut feelings.

migrate_channel_requires_tls() feels better but maybe not so much..
Would migrate_channel_requires_tls_wrapper() be better (but longer..)?

Thanks,

-- 
Peter Xu