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

[Alexey Kardashevskiy]

On 08/07/2013 04:10 PM, Peter Crosthwaite wrote:
> 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?


Oh. Right. It is confusion. I get it now. For some reason I decided that a
specific ARM CPU has its own realize() but it is defined in TYPE_CPU_ARM.
Sorry.


-- 
Alexey