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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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:

- 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__ ?