Re: [PATCH v2 10/10] migration: Switch to TYPE_OBJECT with object properties

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Jun 09, 2026 at 01:25:14PM -0400, Peter Xu wrote:
> The migration object used to depend on TYPE_DEVICE due to:
> 
> - Usage of qdev properties
> - Apply compat properties and global properties
> 
> This patch re-based the object to TYPE_OBJECT with the changes:
> 
> - Switch to object properties API
> - Manually apply both compat and global properties in post_init()
> 
> Note that to avoid too many property getter/setter helpers, this patch used
> the object_property_add_*_ptr_def() APIs so that an pointer is passed to
> bind to the property.  Such API is used for most of the conversions.
> 
> After patch, the migration object initializes instance properties within
> its instance_init() callback, in migrate_params_init().
> 
> One side effect of this change is, since we switched to a loop to add all
> capabilities, the name of the properties representing a migration
> capability may chance from previously hard-coded ones (many with x-).  It's
> fine since it's only used in -global so it's only for debugging.
> 
> Similarly, I removed "x-" from other properites that used to start with
> "x-" but actually are not experimental.

Mixing such a change into a refactoring commit is bad practice,
can you keep property changes separated.

> After the whole conversion, we don't need migration_properties or the count
> anymore, hence can be removed.  While at it, we can also remove two
> DEFINE_PROP*() API that only migration uses (DEFINE_PROP_STR_OR_NULL, and
> DEFINE_PROP_MIG_CAP).
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/options.h   |   8 +-
>  migration/migration.c |  35 ++-
>  migration/options.c   | 526 ++++++++++++++++++++++++++----------------
>  3 files changed, 351 insertions(+), 218 deletions(-)


> diff --git a/migration/options.c b/migration/options.c
> index 5cbfd29099..1cc99382d3 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -54,7 +54,7 @@


> +static void migration_object_init_props_bool(MigrationState *s)
>  {
> -    const Property *prop = opaque;
> -    StrOrNull **ptr = object_field_prop_ptr(obj, prop);
> -    StrOrNull *str_or_null = *ptr;
> +    Object *obj = OBJECT(s);
> +    int i;
>  
> -    /*
> -     * The property should never be NULL because it's part of
> -     * s->parameters and a default value is always set by qdev. It
> -     * should also never be QNULL as the setter doesn't allow it.
> -     */
> -    assert(str_or_null && str_or_null->type != QTYPE_QNULL);
> -    visit_type_str(v, name, &str_or_null->u.s, errp);
> +    struct MigPropBool {
> +        const char *name;
> +        void *ptr;
> +        bool defvar;
> +    } bool_list[] = {
> +        {
> +            "store-global-state",
> +            &s->store_global_state,
> +            true,
> +        },
> +        {
> +            "send-configuration",
> +            &s->send_configuration,
> +            true,
> +        },
> +        {
> +            "send-section-footer",
> +            &s->send_section_footer,
> +            true,
> +        },
> +        {
> +            "send-switchover-start",
> +            &s->send_switchover_start,
> +            true,
> +        },
> +        {
> +            "x-preempt-pre-7-2",
> +            &s->preempt_pre_7_2,
> +            false,
> +        },
> +        {
> +            "x-cpu-throttle-tailslow",
> +            &s->parameters.cpu_throttle_tailslow,
> +            false,
> +        },
> +        {
> +            "multifd-clean-tls-termination",
> +            &s->multifd_clean_tls_termination,
> +            true,
> +        },
> +        {
> +            "multifd-flush-after-each-section",
> +            &s->multifd_flush_after_each_section,
> +            false,
> +        },
> +    };
> +    struct MigPropBool *prop;

This approach to declaring properties is pretty unpleasant
to follow IMHO. Being a custom different approach from every
other object impl is not a good thing.

> +
> +    for (i = 0; i < ARRAY_SIZE(bool_list); i++) {
> +        prop = &bool_list[i];
> +        object_property_add_bool_ptr_def(obj, prop->name,
> +                                         prop->ptr, prop->defvar);
> +    }

Using instance level properties is the old way to do things,
it is preferred to use class level properties instead.

This means you can't use the "ptr" concept to directly reference
the instance fields and have to provide setters / getters explicitly
instead, but as you've shown with the TLS properties,  a macro can
make it simple to define the repetitive getters/setters.



> +static void migration_object_init_props_enum(MigrationState *s)
> +{
> +    Object *obj = OBJECT(s);
> +    ObjectProperty *prop;
> +
> +    prop = object_property_add_enum(obj, "multifd-compression",
> +                                    "MultiFDCompression",
> +                                    &MultiFDCompression_lookup,
> +                                    mig_prop_multifd_compression_get,
> +                                    mig_prop_multifd_compression_set);
> +    object_property_set_default_str(prop, DEFAULT_MIGRATE_MULTIFD_COMPRESSION);
> +
> +    prop = object_property_add_enum(obj, "mode", "MigMode", &MigMode_lookup,
> +                                    mig_prop_mode_get, mig_prop_mode_set);
> +    object_property_set_default_str(prop, "normal");
> +
> +    prop = object_property_add_enum(obj, "zero-page-detection",
> +                                    "ZeroPageDetection",
> +                                    &ZeroPageDetection_lookup,
> +                                    mig_prop_zero_page_detection_get,
> +                                    mig_prop_zero_page_detection_set);
> +    object_property_set_default_str(prop, "multifd");
> +}

Perhaps I'm missing something, but I'm not seeing the point in
using the set_default methods - in fact I'm not really sure why
they exist in QOM at all.

I'd expect all defaults to be set in the instance _init method.
ie why isn't this done as:

   void migrate_params_init(MigrationState *s)
   {
      s->parameters.mode = MIG_MODE_NORMAL;
      s->parameters.zero_page_detection = ZERO_PAGE_DETECTION_MULTIFD;
      .... all other defaults...
   }


> +
> +static void migration_object_init_properties(MigrationState *s)
> +{
> +    migration_object_init_props_bool(s);
> +    migration_object_init_props_uint8(s);
> +    migration_object_init_props_uint32(s);
> +    migration_object_init_props_uint64(s);
> +    migration_object_init_props_size(s);
> +    migration_object_init_props_caps(s);
> +    migration_object_init_props_tls(s);
> +    migration_object_init_props_enum(s);
> +}

...and class properties be registered in migrate_params_class_init()
and the grouping per type isn't helpful IMHO, just put all the
object_class_property_add calls inline in one place.

>  
>  bool migrate_auto_converge(void)
>  {
> @@ -1107,9 +1246,10 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      return params;
>  }
>  
> -void migrate_params_init(MigrationParameters *params)
> +void migrate_params_init(MigrationState *s)
>  {
> -    migrate_mark_all_params_present(params);
> +    migration_object_init_properties(s);
> +    migrate_mark_all_params_present(&s->parameters);
>  }
>  
>  static void migrate_post_update_params(MigrationParameters *new, Error **errp)
> -- 
> 2.53.0
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|