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

[Andreas Färber]

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.
obj serves for possible optimization, as demonstrated by an earlier
patch by Peter that avoids the hashtable lookup by iterating over obj's
parent classes. If we pass obj down through our macros then such changes
can be done in one central place rather than reviewing and changing the
whole code base.

> realize() of TYPE_ARM_CPU_CORTEXTA9 (example) calls its parent
> (TYPE_ARM_CPU) realize() and that realize() calls its parent (TYPE_CPU)
> realize.
> 
> It some level (for example, TYPE_ARM_CPU) does not want to implement
> realize(), then pointer to realize() from the upper class should be copied
> into TYPE_ARM_CPU's type info struct. class_init() callbacks are called for
> every class in the chain, so if some class_init() won't write to realize()
> pointer, it should be what the parent initialized it to, no?
> 
> What do I miss here?

You are missing that we do implement realize for TYPE_ARM_CPU, not for
its derived types. So we want to call TYPE_CPU's realize from there, not
some random realize determined by what type dev happens to have.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg