Re: [XEN PATCH v3 1/2] x86/vmsi: rename variables to address MISRA C:2012 Rule 5.3

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.08.2023 15:38, Nicola Vetrini wrote:
> On 08/08/2023 15:22, Jan Beulich wrote:
>> On 08.08.2023 14:22, Nicola Vetrini wrote:
>>> The local variables 'irq_desc' shadow the homonymous global variable,
>>> declared in 'xen/arch/x86/include/asm/irq.h', therefore they are 
>>> renamed
>>> 'irqd' for consistency with ARM code. Other variables of the same type
>>> in the file are also renamed 'irqd' for consistency.
>>
>> I'm pretty sure I pointed out that Arm uses a mix of "desc" and "irqd".
>> So "consistency with ARM code" doesn't ...
>>
>>> --- a/xen/arch/x86/hvm/vmsi.c
>>> +++ b/xen/arch/x86/hvm/vmsi.c
>>> @@ -281,7 +281,7 @@ static int msixtbl_write(struct vcpu *v, unsigned 
>>> long address,
>>>      unsigned int nr_entry, index;
>>>      int r = X86EMUL_UNHANDLEABLE;
>>>      unsigned long flags;
>>> -    struct irq_desc *desc;
>>> +    struct irq_desc *irqd;
>>
>> ... require e.g. this rename. As mentioned before: Let's limit code
>> churn where possible, and where going beyond what's strictly necessary
>> isn't otherwise useful; there's already enough of it with all these not
>> really helpful Misra changes.
>>
>>> @@ -462,7 +462,7 @@ static void del_msixtbl_entry(struct msixtbl_entry 
>>> *entry)
>>>
>>>  int msixtbl_pt_register(struct domain *d, struct pirq *pirq, uint64_t 
>>> gtable)
>>>  {
>>> -    struct irq_desc *irq_desc;
>>> +    struct irq_desc *irqd;
>>
>> This one indeed wants renaming, but then - consistent within the file -
>> to "desc". At least that's my view.
> 
> Well, but having
> 
> struct irq_desc *desc;
> struct msi_desc *msi_desc;
> 
> and then using them both within the function doesn't seem that readable, 

You have a point there, yes. Still I'd then probably follow up with a
change to rename msi_desc -> msi (and I say this despite seeing that
farther down in the file "msi" is also used for another pointer type
variables/parameters). But with what you say in mind I'd also be okay
with you renaming to irqd where renaming is needed, but leaving "desc"
alone.

Jan