| Message ID | 20220106221341.8779-5-leobras@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MSG_ZEROCOPY + multifd | expand |
On Thu, Jan 06, 2022 at 07:13:41PM -0300, Leonardo Bras wrote: > void migration_channel_process_incoming(QIOChannel *ioc) > { > - MigrationState *s = migrate_get_current(); > Error *local_err = NULL; > > trace_migration_set_incoming_channel( > ioc, object_get_typename(OBJECT(ioc))); > > - if (s->parameters.tls_creds && > - *s->parameters.tls_creds && > + if (migrate_use_tls() && > !object_dynamic_cast(OBJECT(ioc), > TYPE_QIO_CHANNEL_TLS)) { > + MigrationState *s = migrate_get_current(); > + Trivial nit: I'd rather keep the line there; as the movement offers nothing, imho.. > migration_tls_channel_process_incoming(s, ioc, &local_err); > } else { > migration_ioc_register_yank(ioc); Reviewed-by: Peter Xu <peterx@redhat.com>
On Thu, Jan 06, 2022 at 07:13:41PM -0300, Leonardo Bras wrote: > A lot of places check parameters.tls_creds in order to evaluate if TLS is > in use, and sometimes call migrate_get_current() just for that test. > > Add new helper function migrate_use_tls() in order to simplify testing > for TLS usage. > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > Reviewed-by: Juan Quintela <quintela@redhat.com> > --- > migration/migration.h | 1 + > migration/channel.c | 6 +++--- > migration/migration.c | 9 +++++++++ > migration/multifd.c | 5 +---- > 4 files changed, 14 insertions(+), 7 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
Hello Peter, On Thu, Jan 13, 2022 at 4:02 AM Peter Xu <peterx@redhat.com> wrote: > > On Thu, Jan 06, 2022 at 07:13:41PM -0300, Leonardo Bras wrote: > > void migration_channel_process_incoming(QIOChannel *ioc) > > { > > - MigrationState *s = migrate_get_current(); > > Error *local_err = NULL; > > > > trace_migration_set_incoming_channel( > > ioc, object_get_typename(OBJECT(ioc))); > > > > - if (s->parameters.tls_creds && > > - *s->parameters.tls_creds && > > + if (migrate_use_tls() && > > !object_dynamic_cast(OBJECT(ioc), > > TYPE_QIO_CHANNEL_TLS)) { > > + MigrationState *s = migrate_get_current(); > > + > > Trivial nit: I'd rather keep the line there; as the movement offers nothing, > imho.. The idea to move the 's' to inside the if block is to make it clear it's only used in this case. But if you think it's better to keep it at the beginning of the function, sure, I can change that. Just let me know. > > > migration_tls_channel_process_incoming(s, ioc, &local_err); > > } else { > > migration_ioc_register_yank(ioc); > > Reviewed-by: Peter Xu <peterx@redhat.com> > Thanks! > -- > Peter Xu > Best regards, Leo
Hello Daniel, On Thu, Jan 13, 2022 at 10:11 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Thu, Jan 06, 2022 at 07:13:41PM -0300, Leonardo Bras wrote: > > A lot of places check parameters.tls_creds in order to evaluate if TLS is > > in use, and sometimes call migrate_get_current() just for that test. > > > > Add new helper function migrate_use_tls() in order to simplify testing > > for TLS usage. > > > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > --- > > migration/migration.h | 1 + > > migration/channel.c | 6 +++--- > > migration/migration.c | 9 +++++++++ > > migration/multifd.c | 5 +---- > > 4 files changed, 14 insertions(+), 7 deletions(-) > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > Thanks! > 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 :| > Best regards, Leo
On Wed, Jan 19, 2022 at 03:06:55PM -0300, Leonardo Bras Soares Passos wrote: > Hello Peter, > > On Thu, Jan 13, 2022 at 4:02 AM Peter Xu <peterx@redhat.com> wrote: > > > > On Thu, Jan 06, 2022 at 07:13:41PM -0300, Leonardo Bras wrote: > > > void migration_channel_process_incoming(QIOChannel *ioc) > > > { > > > - MigrationState *s = migrate_get_current(); > > > Error *local_err = NULL; > > > > > > trace_migration_set_incoming_channel( > > > ioc, object_get_typename(OBJECT(ioc))); > > > > > > - if (s->parameters.tls_creds && > > > - *s->parameters.tls_creds && > > > + if (migrate_use_tls() && > > > !object_dynamic_cast(OBJECT(ioc), > > > TYPE_QIO_CHANNEL_TLS)) { > > > + MigrationState *s = migrate_get_current(); > > > + > > > > Trivial nit: I'd rather keep the line there; as the movement offers nothing, > > imho.. > > The idea to move the 's' to inside the if block is to make it clear > it's only used in this case. IMHO not necessary; I hardly read declarations for this, unless there's a bug, e.g. on variable shadowing. Moving it downwards makes it easier to happen. :) > > But if you think it's better to keep it at the beginning of the > function, sure, I can change that. > Just let me know. Since there'll be a new version, that definitely looks nicer. Thanks,
diff --git a/migration/migration.h b/migration/migration.h index 1489eeb165..445d95bbf2 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -344,6 +344,7 @@ bool migrate_use_zero_copy(void); #else #define migrate_use_zero_copy() (false) #endif +int migrate_use_tls(void); int migrate_use_xbzrle(void); uint64_t migrate_xbzrle_cache_size(void); bool migrate_colo_enabled(void); diff --git a/migration/channel.c b/migration/channel.c index c4fc000a1a..1a45b75d29 100644 --- a/migration/channel.c +++ b/migration/channel.c @@ -32,16 +32,16 @@ */ void migration_channel_process_incoming(QIOChannel *ioc) { - MigrationState *s = migrate_get_current(); Error *local_err = NULL; trace_migration_set_incoming_channel( ioc, object_get_typename(OBJECT(ioc))); - if (s->parameters.tls_creds && - *s->parameters.tls_creds && + if (migrate_use_tls() && !object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) { + MigrationState *s = migrate_get_current(); + migration_tls_channel_process_incoming(s, ioc, &local_err); } else { migration_ioc_register_yank(ioc); diff --git a/migration/migration.c b/migration/migration.c index aa8f1dc835..7bcb800890 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -2573,6 +2573,15 @@ bool migrate_use_zero_copy(void) } #endif +int migrate_use_tls(void) +{ + MigrationState *s; + + s = migrate_get_current(); + + return s->parameters.tls_creds && *s->parameters.tls_creds; +} + int migrate_use_xbzrle(void) { MigrationState *s; diff --git a/migration/multifd.c b/migration/multifd.c index 3242f688e5..677e942747 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -796,14 +796,11 @@ 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)), p->tls_hostname, error); if (!error) { - if (s->parameters.tls_creds && - *s->parameters.tls_creds && + if (migrate_use_tls() && !object_dynamic_cast(OBJECT(ioc), TYPE_QIO_CHANNEL_TLS)) { multifd_tls_channel_connect(p, ioc, &error);