Re: [PATCH v2 02/24] migration: Add a qdev property for StrOrNull

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Jul 01, 2025 at 08:38:19AM +0200, Markus Armbruster wrote:
>> Fabiano Rosas <farosas@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 <farosas@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();
>>