Re: [PATCH v7 4/5] migration: Add migrate_use_tls() helper
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 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,
--
Peter Xu
Re: [PATCH v7 4/5] migration: Add migrate_use_tls() helper
Hello Daniel, On Thu, Jan 13, 2022 at 10:11 AM Daniel P. Berrangé 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 > > Reviewed-by: Juan Quintela > > --- > > 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é > 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
Re: [PATCH v7 4/5] migration: Add migrate_use_tls() helper
Hello Peter,
On Thu, Jan 13, 2022 at 4:02 AM Peter Xu 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, _err);
> > } else {
> > migration_ioc_register_yank(ioc);
>
> Reviewed-by: Peter Xu
>
Thanks!
> --
> Peter Xu
>
Best regards,
Leo
Re: [PATCH v7 4/5] migration: Add migrate_use_tls() helper
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 > Reviewed-by: Juan Quintela > --- > 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é 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 :|
Re: [PATCH v7 4/5] migration: Add migrate_use_tls() helper
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, _err);
> } else {
> migration_ioc_register_yank(ioc);
Reviewed-by: Peter Xu
--
Peter Xu
[PATCH v7 4/5] migration: Add migrate_use_tls() helper
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
Reviewed-by: Juan Quintela
---
migration/migration.h | 1 +
migration/channel.c | 6 +++---
migration/migration.c | 9 +
migration/multifd.c | 5 +
4 files changed, 14 insertions(+), 7 deletions(-)
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, _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, );
--
2.34.1
