migration: Use JSON null instead of "" to reset parameter to default

migrate-set-parameters sets migration parameters according to is
arguments like this:

* Present means "set the parameter to this value"

* Absent means "leave the parameter unchanged"

* Except for parameters tls_creds and tls_hostname, "" means "reset
  the parameter to its default value

The first two are perfectly normal: presence of the parameter makes
the command do something.

The third one overloads the parameter with a second meaning.  The
overloading is *implicit*, i.e. it's not visible in the types.  Works
here, because "" is neither a valid TLS credentials ID, nor a valid
host name.

Pressing argument values the schema accepts, but are semantically
invalid, into service to mean "reset to default" is not general, as
suitable invalid values need not exist.  I also find it ugly.

To clean this up, we could add a separate flag argument to ask for
"reset to default", or add a distinct value to @tls_creds and
@tls_hostname.  This commit implements the latter: add JSON null to
the values of @tls_creds and @tls_hostname, deprecate "".

Because we're so close to the 2.10 freeze, implement it in the
stupidest way possible: have qmp_migrate_set_parameters() rewrite null
to "" before anything else can see the null.  The proper way to do it
would be rewriting "" to null, but that requires fixing up code to
work with null.  Add TODO comments for that.

Signed-off-by: Markus Armbruster <[email protected]>
Reviewed-by: Daniel P. Berrange <[email protected]>
Reviewed-by: Eric Blake <[email protected]>

diff --git a/hmp.c b/hmp.c
index 4b6b1149bb4f934d797d77e978ed1b3df7d03be9..fd80dce7587c10f720d23cf0132afbbd0007679e 100644 (file)
--- a/hmp.c
+++ b/hmp.c
@@ -1586,11 +1586,15 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
                 break;
             case MIGRATION_PARAMETER_TLS_CREDS:
                 p->has_tls_creds = true;
-                visit_type_str(v, param, &p->tls_creds, &err);
+                p->tls_creds = g_new0(StrOrNull, 1);
+                p->tls_creds->type = QTYPE_QSTRING;
+                visit_type_str(v, param, &p->tls_creds->u.s, &err);
                 break;
             case MIGRATION_PARAMETER_TLS_HOSTNAME:
                 p->has_tls_hostname = true;
-                visit_type_str(v, param, &p->tls_hostname, &err);
+                p->tls_hostname = g_new0(StrOrNull, 1);
+                p->tls_hostname->type = QTYPE_QSTRING;
+                visit_type_str(v, param, &p->tls_hostname->u.s, &err);
                 break;
             case MIGRATION_PARAMETER_MAX_BANDWIDTH:
                 p->has_max_bandwidth = true;

diff --git a/migration/migration.c b/migration/migration.c
index 205b801b70d233952a7ef2bd78af1acc1e0c98f9..085c32c9945502530bbc77c642f6e2cc30313a34 100644 (file)
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -770,11 +770,13 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
     }
 
     if (params->has_tls_creds) {
-        dest->tls_creds = g_strdup(params->tls_creds);
+        assert(params->tls_creds->type == QTYPE_QSTRING);
+        dest->tls_creds = g_strdup(params->tls_creds->u.s);
     }
 
     if (params->has_tls_hostname) {
-        dest->tls_hostname = g_strdup(params->tls_hostname);
+        assert(params->tls_hostname->type == QTYPE_QSTRING);
+        dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
     }
 
     if (params->has_max_bandwidth) {
@@ -822,12 +824,14 @@ static void migrate_params_apply(MigrateSetParameters *params)
 
     if (params->has_tls_creds) {
         g_free(s->parameters.tls_creds);
-        s->parameters.tls_creds = g_strdup(params->tls_creds);
+        assert(params->tls_creds->type == QTYPE_QSTRING);
+        s->parameters.tls_creds = g_strdup(params->tls_creds->u.s);
     }
 
     if (params->has_tls_hostname) {
         g_free(s->parameters.tls_hostname);
-        s->parameters.tls_hostname = g_strdup(params->tls_hostname);
+        assert(params->tls_hostname->type == QTYPE_QSTRING);
+        s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
     }
 
     if (params->has_max_bandwidth) {
@@ -858,6 +862,21 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
 {
     MigrationParameters tmp;
 
+    /* TODO Rewrite "" to null instead */
+    if (params->has_tls_creds
+        && params->tls_creds->type == QTYPE_QNULL) {
+        QDECREF(params->tls_creds->u.n);
+        params->tls_creds->type = QTYPE_QSTRING;
+        params->tls_creds->u.s = strdup("");
+    }
+    /* TODO Rewrite "" to null instead */
+    if (params->has_tls_hostname
+        && params->tls_hostname->type == QTYPE_QNULL) {
+        QDECREF(params->tls_hostname->u.n);
+        params->tls_hostname->type = QTYPE_QSTRING;
+        params->tls_hostname->u.s = strdup("");
+    }
+
     migrate_params_test_apply(params, &tmp);
 
     if (!migrate_params_check(&tmp, errp)) {

diff --git a/qapi-schema.json b/qapi-schema.json
index 70c35412dfe0a538774546724ea3b1d24d4fa981..9c6c3e1a536c83c122683a44bc2e490a1ea39668 100644 (file)
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -115,6 +115,22 @@
 ##
 { 'command': 'qmp_capabilities' }
 
+##
+# @StrOrNull:
+#
+# This is a string value or the explicit lack of a string (null
+# pointer in C).  Intended for cases when 'optional absent' already
+# has a different meaning.
+#
+# @s: the string value
+# @n: no string value
+#
+# Since: 2.10
+##
+{ 'alternate': 'StrOrNull',
+  'data': { 's': 'str',
+            'n': 'null' } }
+
 ##
 # @LostTickPolicy:
 #
@@ -1098,8 +1114,8 @@
             '*decompress-threads': 'int',
             '*cpu-throttle-initial': 'int',
             '*cpu-throttle-increment': 'int',
-            '*tls-creds': 'str',
-            '*tls-hostname': 'str',
+            '*tls-creds': 'StrOrNull',
+            '*tls-hostname': 'StrOrNull',
             '*max-bandwidth': 'int',
             '*downtime-limit': 'int',
             '*x-checkpoint-delay': 'int',