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

[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.

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?



> Regards,
> Peter
> 
>> And use it like this:
>>
>> static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>> {
>>     XICSStateClass *xsc = XICS_COMMON_CLASS(
>>                 OBJECT_GET_PARENT_CLASS(OBJECT(icp), TYPE_KVM_XICS));
>> ...
>>
>> Here both source and destination classes are checked, everyone must be happy 
>> :)
>>
>>
>> --
>> Alexey
>>


-- 
Alexey