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.
+ */
+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,
+};
+
bool migrate_auto_converge(void)
{
MigrationState *s = migrate_get_current();
--
2.35.3Fabiano 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?
> +
> bool migrate_auto_converge(void)
> {
> MigrationState *s = migrate_get_current();
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.
>
> > +
> > bool migrate_auto_converge(void)
> > {
> > MigrationState *s = migrate_get_current();
>
--
Peter Xu
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();
>>
© 2016 - 2025 Red Hat, Inc.