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.
>
> > +
> > bool migrate_auto_converge(void)
> > {
> > MigrationState *s = migrate_get_current();
>
--
Peter Xu
