On Fri, Aug 25, 2023 at 01:15:16PM -0400, Peter Xu wrote:
> Quotting from Markus in his replies:
>
> 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).
>
> Here to deduplicate the two objects: keep @MigrationParameters as the name
> of object to use in both places, drop @MigrateSetParameters, at the
> meantime switch types of @tls* fields from "str" to "StrOrNull" types.
>
> I found that the TLS code wasn't so much relying on tls_* fields being
> non-NULL at all. Actually on the other way round: if we set tls_authz to
> an empty string (NOTE: currently, migrate_init() missed initializing
> tls_authz; also touched it up in this patch), we can already fail one of
> the migration-test (tls/x509/default-host), as qauthz_is_allowed_by_id()
> will assume tls_authz set even if tls_auths is an empty string.
>
> It means we're actually relying on tls_* fields being NULL even if it's the
> empty string.
>
> Let's just make it a rule to return NULL for empty string on these fields
> internally. For that, when converting a StrOrNull into a char* (where we
> introduced a helper here in this patch) we'll also make the empty string to
> be NULL, to make it always work. And it doesn't show any issue either when
> applying that logic to both tls_creds and tls_hostname.
>
> With above, we can safely change both migration_tls_client_create() and
> migrate_tls() to not check the empty string too finally.. not needed
> anymore.
>
> Also, we can drop the hackish conversions in qmp_migrate_set_parameters()
> where we want to make sure it's a QSTRING; it's not needed now.
>
> This greatly deduplicates the code not only in qapi/migration.json, but
> also in the generic migration code.
>
> Markus helped greatly with this patch. Besides a better commit
> message (where I just "stole" from the reply), debugged and resolved a
> double free, but also provided the StrOrNull property implementation to be
> used in MigrationState object when switching tls_* fields to StrOrNull.
>
> Co-developed-by: Markus Armbruster <arm...@redhat.com>
> Signed-off-by: Peter Xu <pet...@redhat.com>
> ---
> qapi/migration.json | 191 +---------------------------
> include/hw/qdev-properties.h | 3 +
> migration/options.h | 3 +
> hw/core/qdev-properties.c | 40 ++++++
> migration/migration-hmp-cmds.c | 20 +--
> migration/options.c | 220 ++++++++++-----------------------
> migration/tls.c | 3 +-
> 7 files changed, 125 insertions(+), 355 deletions(-)
Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
