Re: [Qemu-devel] [PATCH for-next v2 01/22] object: Add OBJECT_GET_PARENT_CLASS() macro

[Peter Crosthwaite]

On Wed, Aug 7, 2013 at 3:53 PM, Alexey Kardashevskiy <address@hidden> wrote:
> On 08/07/2013 03:45 PM, Andreas Färber wrote:
>> Am 07.08.2013 07:38, schrieb Alexey Kardashevskiy:
>>> On 08/07/2013 02:20 PM, Peter Crosthwaite wrote:
>>>> Hi,
>>>>
>>>> On Wed, Aug 7, 2013 at 1:36 PM, Alexey Kardashevskiy <address@hidden> 
>>>> wrote:
>>>>> On 08/06/2013 06:33 PM, Andreas Färber wrote:
>>>>>> Am 06.08.2013 07:54, schrieb Alexey Kardashevskiy:
>>>>>>> On 08/01/2013 12:17 PM, Andreas Färber wrote:
>>>>>>>> The object argument is currently unused and may be used to optimize the
>>>>>>>> class lookup when needed.
>>>>>>>>
>>>>>>>> Inspired-by: Peter Crosthwaite <address@hidden>
>>>>>>>> Signed-off-by: Andreas Färber <address@hidden>
>>>>>>>> ---
>>>>>>>>  include/qom/object.h | 10 ++++++++++
>>>>>>>>  1 file changed, 10 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/qom/object.h b/include/qom/object.h
>>>>>>>> index 23fc048..a8e71dc 100644
>>>>>>>> --- a/include/qom/object.h
>>>>>>>> +++ b/include/qom/object.h
>>>>>>>> @@ -511,6 +511,16 @@ struct TypeInfo
>>>>>>>>      OBJECT_CLASS_CHECK(class, object_get_class(OBJECT(obj)), name)
>>>>>>>>
>>>>>>>>  /**
>>>>>>>> + * OBJECT_GET_PARENT_CLASS:
>>>>>>>> + * @obj: The object to obtain the parent class for.
>>>>>>>> + * @name: The QOM typename of @obj.
>>>>>>>> + *
>>>>>>>> + * Returns the parent class for a given object of a specific class.
>>>>>>>> + */
>>>>>>>> +#define OBJECT_GET_PARENT_CLASS(obj, name) \
>>>>>>>> +    object_class_get_parent(object_class_by_name(name))
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>>   * InterfaceInfo:
>>>>>>>>   * @type: The name of the interface.
>>>>>>>>   *
>>>>>>>>
>>>>>>>
>>>>>>> Has anyone ever tried to use this macro?
>>>>>>
>>>>>> Since you're asking me, obviously later in this virtio series it's used
>>>>>> and in the IndustryPack series as well.
>>>>>>
>>>>>> I'm not aware of anyone else having used it yet - I'm still waiting for
>>>>>> review feedback from Peter Cr. and/or Anthony (or you!) before I put it
>>>>>> on qom-next.
>>>>>
>>>>>
>>>>> Still do not understand what "obj" is doing here.
>>>>
>>>> The object is currently where cast cache optimizations are
>>>> implemented. So having a handle to it is useful if ever these
>>>> get-parent operations end up in fast paths and we need to do one of
>>>> Anthony's caching tricks.
>>>>
>>>>> This what I would suggest:
>>>>>
>>>>> #define OBJECT_GET_PARENT_CLASS(obj, name) \
>>>>>     object_class_get_parent(OBJECT_GET_CLASS(ObjectClass, (obj), (name)))
>>>>>
>>>>
>>>> You have changed the semantic from what Andreas has implemented. This
>>>> will always return the parent of the concrete class, whereas to solve
>>>> the save-override-call problem you need to get the parent of abstract
>>>> level that is overriding the function (not always the concrete class).
>>>>
>>>>> @name here is just to make sure we are at the right level of the class
>>>>> hierarchy.
>>>>>
>>>>
>>>> Its @name that is actually important. Its more than just assertive,
>>>> variation of @name for the same object will and should return
>>>> different results (aside from just checking). The demonstrative case
>>>> for this requirement is TYPE_ARM_CPU, which has a realize fn that
>>>> needs to call the parent version as implemented by TYPE_CPU. ARM CPUs
>>>> however have a whole bunch of concrete classes inheriting from
>>>> TYPE_ARM_CPU. So using your macro, TYPE_ARM_CPU's realize fn will only
>>>> be able to get a handle to the parent of the concrete class (i.e.
>>>> TYPE_ARM_CPU)
>>>
>>> This is what I would really (really) expect in this situation.
>>>
>>>> and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as
>>>> intended.
>>>
>>> Oh. Then it is not get_parent() at all, this is get_grand_parent.
>>
>> No. It is parent of TYPE_ARM_CPU, as specified by the name argument.
>
> Here I lost you. You really want finalize() of TYPE_CPU, no matter how many
> classes are between TYPE_CPU and your final ARM CPU class.

No, we want the immediate parent of the TYPE_ARM_CPU class which is
not set in stone. Directly refing TYPE_CPU makes it difficult for
anyone trying to re-organise the QOM heirachy. The idea behind this
approach was that TYPE_ARM_CPU::realize does not make assumptions
about the classes above it (except that someone in the ancestry is
TYPE_DEVICE for the definition of realize) nor the classes below it
(as already discussed).

For example, if someone decides to implement TYPE_CPU_FOO (abstract
child of TYPE_CPU) and reparents TYPE_ARM_CPU to this, then without
need-for-change TYPE_ARM_CPU will now call TYPE_CPU_FOO:realize rather
than inadvertently skipping over levels to a grandparent
implementation. It can be done your proposed way, but re-organising
the heirachy will potentially require change patterns that are avoided
with this approach.

So call it
> directly, why do you need this workaround with get_parent if it is not
> really a parent of the current obj and you do not trust to what you get in

This is probably terminology confusion. Its a parent of a class not an
object. The documentation and naming maybe needs work?

Regards,
Peter