Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.08.2023 14:53, Simone Ballarin wrote:
> On 29/08/23 00:10, Julien Grall wrote:
>> On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
>> wrote:
>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>> @@ -1,10 +1,10 @@
>>> +#ifndef __ASM_ARM_HYPERCALL_H__
>>> +#define __ASM_ARM_HYPERCALL_H__
>>> +
>>>   #ifndef __XEN_HYPERCALL_H__
>>>   #error "asm/hypercall.h should not be included directly - include
>>> xen/hypercall.h instead"
>>>   #endif
>>>
>>> -#ifndef __ASM_ARM_HYPERCALL_H__
>>> -#define __ASM_ARM_HYPERCALL_H__
>>> -
>>
>>
>> I understand that you are trying to fix a misra violation. However, this
>> feels like it was done on purpose.
>>
>> With the new change, you would not always check that the file were included
>> at the correct place. I am not against this change but this ought to be
>> explained.
> I don't think the semantics have changed. Please correct me if I'm wrong.
> 
> With this change, the only situation where the check is not performed is 
> when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been 
> successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is 
> defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be 
> useless.
> 
> The same thing happened with the code before the change: if I include 
> the file after xen/hypercall.h, the check will always succeed.

Hmm, I think you're right, but I draw a different conclusion: The check
fails to work as intended. And this can only be repaired without your
adjustment.

Jan