[RFC,02/13] migration: Normalize tls arguments

Commit Message

Fabiano Rosas April 11, 2025, 7:14 p.m. UTC

The tls_creds, tls_authz and tls_hostname arguments are strings that
can be set by the user. They are allowed to be either a valid string,
an empty string or NULL. The values "" and NULL are effectively
treated the same by the code, but this is not entirely clear because
the handling is not uniform.

Make the 3 variables be handled the same and at the same place in
options.c. Note that this affects only the internal usage of the
variables.

(migrate_tls() had to be moved to be able to use migrate_tls_creds())

Signed-off-by: Fabiano Rosas <[e-mail]>
---
 migration/options.c | 81 ++++++++++++++++++++++++---------------------
 migration/tls.c     |  2 +-
 2 files changed, 44 insertions(+), 39 deletions(-)

Comments

Daniel P. Berrangé April 14, 2025, 4:30 p.m. UTC | #1

On Fri, Apr 11, 2025 at 04:14:32PM -0300, Fabiano Rosas wrote:
> The tls_creds, tls_authz and tls_hostname arguments are strings that
> can be set by the user. They are allowed to be either a valid string,
> an empty string or NULL. The values "" and NULL are effectively
> treated the same by the code, but this is not entirely clear because
> the handling is not uniform.
> 
> Make the 3 variables be handled the same and at the same place in
> options.c. Note that this affects only the internal usage of the
> variables.
> 
> (migrate_tls() had to be moved to be able to use migrate_tls_creds())
> 
> Signed-off-by: Fabiano Rosas <[e-mail]>
> ---
>  migration/options.c | 81 ++++++++++++++++++++++++---------------------
>  migration/tls.c     |  2 +-
>  2 files changed, 44 insertions(+), 39 deletions(-)
> 
> diff --git a/migration/options.c b/migration/options.c
> index cb8eec218f..7cd465ca94 100644
> --- a/migration/options.c
> +++ b/migration/options.c


> @@ -1184,18 +1200,27 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>      }
>  
>      if (params->tls_creds) {
> -        assert(params->tls_creds->type == QTYPE_QSTRING);
> -        dest->tls_creds = params->tls_creds->u.s;
> +        if (params->tls_creds->type == QTYPE_QNULL) {
> +            dest->tls_creds = NULL;
> +        } else {
> +            dest->tls_creds = params->tls_creds->u.s;
> +        }

Feels like it is still worth having the assert(QTYPE_QSTRING)
in the else branch before we blindly reference the string pointer

>      }
>  
>      if (params->tls_hostname) {
> -        assert(params->tls_hostname->type == QTYPE_QSTRING);
> -        dest->tls_hostname = params->tls_hostname->u.s;
> +        if (params->tls_hostname->type == QTYPE_QNULL) {
> +            dest->tls_hostname = NULL;
> +        } else {
> +            dest->tls_hostname = params->tls_hostname->u.s;
> +        }
>      }
>  
>      if (params->tls_authz) {
> -        assert(params->tls_authz->type == QTYPE_QSTRING);
> -        dest->tls_authz = params->tls_authz->u.s;
> +        if (params->tls_authz->type == QTYPE_QNULL) {
> +            dest->tls_authz = NULL;
> +        } else {
> +            dest->tls_authz = params->tls_authz->u.s;
> +        }
>      }
>  
>      if (params->has_max_bandwidth) {

With regards,
Daniel

Patch

diff --git a/migration/options.c b/migration/options.c
index cb8eec218f..7cd465ca94 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -379,13 +379,6 @@  bool migrate_rdma(void)
     return s->rdma_migration;
 }
 
