Re: [PATCH 2/6] migration: Fix migrate-set-parameters argument validation

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Commit 741d4086c8 "migration: Use proper types in json" (v2.12.0)
>> switched MigrationParameters to narrower integer types, and removed
>> the simplified qmp_migrate_set_parameters()'s argument checking
>> accordingly.
>> 
>> Good idea, except qmp_migrate_set_parameters() takes
>> MigrateSetParameters, not MigrationParameters.  Its job is updating
>> migrate_get_current()->parameters (which *is* of type
>> MigrationParameters) according to its argument.  The integers now get
>> truncated silently.  Reproducer:
>> 
>>     ---> {'execute': 'query-migrate-parameters'}
>>     <--- {"return": {[...] "compress-threads": 8, [...]}}
>>     ---> {"execute": "migrate-set-parameters", "arguments": 
>> {"compress-threads": 257}}
>>     <--- {"return": {}}
>>     ---> {'execute': 'query-migrate-parameters'}
>>     <--- {"return": {[...] "compress-threads": 1, [...]}}
>> 
>> Fix by resynchronizing MigrateSetParameters with MigrationParameters.
>
> Having those two separate types is a pain!

It is!

MigrateSetParameters is the argument of migrate-set-parameters,
MigrationParameters is the result of query-migrate-parameters and part
of the internal migration state.

Differences:

(1) Optional members

    For migrate-set-parameters, we need *all* members to be optional.

    For migration state and query-migrate-parameters, we want only some
    members to be optional (currently only @tls-authz, I think).

(2) Special values

    migrate-set-parameters has a "reset to default, whatever that may
    be" feature for some members (currently only @tls-creds,
    @tls-hostname, @tls-authz, I think).  Doing that cleanly requires an
    additonal value.

The first attempt to fuse the two types (commit de63ab6124 "migrate:
Share common MigrationParameters struct", 2016-10-13) took care of (1).
Introspection of query-migrate-parameters became mildly misleading (it
claims members are optional that aren't), and C code dealing with
migration state had to take care to set the has_FOO = true.  Tolerable.

I had to revert it to address (2) cleanly and in time: commit 01fa559826
"migration: Use JSON null instead of "" to reset parameter to default",
2017-07-24.

A second try needs to take care of (2) as well.  Messes up
query-migrate-parameters some more: introspection claims a few members
can be null that can't.

Is the "reset" feature is worth all that trouble?  Is overloading
migrate-set-parameters a good idea?

>> Fixes: 741d4086c856320807a2575389d7c0505578270b
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks!