Re: [PATCH 01/21] migration: Normalize tls arguments

[Fabiano Rosas <farosas@suse.de>]

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.