I apologize for the late reply.
Peter Xu <pet...@redhat.com> writes:
> On Tue, Sep 26, 2023 at 10:40:27PM +0200, Markus Armbruster wrote:
>> Peter Xu <pet...@redhat.com> writes:
>>
>> > Quotting from Markus in his replies:
>>
>> Quoting
>>
>> Suggest something like "Markus recently wrote:"
>
> Will do.
>
>>
>> > 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.
>>
>> Suggest
>>
>> Eliminate the duplication follows.
>>
>> Change MigrationParameters members @tls_creds, @tls_hostname,
>> @tls_authz to StrOrNull. query-migrate-parameters will of course
>> never return null.
>>
>> Since these members are qdev properties, we need property machinery
>> for StrOrNull: DEFINE_PROP_STR_OR_NULL().
>>
>> Switch migrate-set-parameters to MigrationParameters, and delete
>> MigrateSetParameters.
>
> Will do.
>
>>
>> Can you make this patch do just this de-duplication, and everything else
>> (described below) separately?
>
> One thing I can do is to move qdev_prop_str_or_null impl (from you) into a
> separate patch. I can drop some unnecessary changes too when possible,
> not yet sure what else I can split, but I can try once there.
Suggest to give it a quick try, then see whether you like the resulting
split better than what you have now.
>> > 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>
>> > Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
>> > Signed-off-by: Peter Xu <pet...@redhat.com>
[...]
>> > diff --git a/migration/options.c b/migration/options.c
>> > index 6bbfd4853d..12e392f68c 100644
>> > --- a/migration/options.c
>> > +++ b/migration/options.c
>> > @@ -164,9 +164,12 @@ 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),
>> > + DEFINE_PROP_STR_OR_NULL("tls-creds", MigrationState,
>> > + parameters.tls_creds),
>> > + DEFINE_PROP_STR_OR_NULL("tls-hostname", MigrationState,
>> > + parameters.tls_hostname),
>> > + DEFINE_PROP_STR_OR_NULL("tls-authz", MigrationState,
>> > + parameters.tls_authz),
>>
>> The qdev machinery now additionally accepts JSON null as property
>> value.
>>
>> If that's undesirable, we need to reject it.
>
> I don't think we have a need to pass in null here, not to mention this is
> only for debugging purpose.
Not sure I understand the bit about debugging.
The point I was trying to make is this. Before the patch, we reject
attempts to set the property value to null. Afterwards, we accept them,
i.e. the patch loses "reject null property value". If this loss is
undesirable, we better replace it with suitable hand-written code.
> The real problem here, IMHO, is current patch will crash if someone
> specifies -global migration.tls_* fields..
Trips this assertion:
bool visit_start_alternate(Visitor *v, const char *name,
GenericAlternate **obj, size_t size,
Error **errp)
{
bool ok;
assert(obj && size >= sizeof(GenericAlternate));
assert(!(v->type & VISITOR_OUTPUT) || *obj);
trace_visit_start_alternate(v, name, obj, size);
if (!v->start_alternate) {
---> assert(!(v->type & VISITOR_INPUT));
return true;
}
ok = v->start_alternate(v, name, obj, size, errp);
if (v->type & VISITOR_INPUT) {
assert(ok != !*obj);
}
return ok;
}
Documented with the start_alternate() method in visitor-impl.h:
/* Must be set by input and clone visitors to visit alternates */
bool (*start_alternate)(Visitor *v, const char *name,
GenericAlternate **obj, size_t size,
Error **errp);
Known restriction of the string input visitor:
/*
* The string input visitor does not implement support for visiting
* QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
* of integers (except type "size") are supported.
*/
Visitor *string_input_visitor_new(const char *str);
A similar restriction is documented for the string output visitor.
> Unfortunately I'm not an expert on qapi. Markus, does something like this
> look like a valid fix to you?
I'm not sure whether your simple patch is sufficient for lifting the
restriction. Needs a deeper think, I'm afraid. Can we make progress on
the remainder of the series in parallel?
I wish we could get rid of the string visitors.
> ===8<===
> commit 19758cbaa99c0bad634babdb6f59f23bf0de7b75
> Author: Peter Xu <pet...@redhat.com>
> Date: Mon Oct 2 14:26:23 2023 -0400
>
> qapi: Allow the string visitor to run on StrOrNull
>
> Unlike most of the existing types, StrOrNull needs to manage its own
> allocations. Provide string_input_start_alternate() for the string
> visitor
> so that things like StrOrNull will start to work.
>
> Signed-off-by: Peter Xu <pet...@redhat.com>
> ---
> qapi/string-input-visitor.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index 197139c1c0..263e26596c 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -387,6 +387,15 @@ static void string_input_free(Visitor *v)
> g_free(siv);
> }
>
> +static bool string_input_start_alternate(Visitor *v, const char *name,
> + GenericAlternate **obj, size_t size,
> + Error **errp)
> +{
> + *obj = g_malloc0(size);
> + (*obj)->type = QTYPE_QSTRING;
> + return true;
> +}
> +
> Visitor *string_input_visitor_new(const char *str)
> {
> StringInputVisitor *v;
> @@ -407,6 +416,7 @@ Visitor *string_input_visitor_new(const char *str)
> v->visitor.check_list = check_list;
> v->visitor.end_list = end_list;
> v->visitor.free = string_input_free;
> + v->visitor.start_alternate = string_input_start_alternate;
>
> v->string = str;
> v->lm = LM_NONE;
> ===8<===
>
>>
>> If it's desirable, we need to document it, and we should probably make
>> it a separate patch.
>>
>> To answer the question whether it's desirable, we need to recall the
>> purpose of the properties. They go back to
>>
>> commit e5cb7e7677010f529d3f0f9dcdb385dea9446f8d
>> Author: Peter Xu <pet...@redhat.com>
>> Date: Tue Jun 27 12:10:13 2017 +0800
>>
>> migration: let MigrationState be a qdev
>>
>> Let the old man "MigrationState" join the object family. Direct benefit
>> is that we can start to use all the property features derived from
>> current QDev, like: HW_COMPAT_* bits, command line setup for migration
>> parameters (so will never need to set them up each time using HMP/QMP,
>> this is really, really attractive for test writters), etc.
>>
>> I see no reason to disallow this happen yet. So let's start from this
>> one, to see whether it would be anything good.
>>
>> Now we init the MigrationState struct statically in main() to make sure
>> it's initialized after global properties are applied, since we'll use
>> them during creation of the object.
>>
>> No functional change at all.
>>
>> Reviewed-by: Juan Quintela <quint...@redhat.com>
>> Signed-off-by: Peter Xu <pet...@redhat.com>
>> Message-Id: <1498536619-14548-5-git-send-email-pet...@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
>> Signed-off-by: Juan Quintela <quint...@redhat.com>
>>
>> > DEFINE_PROP_UINT64("x-vcpu-dirty-limit-period", MigrationState,
>> > parameters.x_vcpu_dirty_limit_period,
>> > DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD),
>> > @@ -201,6 +204,38 @@ Property migration_properties[] = {
>> > DEFINE_PROP_END_OF_LIST(),
>> > };
>> >
>> > +/*
>> > + * NOTE: here we have a trick when converting the empty string (""): we
>> > + * need to make sure the empty string ("") will be converted to NULL, as
>> > + * some TLS code may rely on that to detect whether something is enabled
>> > + * (e.g., the tls_authz field).
>>
>> Suggest
>>
>> * Map JSON null and JSON "" to NULL, other JSON strings to the string.
>> * The curious special treatment of "" is necessary, because it needs
>> * to be interpreted like JSON null for backward compatibility.
>
> Sure.
>
>>
>> > + */
>> > +const char *str_from_StrOrNull(StrOrNull *obj)
>> > +{
>> > + if (!obj || obj->type == QTYPE_QNULL) {
>> > + return NULL;
>> > + } else if (obj->type == QTYPE_QSTRING) {
>> > + if (obj->u.s[0] == '\0') {
>> > + return NULL;
>> > + } else {
>> > + return obj->u.s;
>> > + }
>> > + } else {
>> > + abort();
>> > + }
>> > +}
>> > +
>> > +StrOrNull *StrOrNull_from_str(const char *str)
>> > +{
>> > + StrOrNull *obj = g_new0(StrOrNull, 1);
>> > +
>> > + assert(str);
>> > + obj->type = QTYPE_QSTRING;
>> > + obj->u.s = g_strdup(str);
>> > +
>> > + return obj;
>> > +}
>> > +
>> > bool migrate_auto_converge(void)
>> > {
>> > MigrationState *s = migrate_get_current();
>> > @@ -378,9 +413,11 @@ bool migrate_postcopy(void)
>> >
>> > bool migrate_tls(void)
>> > {
>> > - MigrationState *s = migrate_get_current();
>> > -
>> > - return s->parameters.tls_creds && *s->parameters.tls_creds;
>> > + /*
>> > + * The whole TLS feature relies on a non-empty tls-creds set first.
>> > + * It's disabled otherwise.
>> > + */
>>
>> Suggest
>>
>> /* TLS is enabled when @tls-creds is non-null */
>
> But this is not the fact.. we probably need to say "non-null and non-empty
> string". I'll drop it in the new version directly, as I decided to rely
> that on the comment above str_from_StrOrNull(). Let me know if you have
> other preference.
I'll check the new version.
>> > + return migrate_tls_creds();
>> > }
>> >
>> > typedef enum WriteTrackingSupport {
>> > @@ -827,21 +864,21 @@ const char *migrate_tls_authz(void)
>> > {
>> > MigrationState *s = migrate_get_current();
>> >
>> > - return s->parameters.tls_authz;
>> > + return str_from_StrOrNull(s->parameters.tls_authz);
>> > }
>>
>> This maps "" to null on use of .tls_authz. Direct use is wrong.
>>
>> The alternative is to map it when it's set. Then setting it must go
>> through a setter that maps, and direct use is fine. Observation, not a
>> demand.
>
> Right. I plan to leave it there until later, though, and make sure all
> reference to it will be correct (using str_from_StrOrNull if needed, or
> migrate_tls_*).
>
>>
>> >
>> > const char *migrate_tls_creds(void)
>> > {
>> > MigrationState *s = migrate_get_current();
>> >
>> > - return s->parameters.tls_creds;
>> > + return str_from_StrOrNull(s->parameters.tls_creds);
>> > }
>> >
>> > const char *migrate_tls_hostname(void)
>> > {
>> > MigrationState *s = migrate_get_current();
>> >
>> > - return s->parameters.tls_hostname;
>> > + return str_from_StrOrNull(s->parameters.tls_hostname);
>> > }
>>
>> Likewise.
>>
>> >
>> > uint64_t migrate_xbzrle_cache_size(void)
>> > @@ -911,10 +948,9 @@ MigrationParameters
>> > *qmp_query_migrate_parameters(Error **errp)
>> > params->cpu_throttle_increment = s->parameters.cpu_throttle_increment;
>> > params->has_cpu_throttle_tailslow = true;
>> > params->cpu_throttle_tailslow = s->parameters.cpu_throttle_tailslow;
>> > - params->tls_creds = g_strdup(s->parameters.tls_creds);
>> > - params->tls_hostname = g_strdup(s->parameters.tls_hostname);
>> > - params->tls_authz = g_strdup(s->parameters.tls_authz ?
>> > - s->parameters.tls_authz : "");
>> > + params->tls_creds = QAPI_CLONE(StrOrNull, s->parameters.tls_creds);
>> > + params->tls_hostname = QAPI_CLONE(StrOrNull,
>> > s->parameters.tls_hostname);
>> > + params->tls_authz = QAPI_CLONE(StrOrNull, s->parameters.tls_authz);
>> > params->has_max_bandwidth = true;
>> > params->max_bandwidth = s->parameters.max_bandwidth;
>> > params->has_downtime_limit = true;
>> > @@ -963,8 +999,9 @@ MigrationParameters
>> > *qmp_query_migrate_parameters(Error **errp)
>> >
>> > void migrate_params_init(MigrationParameters *params)
>> > {
>> > - params->tls_hostname = g_strdup("");
>> > - params->tls_creds = g_strdup("");
>> > + params->tls_hostname = StrOrNull_from_str("");
>> > + params->tls_creds = StrOrNull_from_str("");
>> > + params->tls_authz = StrOrNull_from_str("");
>> >
>> > /* Set has_* up only for parameter checks */
>> > params->has_compress_level = true;
>> > @@ -1145,7 +1182,7 @@ bool migrate_params_check(MigrationParameters
>> > *params, Error **errp)
>> > #ifdef CONFIG_LINUX
>> > if (migrate_zero_copy_send() &&
>> > ((params->has_multifd_compression && params->multifd_compression)
>> > ||
>> > - (params->tls_creds && *params->tls_creds))) {
>> > + migrate_tls())) {
>>
>> Correct only if params == current_migration! Are they equal?
>
> No!
>
> I'll fix that with str_from_StrOrNull(). Definitely not pretty, but I want
> to avoid growing this set so I can go back to the
> avail-switchover-bandwidth patch soon. Let me know if you have other
> suggestions.
We can accept a bit of ugliness for de-triplicating migration parameters
in the QAPI schema.
[...]
