Re: [XEN PATCH v2 00/10] address violations of MISRA C:2012 Directive 4.10

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.09.2023 15:17, Simone Ballarin wrote:
> On 28/09/23 14:51, Jan Beulich wrote:
>> On 28.09.2023 14:46, Simone Ballarin wrote:
>>> On 13/09/23 10:02, Jan Beulich wrote:
>>>> On 12.09.2023 11:36, Simone Ballarin 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) and the #if directive cannot
>>>>> be used for other checks.
>>>>>
>>>>> Simone Ballarin (10):
>>>>>     misra: add deviation for headers that explicitly avoid guards
>>>>>     misra: modify deviations for empty and generated headers
>>>>>     misra: add deviations for direct inclusion guards
>>>>>     xen/arm: address violations of MISRA C:2012 Directive 4.10
>>>>>     xen/x86: address violations of MISRA C:2012 Directive 4.10
>>>>>     x86/EFI: address violations of MISRA C:2012 Directive 4.10
>>>>>     xen/common: address violations of MISRA C:2012 Directive 4.10
>>>>>     xen/efi: address violations of MISRA C:2012 Directive 4.10
>>>>>     xen: address violations of MISRA C:2012 Directive 4.10
>>>>>     x86/asm: address violations of MISRA C:2012 Directive 4.10
>>>>
>>>> Just to mention it here again for the entire series, seeing that despite
>>>> my earlier comments to this effect a few R-b have arrived: If private
>>>> headers need to gain guards (for, imo, no real reason), we first need to
>>>> settle on a naming scheme for these guards, such that guards used in
>>>> private headers aren't at risk of colliding with ones used headers
>>>> living in one of the usual include directories. IOW imo fair parts of
>>>> this series may need redoing.
>>>>
>>>> Jan
>>>>
>>>
>>> My proposal is:
>>> - the relative path from "xen/arch" for files in this directory
>>>     (i.e. X86_X86_X86_MMCONFIG_H for "xen/arch/x86/x86_64/mmconfig.h";
>>
>> X86_X86_64_MMCONFIG_H that is?
>>
>> Yet then this scheme won't hold for xen/arch/include/asm/... ? It's also
>> not clear whether you're deliberately omitting leading/trailing underscores
>> here, which may be a way to distinguish private from global headers.
> 
> Each name that begins with a double or single underscore (__, _) 
> followed by an uppercase letter is reserved. Using a reserved identifier 
> is an undefined-b.
> 
> I would be better to avoid them.

I'm with you about avoiding them, except that we use such all over the
place. Taking this together with ...

>>> - for the others, the entire path.
>>
>> What exactly is "entire" here?
> 
> Let me try again.
> 
> If we are inside xen/arch the relative path starting from this directory:
>    | xen/arch/x86/include/asm/compat.h
>    X86_INCLUDE_ASM_COMPAT_H
> 
> For xen/include, the current convention.
> Maybe, in a future patch, we can consider removing the leading _.
> 
> For the others, the relative path after xen:
>    | xen/common/efi/efi.h
>    COMMON_EFI_EFI_H

... this you're effectively suggesting to change all existing guards.
That's an option, but likely not a preferred one. Personally I'd prefer
if in particular the headers in xen/include/ and in xen/arch/*include/
didn't needlessly include _INCLUDE_ in their guard names.

I'm really curious what others think.

Jan