-bool migrate_tls(void)
-{
-    MigrationState *s = migrate_get_current();
-
-    return s->parameters.tls_creds && *s->parameters.tls_creds;
-}
-
 typedef enum WriteTrackingSupport {
     WT_SUPPORT_UNKNOWN = 0,
     WT_SUPPORT_ABSENT,
@@ -814,21 +807,41 @@  const char *migrate_tls_authz(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->parameters.tls_authz;
+    if (s->parameters.tls_authz &&
+        *s->parameters.tls_authz) {
+        return s->parameters.tls_authz;
+    }
+
+    return NULL;
 }
 
 const char *migrate_tls_creds(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->parameters.tls_creds;
+    if (s->parameters.tls_creds &&
+        *s->parameters.tls_creds) {
+        return s->parameters.tls_creds;
+    }
+
+    return NULL;
 }
 
 const char *migrate_tls_hostname(void)
 {
     MigrationState *s = migrate_get_current();
 
-    return s->parameters.tls_hostname;
+    if (s->parameters.tls_hostname &&
+        *s->parameters.tls_hostname) {
+        return s->parameters.tls_hostname;
+    }
+
+    return NULL;
+}
+
+bool migrate_tls(void)
+{
+    return !!migrate_tls_creds();
 }
 
 uint64_t migrate_vcpu_dirty_limit_period(void)
@@ -883,8 +896,10 @@  MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
     params->has_cpu_throttle_tailslow = true;
     params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow;
-    params->tls_creds = g_strdup(s->parameters.tls_creds);
-    params->tls_hostname = g_strdup(s->parameters.tls_hostname);
+    params->tls_creds = g_strdup(s->parameters.tls_creds ?
+                                 s->parameters.tls_creds : "");
+    params->tls_hostname = g_strdup(s->parameters.tls_hostname ?
+                                    s->parameters.tls_hostname : "");
     params->tls_authz = g_strdup(s->parameters.tls_authz ?
                                  s->parameters.tls_authz : "");
     params->has_max_bandwidth = true;
@@ -945,6 +960,7 @@  void migrate_params_init(MigrationParameters *params)
 {
     params->tls_hostname = g_strdup("");
     params->tls_creds = g_strdup("");
+    params->tls_authz = g_strdup("");
 
     /* Set has_* up only for parameter checks */
     params->has_throttle_trigger_threshold = true;
@@ -1184,18 +1200,27 @@  static void migrate_params_test_apply(MigrateSetParameters *params,
     }
 
     if (params->tls_creds) {
-        assert(params->tls_creds->type == QTYPE_QSTRING);
-        dest->tls_creds = params->tls_creds->u.s;
+        if (params->tls_creds->type == QTYPE_QNULL) {
+            dest->tls_creds = NULL;
+        } else {
+            dest->tls_creds = params->tls_creds->u.s;
+        }
     }
 
     if (params->tls_hostname) {
-        assert(params->tls_hostname->type == QTYPE_QSTRING);
-        dest->tls_hostname = params->tls_hostname->u.s;
+        if (params->tls_hostname->type == QTYPE_QNULL) {
+            dest->tls_hostname = NULL;
+        } else {
+            dest->tls_hostname = params->tls_hostname->u.s;
+        }
     }
 
     if (params->tls_authz) {
-        assert(params->tls_authz->type == QTYPE_QSTRING);
-        dest->tls_authz = params->tls_authz->u.s;
+        if (params->tls_authz->type == QTYPE_QNULL) {
+            dest->tls_authz = NULL;
+        } else {
+            dest->tls_authz = params->tls_authz->u.s;
+        }
     }
 
     if (params->has_max_bandwidth) {
@@ -1413,26 +1438,6 @@  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
 {
     MigrationParameters tmp;
 
-    /* TODO Rewrite "" to null instead for all three tls_* parameters */
-    if (params->tls_creds
-        && params->tls_creds->type == QTYPE_QNULL) {
-        qobject_unref(params->tls_creds->u.n);
-        params->tls_creds->type = QTYPE_QSTRING;
-        params->tls_creds->u.s = strdup("");
-    }
-    if (params->tls_hostname
-        && params->tls_hostname->type == QTYPE_QNULL) {
-        qobject_unref(params->tls_hostname->u.n);
-        params->tls_hostname->type = QTYPE_QSTRING;
-        params->tls_hostname->u.s = strdup("");
-    }
-    if (params->tls_authz
-        && params->tls_authz->type == QTYPE_QNULL) {
-        qobject_unref(params->tls_authz->u.n);
-        params->tls_authz->type = QTYPE_QSTRING;
-        params->tls_authz->u.s = strdup("");
-    }
-
     migrate_params_test_apply(params, &tmp);
 
     if (!migrate_params_check(&tmp, errp)) {
diff --git a/migration/tls.c b/migration/tls.c
index 5cbf952383..8a89d3f767 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -126,7 +126,7 @@  QIOChannelTLS *migration_tls_client_create(QIOChannel *ioc,
     }
 
     const char *tls_hostname = migrate_tls_hostname();
-    if (tls_hostname && *tls_hostname) {
+    if (tls_hostname) {
         hostname = tls_hostname;
     }