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

[Peter Crosthwaite]

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) and not the parent of TYPE_ARM_CPU (i.e. TYPE_CPU) as
intended.

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
>