Peter Xu <pet...@redhat.com> writes:
> These two structs are mostly identical besides some fields (quote from
> Daniel P. Berrangé in his reply):
>
> 1c1
> < { 'struct': 'MigrationParameters',
> ---
> > { 'struct': 'MigrateSetParameters',
> 14,16c14,16
> < '*tls-creds': 'str',
> < '*tls-hostname': 'str',
> < '*tls-authz': 'str',
> ---
> > '*tls-creds': 'StrOrNull',
> > '*tls-hostname': 'StrOrNull',
> > '*tls-authz': 'StrOrNull',
>
> Here the difference is @MigrateSetParameters object would allow 'null'
> values for any tls-* fields passed in.
>
> Markus used to describe why it happened to be StrOrNull, and also his
> concern on having a pure "str" type to be problematic as the reset
> indicator in the commit 01fa559826 ("migration: Use JSON null instead of ""
> to reset parameter to default"). There, "null" is introduced for the tls
> fields even though being treated as "" (empty string) internally to match
> the code.
Suggest
migration/qapi: Replace @MigrateSetParameters with @MigrationParameters
migrate-set-parameters sets migration parameters, and
query-migrate-parameters gets them. Unsurprisingly, the former's
argument type MigrateSetParameters is quite close to the latter's
return type MigrationParameters. The differences are subtle:
1. Since migrate-set-parameters supports setting selected parameters,
its arguments must all be optional (so you can omit the ones you
don't want to change). query-migrate-parameters results are also
all optional, but almost all of them are in fact always present.
2. For parameters @tls_creds, @tls_hostname, @tls_authz,
migrate-set-parameters interprets special value "" as "reset to
default". Works, because "" is semantically invalid. Not a
general solution, because a semantically invalid value need not
exist. Markus added a general solution in commit 01fa559826
("migration: Use JSON null instead of "" to reset parameter to
default"). This involved changing the type from 'str' to
'StrOrNull'.
3. When parameter @block-bitmap-mapping has not been set,
query-migrate-parameters does not return it (absent optional
member). Clean (but undocumented). When parameters @tls_creds,
@tls_hostname, @tls_authz have not been set, it returns the
semantically invalid value "". Not so clean (and just as
undocumented).
Items 2. and 3. need fact-checking.
> Here to deduplicate the two objects, logically it'll be safe only if we use
> "StrOrNull" to replace "str" type, not vice versa. However we may face
> difficulty using StrOrNull as part of MigrationState.parameters [1] when
> replacing existing @MigrationParameters to use StrOrNull. With the fact
> that nobody seems to be using "null" for tls-* fields (see the long
> standing qemu crash bug on tls-authz when "null" was passed in), let's use
> "str" to represent both objects.
"May face difficulty" is insufficiently strong to justify such
incompatible change.
I'll have a look at the difficulties you mentioned in [1]. If we can
overcome them with reasonable effort, we can and should avoid the
compatibility break. If we can't, we add proper rationale here.
> This greatly deduplicates the code not only in qapi/migration.json, but
> also in the generic migration code on handling transitions between
> StrOrNull <-> str types.
>
> [1] https://lore.kernel.org/all/ZNKfoqM0V6pcvrz%2F@x1n/
>
> Signed-off-by: Peter Xu <pet...@redhat.com>
