[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH 1/6] migration: Fix and clean up around @tls-authz
From: |
Markus Armbruster |
Subject: |
[PATCH 1/6] migration: Fix and clean up around @tls-authz |
Date: |
Fri, 13 Nov 2020 07:52:31 +0100 |
Commit d2f1d29b95 "migration: add support for a "tls-authz" migration
parameter" added MigrationParameters member @tls-authz. Whereas the
other members aren't really optional (see commit 1bda8b3c695), this
one is genuinely optional: migration_instance_init() leaves it absent,
and migration_tls_channel_process_incoming() passes it to
qcrypto_tls_session_new(), which checks for null.
Commit d2f1d29b95 has a number of issues, though:
* When qmp_query_migrate_parameters() copies migration parameters into
its reply, it ignores has_tls_authz, and assumes true instead. When
it is false,
- HMP info migrate_parameters prints the null pointer (crash bug on
some systems), and
- QMP query-migrate-parameters replies "tls-authz": "" (because the
QObject output visitor silently maps null pointer to "", which it
really shouldn't).
The HMP defect was noticed and fixed in commit 7cd75cbdb8
'migration: use "" instead of (null) for tls-authz'. Unfortunately,
the fix papered over the real bug: it made
qmp_query_migrate_parameters() map null tls_authz to "". It also
dropped the check for has_tls_authz from
hmp_info_migrate_parameters().
Revert, and fix qmp_query_migrate_parameters() not to screw up
has_tls_authz. No change to HMP. QMP now has "tls-authz" in the
reply only when it's actually present in
migrate_get_current()->parameters. If we prefer to remain
bug-compatible, we should make tls_authz non-optional there.
* migrate_params_test_apply() neglects to apply tls_authz. Currently
harmless, because migrate_params_check() doesn't care. Fix it
anyway.
* qmp_migrate_set_parameters() crashes:
{"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}
Add the necessary rewrite of null to "". For background
information, see commit 01fa559826 "migration: Use JSON null instead
of "" to reset parameter to default".
Fixes: d2f1d29b95aa45d13262b39153ff501ed6b1ac95
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
qapi/migration.json | 2 +-
migration/migration.c | 17 ++++++++++++++---
monitor/hmp-cmds.c | 2 +-
3 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index 3c75820527..688e8da749 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -928,7 +928,7 @@
##
# @MigrationParameters:
#
-# The optional members aren't actually optional.
+# The optional members aren't actually optional, except for @tls-authz.
#
# @announce-initial: Initial delay (in milliseconds) before sending the
# first announce (Since 4.0)
diff --git a/migration/migration.c b/migration/migration.c
index 3263aa55a9..cad56fbf8c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -855,9 +855,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error
**errp)
params->tls_creds = g_strdup(s->parameters.tls_creds);
params->has_tls_hostname = true;
params->tls_hostname = g_strdup(s->parameters.tls_hostname);
- params->has_tls_authz = true;
- params->tls_authz = g_strdup(s->parameters.tls_authz ?
- s->parameters.tls_authz : "");
+ params->has_tls_authz = s->parameters.has_tls_authz;
+ params->tls_authz = g_strdup(s->parameters.tls_authz);
params->has_max_bandwidth = true;
params->max_bandwidth = s->parameters.max_bandwidth;
params->has_downtime_limit = true;
@@ -1433,6 +1432,11 @@ static void
migrate_params_test_apply(MigrateSetParameters *params,
dest->tls_hostname = params->tls_hostname->u.s;
}
+ if (params->has_tls_authz) {
+ assert(params->tls_authz->type == QTYPE_QSTRING);
+ dest->tls_authz = params->tls_authz->u.s;
+ }
+
if (params->has_max_bandwidth) {
dest->max_bandwidth = params->max_bandwidth;
}
@@ -1622,6 +1626,13 @@ void qmp_migrate_set_parameters(MigrateSetParameters
*params, Error **errp)
params->tls_hostname->type = QTYPE_QSTRING;
params->tls_hostname->u.s = strdup("");
}
+ /* TODO Rewrite "" to null instead */
+ if (params->has_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);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index a6a6684df1..492789248f 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -476,7 +476,7 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict
*qdict)
params->max_postcopy_bandwidth);
monitor_printf(mon, "%s: '%s'\n",
MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
- params->tls_authz);
+ params->has_tls_authz ? params->tls_authz : "");
if (params->has_block_bitmap_mapping) {
const BitmapMigrationNodeAliasList *bmnal;
--
2.26.2
- [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters, Markus Armbruster, 2020/11/13
- [PATCH 4/6] migration: Check xbzrle-cache-size more carefully, Markus Armbruster, 2020/11/13
- [PATCH 2/6] migration: Fix migrate-set-parameters argument validation, Markus Armbruster, 2020/11/13
- [PATCH 6/6] migration: Fix a few absurdly defective error messages, Markus Armbruster, 2020/11/13
- [PATCH 3/6] migration: Clean up signed vs. unsigned XBZRLE cache-size, Markus Armbruster, 2020/11/13
- [PATCH 1/6] migration: Fix and clean up around @tls-authz,
Markus Armbruster <=
- [PATCH 5/6] migration: Fix cache_init()'s "Failed to allocate" error messages, Markus Armbruster, 2020/11/13
- Re: [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters, Dr. David Alan Gilbert, 2020/11/13