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

[Simone Ballarin <simone.ballarin@bugseng.com>]

On 29/08/23 00:10, Julien Grall wrote:
> Hi,
> 
> On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@bugseng.com>
> wrote:
> 
>> Add or move inclusion guards to address violations of
>> MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
>> to prevent the contents of a header file being included more than
>> once").
>>
>> Inclusion guards must appear at the beginning of the headers
>> (comments are permitted anywhere).
>>
>> Mechanical change.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>> ---
>>   xen/arch/arm/efi/efi-boot.h          | 6 ++++++
>>   xen/arch/arm/include/asm/hypercall.h | 6 +++---
>>   xen/arch/arm/include/asm/iocap.h     | 6 +++---
>>   3 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
>> index 1c3640bb65..aba522ead5 100644
>> --- a/xen/arch/arm/efi/efi-boot.h
>> +++ b/xen/arch/arm/efi/efi-boot.h
>> @@ -3,6 +3,10 @@
>>    * is intended to be included by common/efi/boot.c _only_, and
>>    * therefore can define arch specific global variables.
>>    */
>> +
>> +#ifndef __ARM_EFI_EFI_BOOT_H__
>> +#define __ARM_EFI_EFI_BOOT_H__
>> +
>>   #include <xen/device_tree.h>
>>   #include <xen/libfdt/libfdt.h>
>>   #include <asm/setup.h>
>> @@ -1003,6 +1007,8 @@ static void __init efi_arch_flush_dcache_area(const
>> void *vaddr, UINTN size)
>>       __flush_dcache_area(vaddr, size);
>>   }
>>
>> +#endif /* __ARM_EFI_EFI_BOOT_H__*/
>> +
>>   /*
>>    * Local variables:
>>    * mode: C
>> diff --git a/xen/arch/arm/include/asm/hypercall.h
>> b/xen/arch/arm/include/asm/hypercall.h
>> index ccd26c5184..4f4d96f1f2 100644
>> --- 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.

> 
> 
>>   #include <public/domctl.h> /* for arch_do_domctl */
>>
>>   long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
>> diff --git a/xen/arch/arm/include/asm/iocap.h
>> b/xen/arch/arm/include/asm/iocap.h
>> index 276fefbc59..4db1b16839 100644
>> --- a/xen/arch/arm/include/asm/iocap.h
>> +++ b/xen/arch/arm/include/asm/iocap.h
>> @@ -1,10 +1,10 @@
>> -#ifndef __X86_IOCAP_H__
>> -#define __X86_IOCAP_H__
>> +#ifndef __ASM_ARM_IOCAP_H__
>> +#define __ASM_ARM_IOCAP_H__
>>
>>   #define cache_flush_permitted(d)                        \
>>       (!rangeset_is_empty((d)->iomem_caps))
>>
>> -#endif
>> +#endif /* __ASM_ARM_IOCAP_H__ */
> 
> 
> I don’t understand how this is related to the rest of the patch. You wrote
> that inclusion must appear first and this is the case here.
> 
> However the name is technically not correct. Is this really related to
> directive 4.10? If so, this should be clarified in the commit message. If
> not, then I think this should be in a separate commit.
>

Yes, you are right. This is not correlated to this series. I will put it 
on a separate commit.

> Cheers,
> 
> 
>>
>>   /*
>>    * Local variables:
>> --
>> 2.34.1
>>
>>
> 

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)