Re: [PATCH v3 2/8] qom/object.c: introduce object_class_property_add_alias()

[Markus Armbruster <armbru@redhat.com>]

Mark Cave-Ayland <mark.caveayland@nutanix.com> writes:

> On 17/09/2026 09:52, Marc-André Lureau wrote:
>
>> Hi
>> On Wed, Sep 2, 2026 at 6:42 PM Mark Cave-Ayland
>> <mark.caveayland@nutanix.com> wrote:
>>>
>>> This is eventually intended to be a replacement for object_property_add_alias()
>>> which uses an object property instead of a class property.
>>>
>>> With the advent of class properties, it is possible that QOM may attempt to
>>> set or retrieve the value of an unset alias property. Update the existing
>>> property_get_alias() and property_set_alias() functions to use the null visitor
>>> if the alias target has not been set, and property_resolve_alias() to return
>>> NULL for the same case.
>>>
>>> This ensures that unset class alias properties accessed e.g. via the monitor do
>>> not cause QEMU to crash.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>> ---
>>>   include/qom/object.h |  25 +++++++++++
>>>   qom/object.c         | 100 +++++++++++++++++++++++++++++++++++++------
>>>   2 files changed, 113 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>> index e24f0a2b2d..96c76975ef 100644
>>> --- a/include/qom/object.h
>>> +++ b/include/qom/object.h
>>> @@ -2263,6 +2263,11 @@ ObjectProperty *object_class_static_property_add_uint64_ptr(ObjectClass *klass,
>>>                                             const uint64_t *v,
>>>                                             ObjectPropertyFlags flags);
>>>
>>> +typedef enum {
>>> +    /* private */
>>> +    OBJ_PROP_ALIAS_CLASS = 0x1,
>>> +} ObjectPropertyAliasFlags;
>>> +
>>>   /**
>>>    * object_property_add_alias:
>>>    * @obj: the object to add a property to
>>> @@ -2283,6 +2288,26 @@ ObjectProperty *object_class_static_property_add_uint64_ptr(ObjectClass *klass,
>>>   ObjectProperty *object_property_add_alias(Object *obj, const char *name,
>>>                                  Object *target_obj, const char *target_name);
>>>
>>> +/**
>>> + * object_class_property_add_alias:
>>> + * @klass: the object class to add a property to
>>> + * @name: the name of the property
>>> + * @offset: the offset from the object instance where the object alias is
>>> + *   stored
>>> + * @target_type: QOM type we expect the alias to resolve to
>>> + * @target_name: the name of the property on the forwarded object
>>> + *
>>> + * Add an alias for a property on an object.  This function will add a property
>>> + * of the same type as the forwarded property.
>>> + *
>>> + * Returns: The newly added property on success, or %NULL on failure.
>> actually, it assert() on error and never returns NULL.
>
> I was able to reproduce this by attempting to add a class alias property with the same name again, and then started to look at other related functions do to understand what the behaviour should be. There seems to be a number of existing issues here:
>
> - object_property_try_add() returns NULL in the case a property cannot
>   be created (which seems to be only when an attempt to make add a
>   property that already exists). However since object_property_add()
>   always passes &error_abort, then QEMU terminates immediately without
>   returning the NULL.

Convention: when we have a pair of functions FOO_try_BAR() and
FOO_BAR(), the former can fail and the latter cannot.  The latter is
useful when failure would be a programming error.

When FOO_try_BAR() sets an Error on failure, FOO_BAR() is a thin wrapper
that passes &error_abort.

We pass &error_abort when failure would be a programming error.  It's
more concise than "call then assert it didn't fail".  Concise is good.

The wrapper makes calls that cannot fail even more concise.  Worth
having only when there are plenty of callers that profit from that.
Someone decided this is the case for object_property_try_add() /
object_property_add().

> - object_class_property_add() will currently assert() if an attempt is
>   made to add a duplicate property: it is fairly trivial to update it to
>   return the same error that object_property_add() does, however it is
>   interesting to note that the kernel-doc states that
>   error_setg(&error_abort, ...) should NOT be used. However in this case
>   it appears to return a genuinely helpful message which I think is
>   worth keeping.

The only difference between object_class_property_add() and
object_property_add() should be where the property is recorded (class
vs. object).  In particular, they should treat failure the same way.

This may make object_class_property_try_add() necessary.

> - There are several places in qom/object.c that do not check the
> return value of object_property_add() / object_class_property_add()
> such as object_add_link_prop(), object_property_add_alias() which
> could potentially dereference a NULL pointer.

object_property_add() is specified not to fail, i.e. not to return null.
It's a programming error if it does.  Letting the program crash on
programming errors by dereferencing a null pointer is just fine.  We can
dress up the crash with assert(obj), but I wouldn't bother.

Use object_property_add() when failure is a programming error.  Use
object_property_try_add() and handle the error when it's not.

> It feels like the assumption is actually that these functions will &error_abort rather than returning NULL as indicated by the kernel-doc. Any thoughts on what we should do here? Ideally we want the same behaviour between the object_property*() and object_class_property*() functions to aid the transition.

Hope this helps!

[...]