Re: [Qemu-devel] [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

[Cédric Le Goater]

On 06/09/2017 04:10 AM, David Gibson wrote:
> On Thu, Jun 08, 2017 at 07:26:35PM +0200, Cédric Le Goater wrote:
>> On 06/08/2017 07:00 PM, Greg Kurz wrote:
>>> On Thu, 8 Jun 2017 18:08:44 +0200
>>> Cédric Le Goater <address@hidden> wrote:
>>>
>>>>>>> FWIW, other people do that as well (see hw/i386/pc_q35.c for example).  
>>>>>>>   
>>>>>>
>>>>>> well, I don't see the benefits of changing a string constant by a 
>>>>>> define. 
>>>>>>  
>>>>>
>>>>> Improved semantics,  especially since the "xics" string appears in 
>>>>> many places with different meanings.   
>>>>
>>>> ah ? If so, we should do a cleanup up. The code seems consistent from 
>>>> what I can see. xics is a general name for :
>>>>
>>>>    'PowerPC interrupt controller (type 2)' 
>>>>
>>>> and it is mostly used as a prefix. There are no "xics" object, only a 
>>>
>>> I'm only talking about "xics" as a property name actually:
>>>
>>> $ git grep '"xics"'
>>> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", 
>>> &err);
>>> hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", 
>>> &err);
>>> hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
>>> hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), 
>>> "xics",
>>> hw/ppc/pnv_core.c:    object_property_add_const_link(obj, "xics", 
>>> OBJECT(xi), &error_abort);
>>> hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", 
>>> &local_err);
>>> hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", 
>>> &err);
>>> hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), "xics", 
>>> obj,  &error_abort);
>>> hw/ppc/spapr.c:    object_property_add_const_link(obj, "xics", 
>>> OBJECT(spapr), &error_abort);
>>> hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, "xics", 
>>> OBJECT(spapr), &error_abort);
>>>
>>> You have to read the code to know which ones are related.
>>
>> The "xics" property link always point to the same object : 
>> the XICSFabric object which is the machine, spapr or pnv. 
>>
>>> With this patch applied, it is mostly obvious, even for the newbie:
>>
>> ah. the goal is to know where in the code the link was set. 
>> It can be even more complex with aliases.
> 
> There doesn't seem to be a strong convention about whether to use raw
> property names or defines across qemu.  I'm not all that fussed either
> way.
> 
> I do see one small advantage to use defines: if you make a typo, it
> will probably result in a compile time error, whereas with a bare
> string it won't show up until a runtime error.

ok. I can agree with that.

> In this case, I intend to take the macro patch, mostly just on the
> basis of avoiding further delays to rework the remaining patches.

But I don't think having two different defines is a good idea : 

+#define ICP_PROP_XICS "xics"
+#define ICS_PROP_XICS "xics"

C.