Peter Xu <pet...@redhat.com> writes:
> On Tue, Jul 01, 2025 at 08:38:19AM +0200, Markus Armbruster wrote:
>> Fabiano Rosas <faro...@suse.de> writes:
>>
>> > The MigrationState is a QOM object with TYPE_DEVICE as a parent. This
>> > was done about eight years ago so the migration code could make use of
>> > qdev properties to define the defaults for the migration parameters
>> > and to be able to expose migration knobs for debugging via the
>> > '-global migration' command line option.
>> >
>> > Due to unrelated historical reasons, three of the migration parameters
>> > (TLS options) received different types when used via the
>> > query-migrate-parameters QMP command than with the
>> > migrate-set-parameters command. This has created a lot of duplication
>> > in the migration code and in the QAPI documentation because the whole
>> > of MigrationParameters had to be duplicated as well.
>> >
>> > The migration code is now being fixed to remove the duplication and
>> > for that to happen the offending fields need to be reconciled into a
>> > single type. The StrOrNull type is going to be used.
>> >
>> > To keep the command line compatibility, the parameters need to
>> > continue being exposed via qdev properties accessible from the command
>> > line. Introduce a qdev property StrOrNull just for that.
>> >
>> > Note that this code is being kept in migration/options.c because this
>> > version of StrOrNull doesn't need to handle QNULL because it was never
>> > a valid option in the previous command line, which took a string.
>> >
>> > Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> > ---
>> > migration/options.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 47 insertions(+)
>> >
>> > diff --git a/migration/options.c b/migration/options.c
>> > index 162c72cda4..384ef9e421 100644
>> > --- a/migration/options.c
>> > +++ b/migration/options.c
>> > @@ -83,6 +83,11 @@
>> > #define DEFINE_PROP_MIG_CAP(name, x) \
>> > DEFINE_PROP_BOOL(name, MigrationState, capabilities[x], false)
>> >
>> > +const PropertyInfo qdev_prop_StrOrNull;
>> > +#define DEFINE_PROP_STR_OR_NULL(_name, _state, _field) \
>> > + DEFINE_PROP(_name, _state, _field, qdev_prop_StrOrNull, StrOrNull *, \
>> > + .set_default = true)
>> > +
>> > #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD 1000 /*
>> > milliseconds */
>> > #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT 1 /* MB/s */
>> >
>> > @@ -204,6 +209,48 @@ const Property migration_properties[] = {
>> > };
>> > const size_t migration_properties_count =
>> > ARRAY_SIZE(migration_properties);
>> >
>> > +/*
>> > + * qdev property for TLS options handling via '-global migration'
>> > + * command line.
>> > + */
>>
>> Looks like this was a function comment. It's not, it applies to the
>> PropertyInfo and its method. Move it to the PropertyInfo?
>>
>> Maybe
>>
>> /*
>> * String property like qdev_prop_string, except it's backed by a
>> * StrOrNull * instead of a char *. This is intended for
>> * TYPE_MIGRATION's TLS options.
>> */
>>
>> > +static void set_StrOrNull(Object *obj, Visitor *v, const char *name,
>> > + void *opaque, Error **errp)
>> > +{
>> > + const Property *prop = opaque;
>> > + StrOrNull **ptr = object_field_prop_ptr(obj, prop);
>> > + StrOrNull *str_or_null = g_new0(StrOrNull, 1);
>> > +
>> > + /*
>> > + * Only str to keep compatibility, QNULL was never used via
>> > + * command line.
>> > + */
>> > + str_or_null->type = QTYPE_QSTRING;
>> > + if (!visit_type_str(v, name, &str_or_null->u.s, errp)) {
>> > + return;
>> > + }
>> > +
>> > + qapi_free_StrOrNull(*ptr);
>> > + *ptr = str_or_null;
>> > +}
>> > +
>> > +static void release_StrOrNull(Object *obj, const char *name, void *opaque)
>> > +{
>> > + const Property *prop = opaque;
>> > + qapi_free_StrOrNull(*(StrOrNull **)object_field_prop_ptr(obj, prop));
>> > +}
>> > +
>> > +static void set_default_value_tls_opt(ObjectProperty *op, const Property
>> > *prop)
>> > +{
>> > + object_property_set_default_str(op, "");
>> > +}
>> > +
>> > +const PropertyInfo qdev_prop_StrOrNull = {
>> > + .type = "StrOrNull",
>> > + .set = set_StrOrNull,
>> > + .release = release_StrOrNull,
>> > + .set_default_value = set_default_value_tls_opt,
>> > +};
>>
>> No getter, i.e. properties will be write-only. This is unusual. Is it
>> safe?
>
> Fair question..
>
> I had a quick look, device_class_set_props_n() will try to register the
> prop with legacy mode first then modern mode. Legacy mode is decided by
> [1] below:
>
> static void qdev_class_add_legacy_property(DeviceClass *dc, const Property
> *prop)
> {
> g_autofree char *name = NULL;
>
> /* Register pointer properties as legacy properties */
> if (!prop->info->print && prop->info->get) { <------------------ [1]
> return;
> }
>
> name = g_strdup_printf("legacy-%s", prop->name);
> object_class_property_add(OBJECT_CLASS(dc), name, "str",
> prop->info->print ? qdev_get_legacy_property : prop->info->get,
> NULL, NULL, (Property *)prop);
> }
>
> When with no get(), it seems it'll be wrongly treated as legacy property..
> which further means whoever tries to get() on the property will invoke
> qdev_get_legacy_property(), and likely crash on accessing info->print()..
>
> The other issue is legacy property doesn't look like to provide a setter
> function.. as it's passing NULL to object_class_property_add(set=XXX).
>
> Likely we'll need to provide get() if without changing qdev code.
>
Peter, thank you for the analysis and sorry all for not commenting on
this earlier. I have reached the same conclusions and have implemented
the .get method.
>>
>> > +
>> > bool migrate_auto_converge(void)
>> > {
>> > MigrationState *s = migrate_get_current();
>>
