From: Markus Armbruster <armbru@redhat.com>
To: qemu-devel@nongnu.org
Cc: "Daniel P . Berrangé" <berrange@redhat.com>,
dgilbert@redhat.com, quintela@redhat.com
Subject: [PATCH 1/6] migration: Fix and clean up around @tls-authz
Date: Fri, 13 Nov 2020 07:52:31 +0100 [thread overview]
Message-ID: <20201113065236.2644169-2-armbru@redhat.com> (raw)
In-Reply-To: <20201113065236.2644169-1-armbru@redhat.com>
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
next prev parent reply other threads:[~2020-11-13 6:56 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-13 6:52 [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters Markus Armbruster
2020-11-13 6:52 ` Markus Armbruster [this message]
2020-11-13 11:56 ` [PATCH 1/6] migration: Fix and clean up around @tls-authz Dr. David Alan Gilbert
2020-12-10 17:35 ` Dr. David Alan Gilbert
2020-12-10 18:10 ` Daniel P. Berrangé
2020-12-14 10:14 ` Markus Armbruster
2020-12-16 10:55 ` Daniel P. Berrangé
2020-12-17 13:07 ` Markus Armbruster
2020-12-17 14:04 ` Daniel P. Berrangé
2021-01-27 16:01 ` Markus Armbruster
2020-11-13 6:52 ` [PATCH 2/6] migration: Fix migrate-set-parameters argument validation Markus Armbruster
2020-11-13 11:49 ` Dr. David Alan Gilbert
2020-11-13 13:24 ` Markus Armbruster
2020-11-13 6:52 ` [PATCH 3/6] migration: Clean up signed vs. unsigned XBZRLE cache-size Markus Armbruster
2020-11-13 10:40 ` Dr. David Alan Gilbert
2020-11-13 6:52 ` [PATCH 4/6] migration: Check xbzrle-cache-size more carefully Markus Armbruster
2020-11-13 10:59 ` Dr. David Alan Gilbert
2020-11-13 13:35 ` Markus Armbruster
2020-11-13 16:39 ` Dr. David Alan Gilbert
2020-11-16 7:00 ` Markus Armbruster
2020-11-13 6:52 ` [PATCH 5/6] migration: Fix cache_init()'s "Failed to allocate" error messages Markus Armbruster
2020-11-13 11:01 ` Dr. David Alan Gilbert
2020-11-13 6:52 ` [PATCH 6/6] migration: Fix a few absurdly defective " Markus Armbruster
2020-11-13 11:27 ` Dr. David Alan Gilbert
2020-11-13 11:56 ` [PATCH 0/6] migration: Fixes and cleanups aroung migrate-set-parameters Dr. David Alan Gilbert
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201113065236.2644169-2-armbru@redhat.com \
--to=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=dgilbert@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).