The migration parameters tls_creds, tls_authz and tls_hostname
currently have a non-uniform handling. When used as arguments to
migrate-set-parameters, their type is StrOrNull and when used as
return value from query-migrate-parameters, their type is a plain
string.
Not only having to convert between the types is cumbersome, but it
also creates the issue of requiring two different QAPI types to be
used, one for each command. MigrateSetParameters is used for
migrate-set-parameters with the TLS arguments as StrOrNull while
MigrationParameters is used for query-migrate-parameters with the TLS
arguments as str.
Since StrOrNull could be considered a superset of str, change the type
of the TLS arguments in MigrationParameters to StrOrNull and add a
helper to ensure they're never actually used as QTYPE_QNULL.
This will allow the type duplication to be removed in the next
patches.
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
migration/migration-hmp-cmds.c | 8 +-
migration/migration.c | 2 +
migration/options.c | 149 ++++++++++++++++++++-------------
migration/options.h | 1 +
migration/tls.c | 2 +-
qapi/migration.json | 6 +-
6 files changed, 99 insertions(+), 69 deletions(-)
diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
index e8a563c7d8..bc8179c582 100644
--- a/migration/migration-hmp-cmds.c
+++ b/migration/migration-hmp-cmds.c
@@ -276,14 +276,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "%s: %u\n",
MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
params->max_cpu_throttle);
- assert(params->tls_creds);
monitor_printf(mon, "%s: '%s'\n",
MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
- params->tls_creds);
- assert(params->tls_hostname);
+ params->tls_creds ? params->tls_creds->u.s : "");
monitor_printf(mon, "%s: '%s'\n",
MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
- params->tls_hostname);
+ params->tls_hostname ? params->tls_hostname->u.s : "");
assert(params->has_max_bandwidth);
monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
@@ -319,7 +317,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->tls_authz ? params->tls_authz->u.s : "");
if (params->has_block_bitmap_mapping) {
const BitmapMigrationNodeAliasList *bmnal;
diff --git a/migration/migration.c b/migration/migration.c
index 4697732bef..f65cb81b6d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -4053,6 +4053,8 @@ static void migration_instance_finalize(Object *obj)
{
MigrationState *ms = MIGRATION_OBJ(obj);
+ migrate_tls_opts_free(&ms->parameters);
+
qemu_mutex_destroy(&ms->error_mutex);
qemu_mutex_destroy(&ms->qemu_file_lock);
qemu_sem_destroy(&ms->wait_unplug_sem);
diff --git a/migration/options.c b/migration/options.c
index 162c72cda4..45a95dc6da 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -162,9 +162,11 @@ const Property migration_properties[] = {
DEFINE_PROP_SIZE("announce-step", MigrationState,
parameters.announce_step,
DEFAULT_MIGRATE_ANNOUNCE_STEP),
- DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
- DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
- DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
+ /*
+ * tls-creds, tls-hostname and tls-authz are of type StrOrNull,
+ * which can't be easily handled (if at all) by qdev. So these
+ * will not be exposed as global migration options (-global).
+ */
DEFINE_PROP_UINT64("x-vcpu-dirty-limit-period", MigrationState,
parameters.x_vcpu_dirty_limit_period,
DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD),
@@ -379,13 +381,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,
@@ -834,21 +829,44 @@ 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->type == QTYPE_QSTRING &&
+ *s->parameters.tls_authz->u.s) {
+ return s->parameters.tls_authz->u.s;
+ }
+
+ 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->type == QTYPE_QSTRING &&
+ *s->parameters.tls_creds->u.s) {
+ return s->parameters.tls_creds->u.s;
+ }
+
+ 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->type == QTYPE_QSTRING &&
+ *s->parameters.tls_hostname->u.s) {
+ return s->parameters.tls_hostname->u.s;
+ }
+
+ return NULL;
+}
+
+bool migrate_tls(void)
+{
+ return !!migrate_tls_creds();
}
uint64_t migrate_vcpu_dirty_limit_period(void)
@@ -888,6 +906,36 @@ AnnounceParameters *migrate_announce_params(void)
return ≈
}
+void migrate_tls_opts_free(MigrationParameters *params)
+{
+ qapi_free_StrOrNull(params->tls_creds);
+ qapi_free_StrOrNull(params->tls_hostname);
+ qapi_free_StrOrNull(params->tls_authz);
+}
+
+/* needs BQL if dst is part of s->parameters */
+static void tls_option_set_str(StrOrNull **dstp, StrOrNull *src)
+{
+ StrOrNull *dst = *dstp;
+
+ assert(!dst);
+
+ dst = *dstp = g_new0(StrOrNull, 1);
+ dst->type = QTYPE_QSTRING;
+
+ if (!src) {
+ dst->u.s = g_strdup("");
+ return;
+ }
+
+ if (src->type == QTYPE_QSTRING) {
+ dst->u.s = g_strdup(src->u.s);
+ } else {
+ assert(src->type == QTYPE_QNULL);
+ dst->u.s = g_strdup("");
+ }
+}
+
MigrationParameters *qmp_query_migrate_parameters(Error **errp)
{
MigrationParameters *params;
@@ -903,10 +951,11 @@ 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_authz = g_strdup(s->parameters.tls_authz ?
- s->parameters.tls_authz : "");
+
+ tls_option_set_str(¶ms->tls_creds, s->parameters.tls_creds);
+ tls_option_set_str(¶ms->tls_hostname, s->parameters.tls_hostname);
+ tls_option_set_str(¶ms->tls_authz, s->parameters.tls_authz);
+
params->has_max_bandwidth = true;
params->max_bandwidth = s->parameters.max_bandwidth;
params->has_avail_switchover_bandwidth = true;
@@ -963,9 +1012,6 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
void migrate_params_init(MigrationParameters *params)
{
- params->tls_hostname = g_strdup("");
- params->tls_creds = g_strdup("");
-
/* Set has_* up only for parameter checks */
params->has_throttle_trigger_threshold = true;
params->has_cpu_throttle_initial = true;
@@ -1142,7 +1188,8 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
#ifdef CONFIG_LINUX
if (migrate_zero_copy_send() &&
((params->has_multifd_compression && params->multifd_compression) ||
- (params->tls_creds && *params->tls_creds))) {
+ (params->tls_creds && params->tls_creds->type == QTYPE_QSTRING &&
+ *params->tls_creds->u.s))) {
error_setg(errp,
"Zero copy only available for non-compressed non-TLS multifd migration");
return false;
@@ -1204,18 +1251,24 @@ 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;
+ tls_option_set_str(&dest->tls_creds, params->tls_creds);
+ } else {
+ /* drop the reference, it's owned by s->parameters */
+ dest->tls_creds = NULL;
}
if (params->tls_hostname) {
- assert(params->tls_hostname->type == QTYPE_QSTRING);
- dest->tls_hostname = params->tls_hostname->u.s;
+ tls_option_set_str(&dest->tls_hostname, params->tls_hostname);
+ } else {
+ /* drop the reference, it's owned by s->parameters */
+ dest->tls_hostname = NULL;
}
if (params->tls_authz) {
- assert(params->tls_authz->type == QTYPE_QSTRING);
- dest->tls_authz = params->tls_authz->u.s;
+ tls_option_set_str(&dest->tls_authz, params->tls_authz);
+ } else {
+ /* drop the reference, it's owned by s->parameters */
+ dest->tls_authz = NULL;
}
if (params->has_max_bandwidth) {
@@ -1320,21 +1373,18 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
}
if (params->tls_creds) {
- g_free(s->parameters.tls_creds);
- assert(params->tls_creds->type == QTYPE_QSTRING);
- s->parameters.tls_creds = g_strdup(params->tls_creds->u.s);
+ qapi_free_StrOrNull(s->parameters.tls_creds);
+ tls_option_set_str(&s->parameters.tls_creds, params->tls_creds);
}
if (params->tls_hostname) {
- g_free(s->parameters.tls_hostname);
- assert(params->tls_hostname->type == QTYPE_QSTRING);
- s->parameters.tls_hostname = g_strdup(params->tls_hostname->u.s);
+ qapi_free_StrOrNull(s->parameters.tls_hostname);
+ tls_option_set_str(&s->parameters.tls_hostname, params->tls_hostname);
}
if (params->tls_authz) {
- g_free(s->parameters.tls_authz);
- assert(params->tls_authz->type == QTYPE_QSTRING);
- s->parameters.tls_authz = g_strdup(params->tls_authz->u.s);
+ qapi_free_StrOrNull(s->parameters.tls_authz);
+ tls_option_set_str(&s->parameters.tls_authz, params->tls_authz);
}
if (params->has_max_bandwidth) {
@@ -1433,32 +1483,11 @@ 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)) {
- /* Invalid parameter */
- return;
+ if (migrate_params_check(&tmp, errp)) {
+ migrate_params_apply(params, errp);
}
- migrate_params_apply(params, errp);
+ migrate_tls_opts_free(&tmp);
}
diff --git a/migration/options.h b/migration/options.h
index 82d839709e..999eee6f3b 100644
--- a/migration/options.h
+++ b/migration/options.h
@@ -91,4 +91,5 @@ ZeroPageDetection migrate_zero_page_detection(void);
bool migrate_params_check(MigrationParameters *params, Error **errp);
void migrate_params_init(MigrationParameters *params);
+void migrate_tls_opts_free(MigrationParameters *params);
#endif
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;
}
diff --git a/qapi/migration.json b/qapi/migration.json
index 41826bde45..fa42d94810 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1293,9 +1293,9 @@
'*cpu-throttle-initial': 'uint8',
'*cpu-throttle-increment': 'uint8',
'*cpu-throttle-tailslow': 'bool',
- '*tls-creds': 'str',
- '*tls-hostname': 'str',
- '*tls-authz': 'str',
+ '*tls-creds': 'StrOrNull',
+ '*tls-hostname': 'StrOrNull',
+ '*tls-authz': 'StrOrNull',
'*max-bandwidth': 'size',
'*avail-switchover-bandwidth': 'size',
'*downtime-limit': 'uint64',
--
2.35.3Fabiano Rosas <farosas@suse.de> writes:
> The migration parameters tls_creds, tls_authz and tls_hostname
> currently have a non-uniform handling. When used as arguments to
> migrate-set-parameters, their type is StrOrNull and when used as
> return value from query-migrate-parameters, their type is a plain
> string.
>
> Not only having to convert between the types is cumbersome, but it
> also creates the issue of requiring two different QAPI types to be
> used, one for each command. MigrateSetParameters is used for
> migrate-set-parameters with the TLS arguments as StrOrNull while
> MigrationParameters is used for query-migrate-parameters with the TLS
> arguments as str.
>
> Since StrOrNull could be considered a superset of str, change the type
> of the TLS arguments in MigrationParameters to StrOrNull and add a
> helper to ensure they're never actually used as QTYPE_QNULL.
The type of @tls_creds, @tls-hostname, @tls-authz changes from str to
StrOrNull in introspection query-migrate-parameters. Loss of precision.
Introspection is already imprecise: it shows the members optional even
though they aren't. We accept the loss of precision to enable
de-duplication.
This should be worked into the commit message.
> This will allow the type duplication to be removed in the next
> patches.
>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> migration/migration-hmp-cmds.c | 8 +-
> migration/migration.c | 2 +
> migration/options.c | 149 ++++++++++++++++++++-------------
> migration/options.h | 1 +
> migration/tls.c | 2 +-
> qapi/migration.json | 6 +-
> 6 files changed, 99 insertions(+), 69 deletions(-)
>
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index e8a563c7d8..bc8179c582 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -276,14 +276,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
> monitor_printf(mon, "%s: %u\n",
> MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
> params->max_cpu_throttle);
> - assert(params->tls_creds);
> monitor_printf(mon, "%s: '%s'\n",
> MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
> - params->tls_creds);
> - assert(params->tls_hostname);
> + params->tls_creds ? params->tls_creds->u.s : "");
> monitor_printf(mon, "%s: '%s'\n",
> MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
> - params->tls_hostname);
> + params->tls_hostname ? params->tls_hostname->u.s : "");
> assert(params->has_max_bandwidth);
> monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
> MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
> @@ -319,7 +317,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->tls_authz ? params->tls_authz->u.s : "");
>
> if (params->has_block_bitmap_mapping) {
> const BitmapMigrationNodeAliasList *bmnal;
Before, the code assumes ->tls_creds, ->tls_hostname, and ->tls_authz
are non-null. It assert its assumption for the first two.
Afterwards, it maps null to "". Why is that necessary? Hmm, see below.
> diff --git a/migration/migration.c b/migration/migration.c
> index 4697732bef..f65cb81b6d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -4053,6 +4053,8 @@ static void migration_instance_finalize(Object *obj)
> {
> MigrationState *ms = MIGRATION_OBJ(obj);
>
> + migrate_tls_opts_free(&ms->parameters);
Is this a bug fix?
As far as I can tell, the object gets destroyed only on QEMU shutdown.
Freeing resources then is unnecessary, except it may help leak detection
tools.
> +
> qemu_mutex_destroy(&ms->error_mutex);
> qemu_mutex_destroy(&ms->qemu_file_lock);
> qemu_sem_destroy(&ms->wait_unplug_sem);
> diff --git a/migration/options.c b/migration/options.c
> index 162c72cda4..45a95dc6da 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -162,9 +162,11 @@ const Property migration_properties[] = {
> DEFINE_PROP_SIZE("announce-step", MigrationState,
> parameters.announce_step,
> DEFAULT_MIGRATE_ANNOUNCE_STEP),
> - DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
> - DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
> - DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
> + /*
> + * tls-creds, tls-hostname and tls-authz are of type StrOrNull,
> + * which can't be easily handled (if at all) by qdev. So these
> + * will not be exposed as global migration options (-global).
> + */
This is a compatibility break.
The orthodox way to break it is deprecate, let the grace period expire,
break. Record in docs/about/deprecated.rst at the beginning, move the
record to docs/about/removed-features.rst at the end.
An argument could be made that the interface in question is
accidental[*], not actually used by anything, and therefore breaking it
without a grace period is fine. But even then we should record the
breakage in docs/about/removed-features.rst.
Aside: the interface in question is a hack (making the migration object
a device) piled onto a hack (the way compat properties work, and how
they spill into -global). Past sins catching up with us...
> DEFINE_PROP_UINT64("x-vcpu-dirty-limit-period", MigrationState,
> parameters.x_vcpu_dirty_limit_period,
> DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD),
> @@ -379,13 +381,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,
> @@ -834,21 +829,44 @@ 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->type == QTYPE_QSTRING &&
> + *s->parameters.tls_authz->u.s) {
> + return s->parameters.tls_authz->u.s;
> + }
> +
> + 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->type == QTYPE_QSTRING &&
> + *s->parameters.tls_creds->u.s) {
> + return s->parameters.tls_creds->u.s;
> + }
> +
> + 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->type == QTYPE_QSTRING &&
> + *s->parameters.tls_hostname->u.s) {
> + return s->parameters.tls_hostname->u.s;
> + }
> +
> + return NULL;
> +}
Again, the code changes to cope with null. Why is that necessary?
Again, see below.
> +
> +bool migrate_tls(void)
> +{
> + return !!migrate_tls_creds();
> }
>
> uint64_t migrate_vcpu_dirty_limit_period(void)
> @@ -888,6 +906,36 @@ AnnounceParameters *migrate_announce_params(void)
> return ≈
> }
>
> +void migrate_tls_opts_free(MigrationParameters *params)
> +{
> + qapi_free_StrOrNull(params->tls_creds);
> + qapi_free_StrOrNull(params->tls_hostname);
> + qapi_free_StrOrNull(params->tls_authz);
> +}
> +
> +/* needs BQL if dst is part of s->parameters */
> +static void tls_option_set_str(StrOrNull **dstp, StrOrNull *src)
> +{
> + StrOrNull *dst = *dstp;
> +
> + assert(!dst);
> +
> + dst = *dstp = g_new0(StrOrNull, 1);
> + dst->type = QTYPE_QSTRING;
> +
> + if (!src) {
> + dst->u.s = g_strdup("");
> + return;
> + }
> +
> + if (src->type == QTYPE_QSTRING) {
> + dst->u.s = g_strdup(src->u.s);
> + } else {
> + assert(src->type == QTYPE_QNULL);
> + dst->u.s = g_strdup("");
> + }
> +}
Postcondition: dstp points to a StrOrNull containing a str,
i.e. QTYPE_QSTRING. Makes sense.
I'd prefer something like
StrOrNull *dst = g_new0(StrOrNull, 1);
... fill in members ...
assert(!*dstp);
*dstp = dst;
> +
> MigrationParameters *qmp_query_migrate_parameters(Error **errp)
> {
> MigrationParameters *params;
> @@ -903,10 +951,11 @@ 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_authz = g_strdup(s->parameters.tls_authz ?
> - s->parameters.tls_authz : "");
> +
> + tls_option_set_str(¶ms->tls_creds, s->parameters.tls_creds);
> + tls_option_set_str(¶ms->tls_hostname, s->parameters.tls_hostname);
> + tls_option_set_str(¶ms->tls_authz, s->parameters.tls_authz);
> +
> params->has_max_bandwidth = true;
> params->max_bandwidth = s->parameters.max_bandwidth;
> params->has_avail_switchover_bandwidth = true;
> @@ -963,9 +1012,6 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>
> void migrate_params_init(MigrationParameters *params)
> {
> - params->tls_hostname = g_strdup("");
> - params->tls_creds = g_strdup("");
> -
Is this the reason why the code now needs to deal with null?
I'm not objecting, just pointing out that the commit message didn't
prepare me for such a change.
> /* Set has_* up only for parameter checks */
> params->has_throttle_trigger_threshold = true;
> params->has_cpu_throttle_initial = true;
I'm stopping here to ask: how exactly does the patch change quasi-global
state, namely current_migration->parameters->tls_*?
[...]
[*] We have oh so many accidental external interfaces.
Markus Armbruster <armbru@redhat.com> writes:
> Fabiano Rosas <farosas@suse.de> writes:
>
>> The migration parameters tls_creds, tls_authz and tls_hostname
>> currently have a non-uniform handling. When used as arguments to
>> migrate-set-parameters, their type is StrOrNull and when used as
>> return value from query-migrate-parameters, their type is a plain
>> string.
>>
>> Not only having to convert between the types is cumbersome, but it
>> also creates the issue of requiring two different QAPI types to be
>> used, one for each command. MigrateSetParameters is used for
>> migrate-set-parameters with the TLS arguments as StrOrNull while
>> MigrationParameters is used for query-migrate-parameters with the TLS
>> arguments as str.
>>
>> Since StrOrNull could be considered a superset of str, change the type
>> of the TLS arguments in MigrationParameters to StrOrNull and add a
>> helper to ensure they're never actually used as QTYPE_QNULL.
>
> The type of @tls_creds, @tls-hostname, @tls-authz changes from str to
> StrOrNull in introspection query-migrate-parameters. Loss of precision.
> Introspection is already imprecise: it shows the members optional even
> though they aren't. We accept the loss of precision to enable
> de-duplication.
>
> This should be worked into the commit message.
>
Ack.
>> This will allow the type duplication to be removed in the next
>> patches.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>> migration/migration-hmp-cmds.c | 8 +-
>> migration/migration.c | 2 +
>> migration/options.c | 149 ++++++++++++++++++++-------------
>> migration/options.h | 1 +
>> migration/tls.c | 2 +-
>> qapi/migration.json | 6 +-
>> 6 files changed, 99 insertions(+), 69 deletions(-)
>>
>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>> index e8a563c7d8..bc8179c582 100644
>> --- a/migration/migration-hmp-cmds.c
>> +++ b/migration/migration-hmp-cmds.c
>> @@ -276,14 +276,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>> monitor_printf(mon, "%s: %u\n",
>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
>> params->max_cpu_throttle);
>> - assert(params->tls_creds);
>> monitor_printf(mon, "%s: '%s'\n",
>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
>> - params->tls_creds);
>> - assert(params->tls_hostname);
>> + params->tls_creds ? params->tls_creds->u.s : "");
>> monitor_printf(mon, "%s: '%s'\n",
>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
>> - params->tls_hostname);
>> + params->tls_hostname ? params->tls_hostname->u.s : "");
>> assert(params->has_max_bandwidth);
>> monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
>> @@ -319,7 +317,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->tls_authz ? params->tls_authz->u.s : "");
>>
>> if (params->has_block_bitmap_mapping) {
>> const BitmapMigrationNodeAliasList *bmnal;
>
> Before, the code assumes ->tls_creds, ->tls_hostname, and ->tls_authz
> are non-null. It assert its assumption for the first two.
>
> Afterwards, it maps null to "". Why is that necessary? Hmm, see below.
>
Maps NULL to "" because the intermediate type, MigrationParameters, has
been changed from str to StrOrNull. For the purposes of info
migrate_parameters and query-migrate-parameters the only valid values
are a non-empty string or an empty string.
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 4697732bef..f65cb81b6d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -4053,6 +4053,8 @@ static void migration_instance_finalize(Object *obj)
>> {
>> MigrationState *ms = MIGRATION_OBJ(obj);
>>
>> + migrate_tls_opts_free(&ms->parameters);
>
> Is this a bug fix?
>
My new version is a little different, but I can make it a separate patch
if that happens to be the case.
> As far as I can tell, the object gets destroyed only on QEMU shutdown.
> Freeing resources then is unnecessary, except it may help leak detection
> tools.
>
From a maintenance perspective I consider any leak as a bug because I
don't have control over what kinds of leak detection tools are ran on
the code. To avoid spending time looking at such bug reports I have ASAN
automated in my testing and I fix early any leaks that it founds.
>> +
>> qemu_mutex_destroy(&ms->error_mutex);
>> qemu_mutex_destroy(&ms->qemu_file_lock);
>> qemu_sem_destroy(&ms->wait_unplug_sem);
>> diff --git a/migration/options.c b/migration/options.c
>> index 162c72cda4..45a95dc6da 100644
>> --- a/migration/options.c
>> +++ b/migration/options.c
>> @@ -162,9 +162,11 @@ const Property migration_properties[] = {
>> DEFINE_PROP_SIZE("announce-step", MigrationState,
>> parameters.announce_step,
>> DEFAULT_MIGRATE_ANNOUNCE_STEP),
>> - DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
>> - DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
>> - DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
>> + /*
>> + * tls-creds, tls-hostname and tls-authz are of type StrOrNull,
>> + * which can't be easily handled (if at all) by qdev. So these
>> + * will not be exposed as global migration options (-global).
>> + */
>
> This is a compatibility break.
>
> The orthodox way to break it is deprecate, let the grace period expire,
> break. Record in docs/about/deprecated.rst at the beginning, move the
> record to docs/about/removed-features.rst at the end.
>
> An argument could be made that the interface in question is
> accidental[*], not actually used by anything, and therefore breaking it
> without a grace period is fine. But even then we should record the
> breakage in docs/about/removed-features.rst.
>
Ok. Alternatively I could try a little harder to keep these
options. I'll see what I can do.
> Aside: the interface in question is a hack (making the migration object
> a device) piled onto a hack (the way compat properties work, and how
> they spill into -global). Past sins catching up with us...
>
Hm, I've been experimenting with adding new objects (still TYPE_DEVICE)
to hold the compatibility options and parameters only. Not the entire
migration state. The MigrationState object would then be converted into
a regular struct.
MigrationState => internal type holding migration state (a better name
could be used)
MigrationOptionsState => -global migration-options.foo=on
MigrationCompatState => -global migration foo=on
But it's getting a little tricky due to s->parameters *also* containing
compat options, namely zero-page-detection.
>> DEFINE_PROP_UINT64("x-vcpu-dirty-limit-period", MigrationState,
>> parameters.x_vcpu_dirty_limit_period,
>> DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD),
>> @@ -379,13 +381,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,
>> @@ -834,21 +829,44 @@ 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->type == QTYPE_QSTRING &&
>> + *s->parameters.tls_authz->u.s) {
>> + return s->parameters.tls_authz->u.s;
>> + }
>> +
>> + 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->type == QTYPE_QSTRING &&
>> + *s->parameters.tls_creds->u.s) {
>> + return s->parameters.tls_creds->u.s;
>> + }
>> +
>> + 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->type == QTYPE_QSTRING &&
>> + *s->parameters.tls_hostname->u.s) {
>> + return s->parameters.tls_hostname->u.s;
>> + }
>> +
>> + return NULL;
>> +}
>
> Again, the code changes to cope with null. Why is that necessary?
> Again, see below.
>
This is actually a bit roundabout indeed. In my new version I do:
- migrate-set-parameters: always write either NULL or a non-empty
QTYPE_QSTRING to s->parameters.
- query-migrate-parameters: always return a (possibly empty)
QTYPE_QSTRING.
With this, internal representation is: NULL for not present, non-empty
string for present.
>> +
>> +bool migrate_tls(void)
>> +{
>> + return !!migrate_tls_creds();
>> }
>>
>> uint64_t migrate_vcpu_dirty_limit_period(void)
>> @@ -888,6 +906,36 @@ AnnounceParameters *migrate_announce_params(void)
>> return ≈
>> }
>>
>> +void migrate_tls_opts_free(MigrationParameters *params)
>> +{
>> + qapi_free_StrOrNull(params->tls_creds);
>> + qapi_free_StrOrNull(params->tls_hostname);
>> + qapi_free_StrOrNull(params->tls_authz);
>> +}
>> +
>> +/* needs BQL if dst is part of s->parameters */
>> +static void tls_option_set_str(StrOrNull **dstp, StrOrNull *src)
>> +{
>> + StrOrNull *dst = *dstp;
>> +
>> + assert(!dst);
>> +
>> + dst = *dstp = g_new0(StrOrNull, 1);
>> + dst->type = QTYPE_QSTRING;
>> +
>> + if (!src) {
>> + dst->u.s = g_strdup("");
>> + return;
>> + }
>> +
>> + if (src->type == QTYPE_QSTRING) {
>> + dst->u.s = g_strdup(src->u.s);
>> + } else {
>> + assert(src->type == QTYPE_QNULL);
>> + dst->u.s = g_strdup("");
>> + }
>> +}
>
> Postcondition: dstp points to a StrOrNull containing a str,
> i.e. QTYPE_QSTRING. Makes sense.
>
> I'd prefer something like
>
> StrOrNull *dst = g_new0(StrOrNull, 1);
>
> ... fill in members ...
>
> assert(!*dstp);
> *dstp = dst;
>
This code is also reworked a bit on the next version, I'll see whether
the comment still applies.
>> +
>> MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>> {
>> MigrationParameters *params;
>> @@ -903,10 +951,11 @@ 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_authz = g_strdup(s->parameters.tls_authz ?
>> - s->parameters.tls_authz : "");
>> +
>> + tls_option_set_str(¶ms->tls_creds, s->parameters.tls_creds);
>> + tls_option_set_str(¶ms->tls_hostname, s->parameters.tls_hostname);
>> + tls_option_set_str(¶ms->tls_authz, s->parameters.tls_authz);
>> +
>> params->has_max_bandwidth = true;
>> params->max_bandwidth = s->parameters.max_bandwidth;
>> params->has_avail_switchover_bandwidth = true;
>> @@ -963,9 +1012,6 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>
>> void migrate_params_init(MigrationParameters *params)
>> {
>> - params->tls_hostname = g_strdup("");
>> - params->tls_creds = g_strdup("");
>> -
>
> Is this the reason why the code now needs to deal with null?
>
> I'm not objecting, just pointing out that the commit message didn't
> prepare me for such a change.
>
I'll document it.
>> /* Set has_* up only for parameter checks */
>> params->has_throttle_trigger_threshold = true;
>> params->has_cpu_throttle_initial = true;
>
> I'm stopping here to ask: how exactly does the patch change quasi-global
> state, namely current_migration->parameters->tls_*?
>
It makes sure the current_migration->parameters->tls_* options are
always QTYPE_QSTRING and either a non-empty or an empty string.
The next version of the patch will instead use non-empty QTYPE_QSTRING
or NULL, which is cleaner from a C perspective.
Both versions ensure the query-* and set-* commands continue to expose
the same values. Query only shows non-empty or empty string and Set
accepts all values of a StrOrNull type.
> [...]
>
>
> [*] We have oh so many accidental external interfaces.
Fabiano Rosas <farosas@suse.de> writes:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> The migration parameters tls_creds, tls_authz and tls_hostname
>>> currently have a non-uniform handling. When used as arguments to
>>> migrate-set-parameters, their type is StrOrNull and when used as
>>> return value from query-migrate-parameters, their type is a plain
>>> string.
>>>
>>> Not only having to convert between the types is cumbersome, but it
>>> also creates the issue of requiring two different QAPI types to be
>>> used, one for each command. MigrateSetParameters is used for
>>> migrate-set-parameters with the TLS arguments as StrOrNull while
>>> MigrationParameters is used for query-migrate-parameters with the TLS
>>> arguments as str.
>>>
>>> Since StrOrNull could be considered a superset of str, change the type
>>> of the TLS arguments in MigrationParameters to StrOrNull and add a
>>> helper to ensure they're never actually used as QTYPE_QNULL.
>>
>> The type of @tls_creds, @tls-hostname, @tls-authz changes from str to
>> StrOrNull in introspection query-migrate-parameters. Loss of precision.
>> Introspection is already imprecise: it shows the members optional even
>> though they aren't. We accept the loss of precision to enable
>> de-duplication.
>>
>> This should be worked into the commit message.
>>
>
> Ack.
>
>>> This will allow the type duplication to be removed in the next
>>> patches.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>> migration/migration-hmp-cmds.c | 8 +-
>>> migration/migration.c | 2 +
>>> migration/options.c | 149 ++++++++++++++++++++-------------
>>> migration/options.h | 1 +
>>> migration/tls.c | 2 +-
>>> qapi/migration.json | 6 +-
>>> 6 files changed, 99 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>>> index e8a563c7d8..bc8179c582 100644
>>> --- a/migration/migration-hmp-cmds.c
>>> +++ b/migration/migration-hmp-cmds.c
>>> @@ -276,14 +276,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>>> monitor_printf(mon, "%s: %u\n",
>>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
>>> params->max_cpu_throttle);
>>> - assert(params->tls_creds);
>>> monitor_printf(mon, "%s: '%s'\n",
>>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
>>> - params->tls_creds);
>>> - assert(params->tls_hostname);
>>> + params->tls_creds ? params->tls_creds->u.s : "");
>>> monitor_printf(mon, "%s: '%s'\n",
>>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
>>> - params->tls_hostname);
>>> + params->tls_hostname ? params->tls_hostname->u.s : "");
>>> assert(params->has_max_bandwidth);
>>> monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
>>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
>>> @@ -319,7 +317,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->tls_authz ? params->tls_authz->u.s : "");
>>>
>>> if (params->has_block_bitmap_mapping) {
>>> const BitmapMigrationNodeAliasList *bmnal;
>>
>> Before, the code assumes ->tls_creds, ->tls_hostname, and ->tls_authz
>> are non-null. It assert its assumption for the first two.
>>
>> Afterwards, it maps null to "". Why is that necessary? Hmm, see below.
>>
>
> Maps NULL to "" because the intermediate type, MigrationParameters, has
> been changed from str to StrOrNull. For the purposes of info
> migrate_parameters and query-migrate-parameters the only valid values
> are a non-empty string or an empty string.
But is NULL possible? If you just change the type from str to StrOrNull
and no more, it isn't.
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 4697732bef..f65cb81b6d 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -4053,6 +4053,8 @@ static void migration_instance_finalize(Object *obj)
>>> {
>>> MigrationState *ms = MIGRATION_OBJ(obj);
>>>
>>> + migrate_tls_opts_free(&ms->parameters);
>>
>> Is this a bug fix?
>>
>
> My new version is a little different, but I can make it a separate patch
> if that happens to be the case.
Yes, please.
>> As far as I can tell, the object gets destroyed only on QEMU shutdown.
>> Freeing resources then is unnecessary, except it may help leak detection
>> tools.
>>
>
> From a maintenance perspective I consider any leak as a bug because I
> don't have control over what kinds of leak detection tools are ran on
> the code. To avoid spending time looking at such bug reports I have ASAN
> automated in my testing and I fix early any leaks that it founds.
>
>>> +
>>> qemu_mutex_destroy(&ms->error_mutex);
>>> qemu_mutex_destroy(&ms->qemu_file_lock);
>>> qemu_sem_destroy(&ms->wait_unplug_sem);
>>> diff --git a/migration/options.c b/migration/options.c
>>> index 162c72cda4..45a95dc6da 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -162,9 +162,11 @@ const Property migration_properties[] = {
>>> DEFINE_PROP_SIZE("announce-step", MigrationState,
>>> parameters.announce_step,
>>> DEFAULT_MIGRATE_ANNOUNCE_STEP),
>>> - DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
>>> - DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
>>> - DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
>>> + /*
>>> + * tls-creds, tls-hostname and tls-authz are of type StrOrNull,
>>> + * which can't be easily handled (if at all) by qdev. So these
>>> + * will not be exposed as global migration options (-global).
>>> + */
>>
>> This is a compatibility break.
>>
>> The orthodox way to break it is deprecate, let the grace period expire,
>> break. Record in docs/about/deprecated.rst at the beginning, move the
>> record to docs/about/removed-features.rst at the end.
>>
>> An argument could be made that the interface in question is
>> accidental[*], not actually used by anything, and therefore breaking it
>> without a grace period is fine. But even then we should record the
>> breakage in docs/about/removed-features.rst.
>>
>
> Ok. Alternatively I could try a little harder to keep these
> options. I'll see what I can do.
What do we think about this external interface?
If we think it's accidental and unused, then putting in more work to
keep it makes no sense.
If we think it's deliberate and/or used, we should either keep it, or
replace it the orthodox way.
Trouble is a common answer for use of accidental interfaces is "beats
me". I wish we'd stop creating them.
>> Aside: the interface in question is a hack (making the migration object
>> a device) piled onto a hack (the way compat properties work, and how
>> they spill into -global). Past sins catching up with us...
>>
>
> Hm, I've been experimenting with adding new objects (still TYPE_DEVICE)
> to hold the compatibility options and parameters only. Not the entire
> migration state. The MigrationState object would then be converted into
> a regular struct.
>
> MigrationState => internal type holding migration state (a better name
> could be used)
> MigrationOptionsState => -global migration-options.foo=on
> MigrationCompatState => -global migration foo=on
>
> But it's getting a little tricky due to s->parameters *also* containing
> compat options, namely zero-page-detection.
-global is a trap. Heck, global configuration state is a trap. See
discussion in review of RFC v3.
We can't always fill in the our spiked pits. Sometimes the best we can
do is to plank them and put up some "here be danger" signs.
>>> DEFINE_PROP_UINT64("x-vcpu-dirty-limit-period", MigrationState,
>>> parameters.x_vcpu_dirty_limit_period,
>>> DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD),
>>> @@ -379,13 +381,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,
>>> @@ -834,21 +829,44 @@ 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->type == QTYPE_QSTRING &&
>>> + *s->parameters.tls_authz->u.s) {
>>> + return s->parameters.tls_authz->u.s;
>>> + }
>>> +
>>> + 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->type == QTYPE_QSTRING &&
>>> + *s->parameters.tls_creds->u.s) {
>>> + return s->parameters.tls_creds->u.s;
>>> + }
>>> +
>>> + 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->type == QTYPE_QSTRING &&
>>> + *s->parameters.tls_hostname->u.s) {
>>> + return s->parameters.tls_hostname->u.s;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>
>> Again, the code changes to cope with null. Why is that necessary?
>> Again, see below.
>>
>
> This is actually a bit roundabout indeed. In my new version I do:
>
> - migrate-set-parameters: always write either NULL or a non-empty
> QTYPE_QSTRING to s->parameters.
>
> - query-migrate-parameters: always return a (possibly empty)
> QTYPE_QSTRING.
>
> With this, internal representation is: NULL for not present, non-empty
> string for present.
>
>>> +
>>> +bool migrate_tls(void)
>>> +{
>>> + return !!migrate_tls_creds();
>>> }
>>>
>>> uint64_t migrate_vcpu_dirty_limit_period(void)
>>> @@ -888,6 +906,36 @@ AnnounceParameters *migrate_announce_params(void)
>>> return ≈
>>> }
>>>
>>> +void migrate_tls_opts_free(MigrationParameters *params)
>>> +{
>>> + qapi_free_StrOrNull(params->tls_creds);
>>> + qapi_free_StrOrNull(params->tls_hostname);
>>> + qapi_free_StrOrNull(params->tls_authz);
>>> +}
>>> +
>>> +/* needs BQL if dst is part of s->parameters */
>>> +static void tls_option_set_str(StrOrNull **dstp, StrOrNull *src)
>>> +{
>>> + StrOrNull *dst = *dstp;
>>> +
>>> + assert(!dst);
>>> +
>>> + dst = *dstp = g_new0(StrOrNull, 1);
>>> + dst->type = QTYPE_QSTRING;
>>> +
>>> + if (!src) {
>>> + dst->u.s = g_strdup("");
>>> + return;
>>> + }
>>> +
>>> + if (src->type == QTYPE_QSTRING) {
>>> + dst->u.s = g_strdup(src->u.s);
>>> + } else {
>>> + assert(src->type == QTYPE_QNULL);
>>> + dst->u.s = g_strdup("");
>>> + }
>>> +}
>>
>> Postcondition: dstp points to a StrOrNull containing a str,
>> i.e. QTYPE_QSTRING. Makes sense.
>>
>> I'd prefer something like
>>
>> StrOrNull *dst = g_new0(StrOrNull, 1);
>>
>> ... fill in members ...
>>
>> assert(!*dstp);
>> *dstp = dst;
>>
>
> This code is also reworked a bit on the next version, I'll see whether
> the comment still applies.
>
>>> +
>>> MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>> {
>>> MigrationParameters *params;
>>> @@ -903,10 +951,11 @@ 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_authz = g_strdup(s->parameters.tls_authz ?
>>> - s->parameters.tls_authz : "");
>>> +
>>> + tls_option_set_str(¶ms->tls_creds, s->parameters.tls_creds);
>>> + tls_option_set_str(¶ms->tls_hostname, s->parameters.tls_hostname);
>>> + tls_option_set_str(¶ms->tls_authz, s->parameters.tls_authz);
>>> +
>>> params->has_max_bandwidth = true;
>>> params->max_bandwidth = s->parameters.max_bandwidth;
>>> params->has_avail_switchover_bandwidth = true;
>>> @@ -963,9 +1012,6 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>>>
>>> void migrate_params_init(MigrationParameters *params)
>>> {
>>> - params->tls_hostname = g_strdup("");
>>> - params->tls_creds = g_strdup("");
>>> -
>>
>> Is this the reason why the code now needs to deal with null?
>>
>> I'm not objecting, just pointing out that the commit message didn't
>> prepare me for such a change.
>>
>
> I'll document it.
>
>>> /* Set has_* up only for parameter checks */
>>> params->has_throttle_trigger_threshold = true;
>>> params->has_cpu_throttle_initial = true;
>>
>> I'm stopping here to ask: how exactly does the patch change quasi-global
>> state, namely current_migration->parameters->tls_*?
>>
>
> It makes sure the current_migration->parameters->tls_* options are
> always QTYPE_QSTRING and either a non-empty or an empty string.
>
> The next version of the patch will instead use non-empty QTYPE_QSTRING
> or NULL, which is cleaner from a C perspective.
Agree.
The struct members will have type StrOrNull, but only certain values can
occur: non-empty string, and NULL. Worth a comment, I think. Also
consider assertions.
> Both versions ensure the query-* and set-* commands continue to expose
> the same values. Query only shows non-empty or empty string and Set
> accepts all values of a StrOrNull type.
>
>> [...]
>>
>>
>> [*] We have oh so many accidental external interfaces.
Markus Armbruster <armbru@redhat.com> writes:
> Fabiano Rosas <farosas@suse.de> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Fabiano Rosas <farosas@suse.de> writes:
>>>
>>>> The migration parameters tls_creds, tls_authz and tls_hostname
>>>> currently have a non-uniform handling. When used as arguments to
>>>> migrate-set-parameters, their type is StrOrNull and when used as
>>>> return value from query-migrate-parameters, their type is a plain
>>>> string.
>>>>
>>>> Not only having to convert between the types is cumbersome, but it
>>>> also creates the issue of requiring two different QAPI types to be
>>>> used, one for each command. MigrateSetParameters is used for
>>>> migrate-set-parameters with the TLS arguments as StrOrNull while
>>>> MigrationParameters is used for query-migrate-parameters with the TLS
>>>> arguments as str.
>>>>
>>>> Since StrOrNull could be considered a superset of str, change the type
>>>> of the TLS arguments in MigrationParameters to StrOrNull and add a
>>>> helper to ensure they're never actually used as QTYPE_QNULL.
>>>
>>> The type of @tls_creds, @tls-hostname, @tls-authz changes from str to
>>> StrOrNull in introspection query-migrate-parameters. Loss of precision.
>>> Introspection is already imprecise: it shows the members optional even
>>> though they aren't. We accept the loss of precision to enable
>>> de-duplication.
>>>
>>> This should be worked into the commit message.
>>>
>>
>> Ack.
>>
>>>> This will allow the type duplication to be removed in the next
>>>> patches.
>>>>
>>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>>> ---
>>>> migration/migration-hmp-cmds.c | 8 +-
>>>> migration/migration.c | 2 +
>>>> migration/options.c | 149 ++++++++++++++++++++-------------
>>>> migration/options.h | 1 +
>>>> migration/tls.c | 2 +-
>>>> qapi/migration.json | 6 +-
>>>> 6 files changed, 99 insertions(+), 69 deletions(-)
>>>>
>>>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>>>> index e8a563c7d8..bc8179c582 100644
>>>> --- a/migration/migration-hmp-cmds.c
>>>> +++ b/migration/migration-hmp-cmds.c
>>>> @@ -276,14 +276,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>>>> monitor_printf(mon, "%s: %u\n",
>>>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
>>>> params->max_cpu_throttle);
>>>> - assert(params->tls_creds);
>>>> monitor_printf(mon, "%s: '%s'\n",
>>>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
>>>> - params->tls_creds);
>>>> - assert(params->tls_hostname);
>>>> + params->tls_creds ? params->tls_creds->u.s : "");
>>>> monitor_printf(mon, "%s: '%s'\n",
>>>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
>>>> - params->tls_hostname);
>>>> + params->tls_hostname ? params->tls_hostname->u.s : "");
>>>> assert(params->has_max_bandwidth);
>>>> monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
>>>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
>>>> @@ -319,7 +317,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->tls_authz ? params->tls_authz->u.s : "");
>>>>
>>>> if (params->has_block_bitmap_mapping) {
>>>> const BitmapMigrationNodeAliasList *bmnal;
>>>
>>> Before, the code assumes ->tls_creds, ->tls_hostname, and ->tls_authz
>>> are non-null. It assert its assumption for the first two.
>>>
>>> Afterwards, it maps null to "". Why is that necessary? Hmm, see below.
>>>
>>
>> Maps NULL to "" because the intermediate type, MigrationParameters, has
>> been changed from str to StrOrNull. For the purposes of info
>> migrate_parameters and query-migrate-parameters the only valid values
>> are a non-empty string or an empty string.
>
> But is NULL possible? If you just change the type from str to StrOrNull
> and no more, it isn't.
>
Since the TLS options don't have a qdev property anymore, they also
don't get set a default value. So s->parameters can indeed have the NULL
value in it.
I could initialize them in migrate_params_init. It's all about choosing
where to move the complexity. I'm leaning towards keeping it in the
interface: query-migrate converts them to whatever it needs to output
and set-migrate writes a normalized version into s->parameters.
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index 4697732bef..f65cb81b6d 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -4053,6 +4053,8 @@ static void migration_instance_finalize(Object *obj)
>>>> {
>>>> MigrationState *ms = MIGRATION_OBJ(obj);
>>>>
>>>> + migrate_tls_opts_free(&ms->parameters);
>>>
>>> Is this a bug fix?
>>>
>>
>> My new version is a little different, but I can make it a separate patch
>> if that happens to be the case.
>
> Yes, please.
>
>>> As far as I can tell, the object gets destroyed only on QEMU shutdown.
>>> Freeing resources then is unnecessary, except it may help leak detection
>>> tools.
>>>
>>
>> From a maintenance perspective I consider any leak as a bug because I
>> don't have control over what kinds of leak detection tools are ran on
>> the code. To avoid spending time looking at such bug reports I have ASAN
>> automated in my testing and I fix early any leaks that it founds.
>>
>>>> +
>>>> qemu_mutex_destroy(&ms->error_mutex);
>>>> qemu_mutex_destroy(&ms->qemu_file_lock);
>>>> qemu_sem_destroy(&ms->wait_unplug_sem);
>>>> diff --git a/migration/options.c b/migration/options.c
>>>> index 162c72cda4..45a95dc6da 100644
>>>> --- a/migration/options.c
>>>> +++ b/migration/options.c
>>>> @@ -162,9 +162,11 @@ const Property migration_properties[] = {
>>>> DEFINE_PROP_SIZE("announce-step", MigrationState,
>>>> parameters.announce_step,
>>>> DEFAULT_MIGRATE_ANNOUNCE_STEP),
>>>> - DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
>>>> - DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
>>>> - DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
>>>> + /*
>>>> + * tls-creds, tls-hostname and tls-authz are of type StrOrNull,
>>>> + * which can't be easily handled (if at all) by qdev. So these
>>>> + * will not be exposed as global migration options (-global).
>>>> + */
>>>
>>> This is a compatibility break.
>>>
>>> The orthodox way to break it is deprecate, let the grace period expire,
>>> break. Record in docs/about/deprecated.rst at the beginning, move the
>>> record to docs/about/removed-features.rst at the end.
>>>
>>> An argument could be made that the interface in question is
>>> accidental[*], not actually used by anything, and therefore breaking it
>>> without a grace period is fine. But even then we should record the
>>> breakage in docs/about/removed-features.rst.
>>>
>>
>> Ok. Alternatively I could try a little harder to keep these
>> options. I'll see what I can do.
>
> What do we think about this external interface?
>
> If we think it's accidental and unused, then putting in more work to
> keep it makes no sense.
>
> If we think it's deliberate and/or used, we should either keep it, or
> replace it the orthodox way.
>
There are two external interfaces actually.
-global migration.some_compat_option=on (stored in MigrationState):
seems intentional and I believe we'd lose the ability to get out of some
tricky situations if we ditched it.
-global migation.some_random_option=on (stored in MigrationParameters):
has become a debugging *feature*, which I personally don't use, but
others do. And worse: we don't know if anyone uses it in production.
We also arbitrarily put x- in front of options for some reason. There is
an argument to drop those because x- is scary and no one should be using
them.
I think it would be good to at least separate the responsibilities so
when the time comes we can deprecate/remove/replace the offending
interfaces. But I won't go into that now, there's already too much
change going on for this release.
Fabiano Rosas <farosas@suse.de> writes:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Markus Armbruster <armbru@redhat.com> writes:
>>>
>>>> Fabiano Rosas <farosas@suse.de> writes:
>>>>
>>>>> The migration parameters tls_creds, tls_authz and tls_hostname
>>>>> currently have a non-uniform handling. When used as arguments to
>>>>> migrate-set-parameters, their type is StrOrNull and when used as
>>>>> return value from query-migrate-parameters, their type is a plain
>>>>> string.
>>>>>
>>>>> Not only having to convert between the types is cumbersome, but it
>>>>> also creates the issue of requiring two different QAPI types to be
>>>>> used, one for each command. MigrateSetParameters is used for
>>>>> migrate-set-parameters with the TLS arguments as StrOrNull while
>>>>> MigrationParameters is used for query-migrate-parameters with the TLS
>>>>> arguments as str.
>>>>>
>>>>> Since StrOrNull could be considered a superset of str, change the type
>>>>> of the TLS arguments in MigrationParameters to StrOrNull and add a
>>>>> helper to ensure they're never actually used as QTYPE_QNULL.
>>>>
>>>> The type of @tls_creds, @tls-hostname, @tls-authz changes from str to
>>>> StrOrNull in introspection query-migrate-parameters. Loss of precision.
>>>> Introspection is already imprecise: it shows the members optional even
>>>> though they aren't. We accept the loss of precision to enable
>>>> de-duplication.
>>>>
>>>> This should be worked into the commit message.
>>>>
>>>
>>> Ack.
>>>
>>>>> This will allow the type duplication to be removed in the next
>>>>> patches.
>>>>>
>>>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>>>> ---
>>>>> migration/migration-hmp-cmds.c | 8 +-
>>>>> migration/migration.c | 2 +
>>>>> migration/options.c | 149 ++++++++++++++++++++-------------
>>>>> migration/options.h | 1 +
>>>>> migration/tls.c | 2 +-
>>>>> qapi/migration.json | 6 +-
>>>>> 6 files changed, 99 insertions(+), 69 deletions(-)
>>>>>
>>>>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>>>>> index e8a563c7d8..bc8179c582 100644
>>>>> --- a/migration/migration-hmp-cmds.c
>>>>> +++ b/migration/migration-hmp-cmds.c
>>>>> @@ -276,14 +276,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>>>>> monitor_printf(mon, "%s: %u\n",
>>>>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
>>>>> params->max_cpu_throttle);
>>>>> - assert(params->tls_creds);
>>>>> monitor_printf(mon, "%s: '%s'\n",
>>>>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
>>>>> - params->tls_creds);
>>>>> - assert(params->tls_hostname);
>>>>> + params->tls_creds ? params->tls_creds->u.s : "");
>>>>> monitor_printf(mon, "%s: '%s'\n",
>>>>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
>>>>> - params->tls_hostname);
>>>>> + params->tls_hostname ? params->tls_hostname->u.s : "");
>>>>> assert(params->has_max_bandwidth);
>>>>> monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
>>>>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
>>>>> @@ -319,7 +317,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->tls_authz ? params->tls_authz->u.s : "");
>>>>>
>>>>> if (params->has_block_bitmap_mapping) {
>>>>> const BitmapMigrationNodeAliasList *bmnal;
>>>>
>>>> Before, the code assumes ->tls_creds, ->tls_hostname, and ->tls_authz
>>>> are non-null. It assert its assumption for the first two.
>>>>
>>>> Afterwards, it maps null to "". Why is that necessary? Hmm, see below.
>>>>
>>>
>>> Maps NULL to "" because the intermediate type, MigrationParameters, has
>>> been changed from str to StrOrNull. For the purposes of info
>>> migrate_parameters and query-migrate-parameters the only valid values
>>> are a non-empty string or an empty string.
>>
>> But is NULL possible? If you just change the type from str to StrOrNull
>> and no more, it isn't.
>>
>
> Since the TLS options don't have a qdev property anymore, they also
> don't get set a default value. So s->parameters can indeed have the NULL
> value in it.
>
> I could initialize them in migrate_params_init. It's all about choosing
> where to move the complexity.
True!
> I'm leaning towards keeping it in the
> interface: query-migrate converts them to whatever it needs to output
> and set-migrate writes a normalized version into s->parameters.
There more than one way to skin this cat. I like to keep state
normalized.
State is an optional StrOrNull. Possible values:
* NULL
* QNull, i.e. non-NULL, ->type is QTYPE_QNULL
* Empty string, i.e. non-NULL, ->type is QTYPE_QSTRING, ->u.s is ""
* Non-empty string, i.e. non-NULL, -> type is QTYPE_QSTRING, ->u.s is
not "" (and cannot be NULL)
As far as I understand, we have just two cases semantically:
* Set, value is a non-empty string (empty makes no sense)
* Unset
I'd normalize the state to "either NULL, or (non-empty) string".
When writing state, we need to normalize.
When reading state, we can rely on it being normalized. Asserting it is
seems prudent, and should help readers.
[...]
>>>>> diff --git a/migration/options.c b/migration/options.c
>>>>> index 162c72cda4..45a95dc6da 100644
>>>>> --- a/migration/options.c
>>>>> +++ b/migration/options.c
>>>>> @@ -162,9 +162,11 @@ const Property migration_properties[] = {
>>>>> DEFINE_PROP_SIZE("announce-step", MigrationState,
>>>>> parameters.announce_step,
>>>>> DEFAULT_MIGRATE_ANNOUNCE_STEP),
>>>>> - DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
>>>>> - DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
>>>>> - DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
>>>>> + /*
>>>>> + * tls-creds, tls-hostname and tls-authz are of type StrOrNull,
>>>>> + * which can't be easily handled (if at all) by qdev. So these
>>>>> + * will not be exposed as global migration options (-global).
>>>>> + */
>>>>
>>>> This is a compatibility break.
>>>>
>>>> The orthodox way to break it is deprecate, let the grace period expire,
>>>> break. Record in docs/about/deprecated.rst at the beginning, move the
>>>> record to docs/about/removed-features.rst at the end.
>>>>
>>>> An argument could be made that the interface in question is
>>>> accidental[*], not actually used by anything, and therefore breaking it
>>>> without a grace period is fine. But even then we should record the
>>>> breakage in docs/about/removed-features.rst.
>>>>
>>>
>>> Ok. Alternatively I could try a little harder to keep these
>>> options. I'll see what I can do.
>>
>> What do we think about this external interface?
>>
>> If we think it's accidental and unused, then putting in more work to
>> keep it makes no sense.
>>
>> If we think it's deliberate and/or used, we should either keep it, or
>> replace it the orthodox way.
>>
>
> There are two external interfaces actually.
>
> -global migration.some_compat_option=on (stored in MigrationState):
>
> seems intentional and I believe we'd lose the ability to get out of some
> tricky situations if we ditched it.
>
> -global migation.some_random_option=on (stored in MigrationParameters):
>
> has become a debugging *feature*, which I personally don't use, but
> others do. And worse: we don't know if anyone uses it in production.
Accidental external interface.
> We also arbitrarily put x- in front of options for some reason. There is
> an argument to drop those because x- is scary and no one should be using
> them.
We pretty much ditched the x- convention in the QAPI schema.
docs/devel/qapi-code-gen.rst:
Names beginning with ``x-`` used to signify "experimental". This
convention has been replaced by special feature "unstable".
Goes back to
commit a3c45b3e62962f99338716b1347cfb0d427cea44
Author: Markus Armbruster <armbru@redhat.com>
Date: Thu Oct 28 12:25:12 2021 +0200
qapi: New special feature flag "unstable"
By convention, names starting with "x-" are experimental. The parts
of external interfaces so named may be withdrawn or changed
incompatibly in future releases.
The naming convention makes unstable interfaces easy to recognize.
Promoting something from experimental to stable involves a name
change. Client code needs to be updated. Occasionally bothersome.
Worse, the convention is not universally observed:
* QOM type "input-barrier" has properties "x-origin", "y-origin".
Looks accidental, but it's ABI since 4.2.
* QOM types "memory-backend-file", "memory-backend-memfd",
"memory-backend-ram", and "memory-backend-epc" have a property
"x-use-canonical-path-for-ramblock-id" that is documented to be
stable despite its name.
We could document these exceptions, but documentation helps only
humans. We want to recognize "unstable" in code, like "deprecated".
So support recognizing it the same way: introduce new special feature
flag "unstable". It will be treated specially by the QAPI generator,
like the existing feature flag "deprecated", and unlike regular
feature flags.
This commit updates documentation and prepares tests. The next commit
updates the QAPI schema. The remaining patches update the QAPI
generator and wire up -compat policy checking.
Management applications can then use query-qmp-schema and -compat to
manage or guard against use of unstable interfaces the same way as for
deprecated interfaces.
docs/devel/qapi-code-gen.txt no longer mandates the naming convention.
Using it anyway might help writers of programs that aren't
full-fledged management applications. Not using it can save us
bothersome renames. We'll see how that shakes out.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-Id: <20211028102520.747396-2-armbru@redhat.com>
The x- convention lives on in external interfaces that bypass QAPI, in
particular QOM/qdev properties.
> I think it would be good to at least separate the responsibilities so
> when the time comes we can deprecate/remove/replace the offending
> interfaces. But I won't go into that now, there's already too much
> change going on for this release.
Makes sense.
Markus Armbruster <armbru@redhat.com> writes:
> Fabiano Rosas <farosas@suse.de> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Fabiano Rosas <farosas@suse.de> writes:
>>>
>>>> Markus Armbruster <armbru@redhat.com> writes:
>>>>
>>>>> Fabiano Rosas <farosas@suse.de> writes:
>>>>>
>>>>>> The migration parameters tls_creds, tls_authz and tls_hostname
>>>>>> currently have a non-uniform handling. When used as arguments to
>>>>>> migrate-set-parameters, their type is StrOrNull and when used as
>>>>>> return value from query-migrate-parameters, their type is a plain
>>>>>> string.
>>>>>>
>>>>>> Not only having to convert between the types is cumbersome, but it
>>>>>> also creates the issue of requiring two different QAPI types to be
>>>>>> used, one for each command. MigrateSetParameters is used for
>>>>>> migrate-set-parameters with the TLS arguments as StrOrNull while
>>>>>> MigrationParameters is used for query-migrate-parameters with the TLS
>>>>>> arguments as str.
>>>>>>
>>>>>> Since StrOrNull could be considered a superset of str, change the type
>>>>>> of the TLS arguments in MigrationParameters to StrOrNull and add a
>>>>>> helper to ensure they're never actually used as QTYPE_QNULL.
>>>>>
>>>>> The type of @tls_creds, @tls-hostname, @tls-authz changes from str to
>>>>> StrOrNull in introspection query-migrate-parameters. Loss of precision.
>>>>> Introspection is already imprecise: it shows the members optional even
>>>>> though they aren't. We accept the loss of precision to enable
>>>>> de-duplication.
>>>>>
>>>>> This should be worked into the commit message.
>>>>>
>>>>
>>>> Ack.
>>>>
>>>>>> This will allow the type duplication to be removed in the next
>>>>>> patches.
>>>>>>
>>>>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>>>>> ---
>>>>>> migration/migration-hmp-cmds.c | 8 +-
>>>>>> migration/migration.c | 2 +
>>>>>> migration/options.c | 149 ++++++++++++++++++++-------------
>>>>>> migration/options.h | 1 +
>>>>>> migration/tls.c | 2 +-
>>>>>> qapi/migration.json | 6 +-
>>>>>> 6 files changed, 99 insertions(+), 69 deletions(-)
>>>>>>
>>>>>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>>>>>> index e8a563c7d8..bc8179c582 100644
>>>>>> --- a/migration/migration-hmp-cmds.c
>>>>>> +++ b/migration/migration-hmp-cmds.c
>>>>>> @@ -276,14 +276,12 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
>>>>>> monitor_printf(mon, "%s: %u\n",
>>>>>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_CPU_THROTTLE),
>>>>>> params->max_cpu_throttle);
>>>>>> - assert(params->tls_creds);
>>>>>> monitor_printf(mon, "%s: '%s'\n",
>>>>>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_CREDS),
>>>>>> - params->tls_creds);
>>>>>> - assert(params->tls_hostname);
>>>>>> + params->tls_creds ? params->tls_creds->u.s : "");
>>>>>> monitor_printf(mon, "%s: '%s'\n",
>>>>>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_HOSTNAME),
>>>>>> - params->tls_hostname);
>>>>>> + params->tls_hostname ? params->tls_hostname->u.s : "");
>>>>>> assert(params->has_max_bandwidth);
>>>>>> monitor_printf(mon, "%s: %" PRIu64 " bytes/second\n",
>>>>>> MigrationParameter_str(MIGRATION_PARAMETER_MAX_BANDWIDTH),
>>>>>> @@ -319,7 +317,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->tls_authz ? params->tls_authz->u.s : "");
>>>>>>
>>>>>> if (params->has_block_bitmap_mapping) {
>>>>>> const BitmapMigrationNodeAliasList *bmnal;
>>>>>
>>>>> Before, the code assumes ->tls_creds, ->tls_hostname, and ->tls_authz
>>>>> are non-null. It assert its assumption for the first two.
>>>>>
>>>>> Afterwards, it maps null to "". Why is that necessary? Hmm, see below.
>>>>>
>>>>
>>>> Maps NULL to "" because the intermediate type, MigrationParameters, has
>>>> been changed from str to StrOrNull. For the purposes of info
>>>> migrate_parameters and query-migrate-parameters the only valid values
>>>> are a non-empty string or an empty string.
>>>
>>> But is NULL possible? If you just change the type from str to StrOrNull
>>> and no more, it isn't.
>>>
>>
>> Since the TLS options don't have a qdev property anymore, they also
>> don't get set a default value. So s->parameters can indeed have the NULL
>> value in it.
>>
>> I could initialize them in migrate_params_init. It's all about choosing
>> where to move the complexity.
>
> True!
>
>> I'm leaning towards keeping it in the
>> interface: query-migrate converts them to whatever it needs to output
>> and set-migrate writes a normalized version into s->parameters.
>
> There more than one way to skin this cat. I like to keep state
> normalized.
>
> State is an optional StrOrNull. Possible values:
>
> * NULL
>
> * QNull, i.e. non-NULL, ->type is QTYPE_QNULL
>
> * Empty string, i.e. non-NULL, ->type is QTYPE_QSTRING, ->u.s is ""
>
> * Non-empty string, i.e. non-NULL, -> type is QTYPE_QSTRING, ->u.s is
> not "" (and cannot be NULL)
>
> As far as I understand, we have just two cases semantically:
>
> * Set, value is a non-empty string (empty makes no sense)
>
> * Unset
>
> I'd normalize the state to "either NULL, or (non-empty) string".
>
This is what I wanted to do (in the next version), but it results in
more complex and less readable code:
A) "abc"|NULL
hmp_info_migrate_parameters:
monitor_printf(..., params->tls_creds ? params->tls_creds->u.s : "");
migrate_tls_creds:
if (s->parameters.tls_creds) {
return s->parameters.tls_creds->u.s;
}
return NULL;
migrate_params_check:
if (migrate_zero_copy_send() && params->tls_creds ...
qmp_migrate_set_parameters:
migrate_params_test_apply(params);
if (migrate_params_check(tmp)) {
migrate_params_apply(params);
tls_normalize_qnull_empty_str_to_null(&s->parameters); <-- [1]
}
qmp_query_migrate_parameters:
params = QAPI_CLONE(MigrationParameters, &s->parameters);
tls_normalize_null_back_to_empty_str(); <-- [2]
Issues here are that in [1] we can't normalize earlier because we need
QTYPE_QNULL to know whether the option was ever set and in [2], since
s->parameters may contain NULL, we need another conversion step to be
able to return "". Overall, this makes the normalization step not very
useful because we need to keep track of which parts of the code are
normalized or not.
If we instead normalize to "either non-empty string or empty string"
then:
B) "abc"|""
hmp_info_migrate_parameters:
monitor_printf(..., params->tls_creds);
migrate_tls_creds:
if (*s->parameters.tls_creds->u.s) {
return s->parameters.tls_creds->u.s;
}
return NULL; <-- [1]
migrate_params_check:
if (migrate_zero_copy_send() && *params->tls_creds->u.s ...
qmp_migrate_set_parameters:
tls_normalize_qnull_null_to_str(&s->parameters); <-- [2]
migrate_params_test_apply(params);
if (migrate_params_check(tmp)) {
migrate_params_apply(params);
}
qmp_query_migrate_parameters:
params = QAPI_CLONE(MigrationParameters, &s->parameters);
The query methods get simpler because s->parameters already contains
data in the format they expect, we can normalize earlier in [2], which
means data is always in the same format throughout
qmp_migrate_set_parameters() and lastly, we already have the getter
methods [1] which can expose "abc"|NULL to the rest of the code anyway.
> When writing state, we need to normalize.
>
> When reading state, we can rely on it being normalized. Asserting it is
> seems prudent, and should help readers.
>
My main concern is that reading can rely on it being normalized, but the
query methods cannot, so they need to do an "extra conversion", which
from the reader's POV, will look nonsensical. It's not as simple as
using a ternary because the StrOrNull object needs to be allocated.
> [...]
>
>>>>>> diff --git a/migration/options.c b/migration/options.c
>>>>>> index 162c72cda4..45a95dc6da 100644
>>>>>> --- a/migration/options.c
>>>>>> +++ b/migration/options.c
>>>>>> @@ -162,9 +162,11 @@ const Property migration_properties[] = {
>>>>>> DEFINE_PROP_SIZE("announce-step", MigrationState,
>>>>>> parameters.announce_step,
>>>>>> DEFAULT_MIGRATE_ANNOUNCE_STEP),
>>>>>> - DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds),
>>>>>> - DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname),
>>>>>> - DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz),
>>>>>> + /*
>>>>>> + * tls-creds, tls-hostname and tls-authz are of type StrOrNull,
>>>>>> + * which can't be easily handled (if at all) by qdev. So these
>>>>>> + * will not be exposed as global migration options (-global).
>>>>>> + */
>>>>>
>>>>> This is a compatibility break.
Coming back to this, I implemented a DEFINE_PROP_STR_OR_NULL. It's way
better than removing the options, because this allows a default to be
set, which means query-migrate-parameters() doesn't need to deal with an
initial NULL value anymore (if query is called before set).
>>>>>
>>>>> The orthodox way to break it is deprecate, let the grace period expire,
>>>>> break. Record in docs/about/deprecated.rst at the beginning, move the
>>>>> record to docs/about/removed-features.rst at the end.
>>>>>
>>>>> An argument could be made that the interface in question is
>>>>> accidental[*], not actually used by anything, and therefore breaking it
>>>>> without a grace period is fine. But even then we should record the
>>>>> breakage in docs/about/removed-features.rst.
>>>>>
>>>>
>>>> Ok. Alternatively I could try a little harder to keep these
>>>> options. I'll see what I can do.
>>>
>>> What do we think about this external interface?
>>>
>>> If we think it's accidental and unused, then putting in more work to
>>> keep it makes no sense.
>>>
>>> If we think it's deliberate and/or used, we should either keep it, or
>>> replace it the orthodox way.
>>>
>>
>> There are two external interfaces actually.
>>
>> -global migration.some_compat_option=on (stored in MigrationState):
>>
>> seems intentional and I believe we'd lose the ability to get out of some
>> tricky situations if we ditched it.
>>
>> -global migation.some_random_option=on (stored in MigrationParameters):
>>
>> has become a debugging *feature*, which I personally don't use, but
>> others do. And worse: we don't know if anyone uses it in production.
>
> Accidental external interface.
>
>> We also arbitrarily put x- in front of options for some reason. There is
>> an argument to drop those because x- is scary and no one should be using
>> them.
>
> We pretty much ditched the x- convention in the QAPI schema.
> docs/devel/qapi-code-gen.rst:
>
> Names beginning with ``x-`` used to signify "experimental". This
> convention has been replaced by special feature "unstable".
>
> Goes back to
>
> commit a3c45b3e62962f99338716b1347cfb0d427cea44
> Author: Markus Armbruster <armbru@redhat.com>
> Date: Thu Oct 28 12:25:12 2021 +0200
>
> qapi: New special feature flag "unstable"
>
> By convention, names starting with "x-" are experimental. The parts
> of external interfaces so named may be withdrawn or changed
> incompatibly in future releases.
This allows dropping about half of the parameters we expose. Deprecate
the other half, move the remaining legitimate compat options into
MigrationParameters, (which can be set by migrate-set-parameters) and
maybe we can remove the TYPE_DEVICE from MigrationState anytime this
decade.
Moving all qdev properties to their own TYPE_DEVICE object and putting
it under --enable-debug is also an idea.
I'm willing to do the work if we ever reach a consensus about this.
>
> The naming convention makes unstable interfaces easy to recognize.
> Promoting something from experimental to stable involves a name
> change. Client code needs to be updated. Occasionally bothersome.
>
> Worse, the convention is not universally observed:
>
> * QOM type "input-barrier" has properties "x-origin", "y-origin".
> Looks accidental, but it's ABI since 4.2.
>
> * QOM types "memory-backend-file", "memory-backend-memfd",
> "memory-backend-ram", and "memory-backend-epc" have a property
> "x-use-canonical-path-for-ramblock-id" that is documented to be
> stable despite its name.
>
> We could document these exceptions, but documentation helps only
> humans. We want to recognize "unstable" in code, like "deprecated".
>
> So support recognizing it the same way: introduce new special feature
> flag "unstable". It will be treated specially by the QAPI generator,
> like the existing feature flag "deprecated", and unlike regular
> feature flags.
>
> This commit updates documentation and prepares tests. The next commit
> updates the QAPI schema. The remaining patches update the QAPI
> generator and wire up -compat policy checking.
>
> Management applications can then use query-qmp-schema and -compat to
> manage or guard against use of unstable interfaces the same way as for
> deprecated interfaces.
>
> docs/devel/qapi-code-gen.txt no longer mandates the naming convention.
> Using it anyway might help writers of programs that aren't
> full-fledged management applications. Not using it can save us
> bothersome renames. We'll see how that shakes out.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> Message-Id: <20211028102520.747396-2-armbru@redhat.com>
>
> The x- convention lives on in external interfaces that bypass QAPI, in
> particular QOM/qdev properties.
>
>> I think it would be good to at least separate the responsibilities so
>> when the time comes we can deprecate/remove/replace the offending
>> interfaces. But I won't go into that now, there's already too much
>> change going on for this release.
>
> Makes sense.
Fabiano Rosas <farosas@suse.de> writes: > Markus Armbruster <armbru@redhat.com> writes: > >> Fabiano Rosas <farosas@suse.de> writes: >> >>> Markus Armbruster <armbru@redhat.com> writes: >>> >>>> Fabiano Rosas <farosas@suse.de> writes: >>>> >>>>> Markus Armbruster <armbru@redhat.com> writes: >>>>> >>>>>> Fabiano Rosas <farosas@suse.de> writes: [...] >> There more than one way to skin this cat. I like to keep state >> normalized. >> >> State is an optional StrOrNull. Possible values: >> >> * NULL >> >> * QNull, i.e. non-NULL, ->type is QTYPE_QNULL >> >> * Empty string, i.e. non-NULL, ->type is QTYPE_QSTRING, ->u.s is "" >> >> * Non-empty string, i.e. non-NULL, -> type is QTYPE_QSTRING, ->u.s is >> not "" (and cannot be NULL) >> >> As far as I understand, we have just two cases semantically: >> >> * Set, value is a non-empty string (empty makes no sense) >> >> * Unset >> >> I'd normalize the state to "either NULL, or (non-empty) string". >> > > This is what I wanted to do (in the next version), but it results in > more complex and less readable code: [...] > If we instead normalize to "either non-empty string or empty string" > then: [...] > The query methods get simpler because s->parameters already contains > data in the format they expect, we can normalize earlier in [2], which > means data is always in the same format throughout > qmp_migrate_set_parameters() and lastly, we already have the getter > methods [1] which can expose "abc"|NULL to the rest of the code anyway. I'd like the possible states to be clearly visible, and suggest to guard them with assertions. Details, such as how exactly the states are encoded, are up to you. You're in a better position to judge them than I am. >> When writing state, we need to normalize. >> >> When reading state, we can rely on it being normalized. Asserting it is >> seems prudent, and should help readers. >> > > My main concern is that reading can rely on it being normalized, but the > query methods cannot, so they need to do an "extra conversion", which > from the reader's POV, will look nonsensical. It's not as simple as > using a ternary because the StrOrNull object needs to be allocated. [...] >>> There are two external interfaces actually. >>> >>> -global migration.some_compat_option=on (stored in MigrationState): >>> >>> seems intentional and I believe we'd lose the ability to get out of some >>> tricky situations if we ditched it. >>> >>> -global migation.some_random_option=on (stored in MigrationParameters): >>> >>> has become a debugging *feature*, which I personally don't use, but >>> others do. And worse: we don't know if anyone uses it in production. >> >> Accidental external interface. >> >>> We also arbitrarily put x- in front of options for some reason. There is >>> an argument to drop those because x- is scary and no one should be using >>> them. >> >> We pretty much ditched the x- convention in the QAPI schema. >> docs/devel/qapi-code-gen.rst: >> >> Names beginning with ``x-`` used to signify "experimental". This >> convention has been replaced by special feature "unstable". >> >> Goes back to >> >> commit a3c45b3e62962f99338716b1347cfb0d427cea44 >> Author: Markus Armbruster <armbru@redhat.com> >> Date: Thu Oct 28 12:25:12 2021 +0200 >> >> qapi: New special feature flag "unstable" >> >> By convention, names starting with "x-" are experimental. The parts >> of external interfaces so named may be withdrawn or changed >> incompatibly in future releases. > > This allows dropping about half of the parameters we expose. Deprecate > the other half, move the remaining legitimate compat options into > MigrationParameters, (which can be set by migrate-set-parameters) and > maybe we can remove the TYPE_DEVICE from MigrationState anytime this > decade. I'd love to get rid of the pseudo-device. > Moving all qdev properties to their own TYPE_DEVICE object and putting > it under --enable-debug is also an idea. > > I'm willing to do the work if we ever reach a consensus about this. I'd like migration to work more like other long-running tasks: pass the entire configuration with the command starting it, provide commands and events to manage the task while it runs. This is advice, not a demand. I'm not going to block change the migration maintainers want. I may ask you to do the QAPI schema part in certain ways, but that's detail. [...]
On Tue, Jul 01, 2025 at 09:08:13AM +0200, Markus Armbruster wrote: > Fabiano Rosas <farosas@suse.de> writes: > > > Markus Armbruster <armbru@redhat.com> writes: > > > >> Fabiano Rosas <farosas@suse.de> writes: > >> > >>> Markus Armbruster <armbru@redhat.com> writes: > >>> > >>>> Fabiano Rosas <farosas@suse.de> writes: > >>>> > >>>>> Markus Armbruster <armbru@redhat.com> writes: > >>>>> > >>>>>> Fabiano Rosas <farosas@suse.de> writes: > > [...] > > >> There more than one way to skin this cat. I like to keep state > >> normalized. > >> > >> State is an optional StrOrNull. Possible values: > >> > >> * NULL > >> > >> * QNull, i.e. non-NULL, ->type is QTYPE_QNULL > >> > >> * Empty string, i.e. non-NULL, ->type is QTYPE_QSTRING, ->u.s is "" > >> > >> * Non-empty string, i.e. non-NULL, -> type is QTYPE_QSTRING, ->u.s is > >> not "" (and cannot be NULL) > >> > >> As far as I understand, we have just two cases semantically: > >> > >> * Set, value is a non-empty string (empty makes no sense) > >> > >> * Unset > >> > >> I'd normalize the state to "either NULL, or (non-empty) string". > >> > > > > This is what I wanted to do (in the next version), but it results in > > more complex and less readable code: > > [...] > > > If we instead normalize to "either non-empty string or empty string" > > then: > > [...] > > > The query methods get simpler because s->parameters already contains > > data in the format they expect, we can normalize earlier in [2], which > > means data is always in the same format throughout > > qmp_migrate_set_parameters() and lastly, we already have the getter > > methods [1] which can expose "abc"|NULL to the rest of the code anyway. > > I'd like the possible states to be clearly visible, and suggest to guard > them with assertions. Details, such as how exactly the states are > encoded, are up to you. You're in a better position to judge them than > I am. > > >> When writing state, we need to normalize. > >> > >> When reading state, we can rely on it being normalized. Asserting it is > >> seems prudent, and should help readers. > >> > > > > My main concern is that reading can rely on it being normalized, but the > > query methods cannot, so they need to do an "extra conversion", which > > from the reader's POV, will look nonsensical. It's not as simple as > > using a ternary because the StrOrNull object needs to be allocated. > > [...] > > >>> There are two external interfaces actually. > >>> > >>> -global migration.some_compat_option=on (stored in MigrationState): > >>> > >>> seems intentional and I believe we'd lose the ability to get out of some > >>> tricky situations if we ditched it. > >>> > >>> -global migation.some_random_option=on (stored in MigrationParameters): > >>> > >>> has become a debugging *feature*, which I personally don't use, but > >>> others do. And worse: we don't know if anyone uses it in production. > >> > >> Accidental external interface. > >> > >>> We also arbitrarily put x- in front of options for some reason. There is > >>> an argument to drop those because x- is scary and no one should be using > >>> them. > >> > >> We pretty much ditched the x- convention in the QAPI schema. > >> docs/devel/qapi-code-gen.rst: > >> > >> Names beginning with ``x-`` used to signify "experimental". This > >> convention has been replaced by special feature "unstable". > >> > >> Goes back to > >> > >> commit a3c45b3e62962f99338716b1347cfb0d427cea44 > >> Author: Markus Armbruster <armbru@redhat.com> > >> Date: Thu Oct 28 12:25:12 2021 +0200 > >> > >> qapi: New special feature flag "unstable" > >> > >> By convention, names starting with "x-" are experimental. The parts > >> of external interfaces so named may be withdrawn or changed > >> incompatibly in future releases. > > > > This allows dropping about half of the parameters we expose. Deprecate > > the other half, move the remaining legitimate compat options into > > MigrationParameters, (which can be set by migrate-set-parameters) and > > maybe we can remove the TYPE_DEVICE from MigrationState anytime this > > decade. > > I'd love to get rid of the pseudo-device. > > > Moving all qdev properties to their own TYPE_DEVICE object and putting > > it under --enable-debug is also an idea. > > > > I'm willing to do the work if we ever reach a consensus about this. > > I'd like migration to work more like other long-running tasks: pass the > entire configuration with the command starting it, provide commands and > events to manage the task while it runs. > > This is advice, not a demand. I'm not going to block change the > migration maintainers want. I may ask you to do the QAPI schema part in > certain ways, but that's detail. This should be doable as IIUC at the end of this series, the QMP 'migrate-incoming' command parameters can express all the required data for accepting an incoming migration. As such, that parameter struct could be exposed on the CLI with the CLI json syntax to accept complex args in a way that wasn't possible historically with -incoming. With 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 :|
© 2016 - 2025 Red Hat, Inc.