"Dr. David Alan Gilbert" <[email protected]> writes:
> * Markus Armbruster ([email protected]) 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 <[email protected]>
>
> Reviewed-by: Dr. David Alan Gilbert <[email protected]>
Thanks!
