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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.09.2023 00:24, Stefano Stabellini wrote:
> On Thu, 28 Sep 2023, Jan Beulich wrote:
>> 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.
> 
> If it is a MISRA requirement to avoid names that begin with single or
> double underscore, then I think we should bite the bullet and change all
> guard names, taking the opportunity to make them consistent.

Note how below you still suggest names with two leading underscores. I'm
afraid ...

> If it is not a MISRA requirement, then I think we should go for the path
> of least resistance and try to make the smallest amount of changes
> overall, which seems to be:

... "least resistance" won't gain us much, as hardly any guards don't
start with an underscore.

> - for xen/include/blah.h, __BLAH_H__
> - for xen/arch/arm/asm/include/blah.h, __ASM_ARM_BLAH_H__
> - for xen/arch/x86/asm/include/blah.h, it is far less consistent, maybe 
> __ASM_X86_BLAH_H__ ?

There are no headers in xen/include/. For (e.g.) xen/include/xen/ we
may go with XEN_BLAH_H; whether ASM prefixes are needed I'm not sure;
we could go with just ARM_BLAH_H and X86_BLAH_H?

The primary question though is (imo) how to deal with private headers,
such that the risk of name collisions is as small as possible.

Jan