| Message ID | 20220331150857.74406-15-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Postcopy Preemption | expand |
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 ? > + 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()' ? > > /* Sending on the return path - generic and then for each message type */ > void migrate_send_rp_shut(MigrationIncomingState *mis, > diff --git a/migration/multifd.c b/migration/multifd.c > index 9ea4f581e2..19e3c44491 100644 > --- a/migration/multifd.c > +++ b/migration/multifd.c > @@ -782,17 +782,12 @@ static bool multifd_channel_connect(MultiFDSendParams *p, > QIOChannel *ioc, > Error *error) > { > - MigrationState *s = migrate_get_current(); > - > trace_multifd_set_outgoing_channel( > ioc, object_get_typename(OBJECT(ioc)), > migrate_get_current()->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)) { > multifd_tls_channel_connect(p, ioc, &error); > if (!error) { > /* > -- > 2.32.0 > With regards, Daniel
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,
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()) { + 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); /* Sending on the return path - generic and then for each message type */ void migrate_send_rp_shut(MigrationIncomingState *mis, diff --git a/migration/multifd.c b/migration/multifd.c index 9ea4f581e2..19e3c44491 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -782,17 +782,12 @@ static bool multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc, Error *error) { - MigrationState *s = migrate_get_current(); - trace_multifd_set_outgoing_channel( ioc, object_get_typename(OBJECT(ioc)), migrate_get_current()->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)) { multifd_tls_channel_connect(p, ioc, &error); if (!error) { /*
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(-)