Markus Armbruster <arm...@redhat.com> writes:
> Fabiano Rosas <faro...@suse.de> writes:
>
>> Markus Armbruster <arm...@redhat.com> writes:
>>
>>> Fabiano Rosas <faro...@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 <faro...@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.
