Re: [PATCH v2 05/10] qom: Create object-property-ptr.[ch]

[Fabiano Rosas <farosas@suse.de>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Jun 10, 2026 at 02:39:09PM -0400, Peter Xu wrote:
>> On Wed, Jun 10, 2026 at 05:15:59PM +0100, Daniel P. Berrangé wrote:
>> > On Tue, Jun 09, 2026 at 01:25:09PM -0400, Peter Xu wrote:
>> > > Create object-property-ptr.[ch] files to include all the helpers for
>> > > object_property_add*_ptr().
>> > > 
>> > > These set of helpers are handy because they look extremely familiar with
>> > > qdev-properties, allowing the caller to provide a pointer and it will
>> > > manage all the setters and getters.
>> > > 
>> > > The follow up patches may introduce more of such helpers.  Since object.c
>> > > has been already too big, split that part out.
>> > 
>> > The "ptr" helpers are all instance level properties which is a concept
>> > we discourage from new usage, in favour of class level properties.
>> > 
>> > I don't think we should be adding more "ptr" helpers, but rather
>> > planning to eliminiate the (surprisingly little) usage of the
>> > existing ones.
>> 
>> The other way to do similar thing is qdev's offset way, but IMHO that's
>> more awkward to remember an offset of a pointer then do math everytime.
>> Essentially, from technical pov we need at least one uintptr_t to store
>> either (1) offset, or (2) field pointer when there's a field that is bound
>> to a prop.  IMHO (2) can be better otherwise we'll need to do all the maths
>> to calculate offsets then when access we add the offset back and do a force
>> cast.  It seems not necessary.
>
> There shouldn't be any need to play with field offsets either. Just
> define the setters & getters to directly access the fields.
>
> Take some samples from the "machine_class_init" in hw/core/machine.c
> as an example of the normal design pattern:
>
>     object_class_property_add_str(oc, "dumpdtb",
>         machine_get_dumpdtb, machine_set_dumpdtb);
>     object_class_property_set_description(oc, "dumpdtb",
>         "Dump current dtb to a file and quit");
>
>    ..snip..
>
>     object_class_property_add_bool(oc, "dump-guest-core",
>         machine_get_dump_guest_core, machine_set_dump_guest_core);
>     object_class_property_set_description(oc, "dump-guest-core",
>         "Include guest memory in a core dump");
>
> These are paired with:
>
>   static char *machine_get_dumpdtb(Object *obj, Error **errp)
>   {
>       MachineState *ms = MACHINE(obj);
>
>       return g_strdup(ms->dumpdtb);
>   }
>
>   static void machine_set_dumpdtb(Object *obj, const char *value, Error **errp)
>   {
>       MachineState *ms = MACHINE(obj);
>
>       g_free(ms->dumpdtb);
>       ms->dumpdtb = g_strdup(value);
>   }
>
> and
>
>   static bool machine_get_dump_guest_core(Object *obj, Error **errp)
>   {
>       MachineState *ms = MACHINE(obj);
>
>       return ms->dump_guest_core;
>   }
>
>   static void machine_set_dump_guest_core(Object *obj, bool value, Error **errp)
>   {
>       MachineState *ms = MACHINE(obj);
>
>       if (!value && QEMU_MADV_DONTDUMP == QEMU_MADV_INVALID) {
>           error_setg(errp, "Dumping guest memory cannot be disabled on this host");
>           return;
>       }
>       ms->dump_guest_core = value;
>   }
>
>
> and defaults (if needed) are set in  the instance init method:
>
>   static void machine_initfn(Object *obj)
>   {
>       MachineState *ms = MACHINE(obj);
>       MachineClass *mc = MACHINE_GET_CLASS(obj);
>
>       ms->dump_guest_core = true;
>       ms->mem_merge = (QEMU_MADV_MERGEABLE != QEMU_MADV_INVALID);
>       ...
>   }
>

Thanks for the practical example. In this usage, do you think there
would be a way to avoid having 53 getters and setters? We often run into
this issue of the rest of QEMU having just a handful of options and
migration having a ton. Anything we can do to streamline this would be
good.

>> OTOH, I still see value on non-class instance properties (that sometimes we
>> don't even want to have some props avail for the class, but conditional to
>> some instances when created dynamically).  If that is needed, IMHO it's
>> fine we still provide per-instance properties.
>
> Conditionally registering properties on instances is an anti-pattern
> IMHO. It results in objects that cannot have all their properties
> statically introspected.
>
> If a class needs different subsets of properties for different
> scenarios, that is potentially a sign there ought to be a base
> class and two or more specialized subclasses. Or that the
> property needs a more explicit "unset" state in addition to
> its other valid values.
>
> For introspection we hack around the use of instance properties
> by instantiating every object in order to trigger registration of
> instance props.  This is such a gross hack that it periodically 
> suffers crashes from objects not expecting to be instantiated
> in this context and having undesired side-effects. Class props
> are the only sane way to reliably provide introspection without
> side-effects.
>
>> Is there any pointer I can read about the discussion previously on this?
>
> I don't know that there's a particular thread in recent times
> that I'd point to, just my ancient series from when I introduced
> class properties for QOM back in 2015 and started the effort to
> convert away from instances properties.
>
>   https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg03112.html
>

Anything we can do to avoid stumbling into this again? Should we add
some words to object_property_add() discouraging instance properties?

>
>
>> 
>> Thanks,
>> 
>> -- 
>> Peter Xu
>> 
>
> With regards,
> Daniel