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

 .../eclair_analysis/ECLAIR/deviations.ecl     |  7 ----
 docs/misra/safe.json                          | 32 +++++++++++++++++++
 xen/arch/arm/efi/efi-boot.h                   |  6 ++++
 xen/arch/arm/efi/runtime.h                    |  1 +
 xen/arch/arm/include/asm/hypercall.h          |  1 +
 xen/arch/x86/Makefile                         |  8 ++---
 xen/arch/x86/cpu/cpu.h                        |  5 +++
 xen/arch/x86/efi/runtime.h                    |  5 +++
 xen/arch/x86/include/asm/compat.h             |  5 +++
 xen/arch/x86/include/asm/cpufeatures.h        |  5 +--
 xen/arch/x86/include/asm/efibind.h            |  5 +++
 xen/arch/x86/include/asm/hypercall.h          |  1 +
 xen/arch/x86/x86_64/mmconfig.h                |  5 +++
 xen/arch/x86/x86_emulate/private.h            |  5 +++
 xen/common/decompress.h                       |  5 +++
 xen/common/efi/efi.h                          |  5 +++
 xen/common/event_channel.h                    |  5 +++
 xen/include/Makefile                          | 10 ++++--
 xen/include/public/arch-x86/cpufeatureset.h   |  1 +
 xen/include/public/errno.h                    |  1 +
 xen/include/xen/err.h                         |  4 ++-
 xen/include/xen/pci_ids.h                     |  5 +++
 xen/include/xen/softirq.h                     |  4 ++-
 xen/include/xen/unaligned.h                   |  1 +
 xen/include/xen/vmap.h                        |  4 ++-
 25 files changed, 115 insertions(+), 21 deletions(-)

-- 
2.34.1

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

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";
- for the others, the entire path.

What do you think?

-- 
Simone Ballarin, M.Sc.

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

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.

> - for the others, the entire path.

What exactly is "entire" here?

Jan

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.

> 
>> - for the others, the entire path.
> 
> What exactly is "entire" here?
> 
> Jan

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



-- 
Simone Ballarin, M.Sc.

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

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

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

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

On Mon, 16 Oct 2023, Jan Beulich wrote:
> 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 ...

Sorry for the confusion. My example was an example of "path of least
resistance" trying to extrapolate the existing patterns. I was not
trying also to comply with the other MISRA rule about leading
underscores.

By "path of least resistance" I meant the smallest amount of work to
address Directive 4.10. As opposed to the "bite the bullet" approach
where we are willing to change everything to be consistent and also
address Directive 4.10.

So in my example of "path of least resistance" I kept the leading
underscores to minimize changes to the existing codebase.

The result of the MISRA C discussion this morning was that in general we
are *not* going to remove leading underscores everywhere, although
in general we would also like to avoid them when it can be done without
harming readability.

Now what does it mean for this patch series? See below for a proposal.


> > 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.

Looking at concrete examples under xen/include/xen:
xen/include/xen/mm.h __XEN_MM_H__
xen/include/xen/dm.h __XEN_DM_H__
xen/include/xen/hypfs.h __XEN_HYPFS_H__

So I think we should do for consistency:
xen/include/xen/blah.h __XEN_BLAH_H__

Even if we know the leading underscore are undesirable, in this case I
would prefer consistency.



On the other hand looking at ARM examples:
xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
xen/arch/arm/include/asm/time.h __ARM_TIME_H__
xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
xen/arch/arm/include/asm/io.h _ASM_IO_H

And also looking at x86 examples:
xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
xen/arch/x86/include/asm/io.h _ASM_IO_H

Thet are very inconsistent.


So for ARM and X86 headers I think we are free to pick anything we want,
including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
me.


For private headers such as:
xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H

More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
ARCH_X86_BLAH_H for new headers.

What do you think?

On 18.10.2023 02:48, Stefano Stabellini wrote:
> On Mon, 16 Oct 2023, Jan Beulich wrote:
>> On 29.09.2023 00:24, Stefano Stabellini wrote:
>>> 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.
> 
> Looking at concrete examples under xen/include/xen:
> xen/include/xen/mm.h __XEN_MM_H__
> xen/include/xen/dm.h __XEN_DM_H__
> xen/include/xen/hypfs.h __XEN_HYPFS_H__
> 
> So I think we should do for consistency:
> xen/include/xen/blah.h __XEN_BLAH_H__
> 
> Even if we know the leading underscore are undesirable, in this case I
> would prefer consistency.

I'm kind of okay with that. FTAOD - here and below you mean to make this
one explicit first exception from the "no new leading underscores" goal,
for newly added headers?

> On the other hand looking at ARM examples:
> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
> xen/arch/arm/include/asm/io.h _ASM_IO_H
> 
> And also looking at x86 examples:
> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
> xen/arch/x86/include/asm/io.h _ASM_IO_H
> 
> Thet are very inconsistent.
> 
> 
> So for ARM and X86 headers I think we are free to pick anything we want,
> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
> me.

To be honest, I'd prefer a global underlying pattern, i.e. if common
headers are "fine" to use leading underscores for guards, arch header
should, too.

> For private headers such as:
> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
> 
> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
> ARCH_X86_BLAH_H for new headers.

I'm afraid I don't like this, as deeper paths would lead to unwieldy
guard names. If we continue to use double-underscore prefixed names
in common and arch headers, why don't we demand no leading underscores
and no path-derived prefixes in private headers? That'll avoid any
collisions between the two groups.

Jan

On Wed, 18 Oct 2023, Jan Beulich wrote:
> On 18.10.2023 02:48, Stefano Stabellini wrote:
> > On Mon, 16 Oct 2023, Jan Beulich wrote:
> >> On 29.09.2023 00:24, Stefano Stabellini wrote:
> >>> 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.
> > 
> > Looking at concrete examples under xen/include/xen:
> > xen/include/xen/mm.h __XEN_MM_H__
> > xen/include/xen/dm.h __XEN_DM_H__
> > xen/include/xen/hypfs.h __XEN_HYPFS_H__
> > 
> > So I think we should do for consistency:
> > xen/include/xen/blah.h __XEN_BLAH_H__
> > 
> > Even if we know the leading underscore are undesirable, in this case I
> > would prefer consistency.
> 
> I'm kind of okay with that. FTAOD - here and below you mean to make this
> one explicit first exception from the "no new leading underscores" goal,
> for newly added headers?

Yes. The reason is for consistency with the existing header files.


> > On the other hand looking at ARM examples:
> > xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
> > xen/arch/arm/include/asm/time.h __ARM_TIME_H__
> > xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
> > xen/arch/arm/include/asm/io.h _ASM_IO_H
> > 
> > And also looking at x86 examples:
> > xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
> > xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
> > xen/arch/x86/include/asm/io.h _ASM_IO_H
> > 
> > Thet are very inconsistent.
> > 
> > 
> > So for ARM and X86 headers I think we are free to pick anything we want,
> > including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
> > me.
> 
> To be honest, I'd prefer a global underlying pattern, i.e. if common
> headers are "fine" to use leading underscores for guards, arch header
> should, too.

I am OK with that too. We could go with:
__ASM_ARM_BLAH_H__
__ASM_X86_BLAH_H__

I used "ASM" to make it easier to differentiate with the private headers
below. Also the version without "ASM" would work but it would only
differ with the private headers in terms of leading underscores. I
thought that also having "ASM" would help readability and help avoid
confusion.


> > For private headers such as:
> > xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
> > xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
> > xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
> > xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
> > 
> > More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
> > ARCH_X86_BLAH_H for new headers.
> 
> I'm afraid I don't like this, as deeper paths would lead to unwieldy
> guard names. If we continue to use double-underscore prefixed names
> in common and arch headers, why don't we demand no leading underscores
> and no path-derived prefixes in private headers? That'll avoid any
> collisions between the two groups.

OK, so for private headers:

ARM_BLAH_H
X86_BLAH_H

What that work for you?

On 19.10.2023 02:44, Stefano Stabellini wrote:
> On Wed, 18 Oct 2023, Jan Beulich wrote:
>> On 18.10.2023 02:48, Stefano Stabellini wrote:
>>> On Mon, 16 Oct 2023, Jan Beulich wrote:
>>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
>>>>> 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.
>>>
>>> Looking at concrete examples under xen/include/xen:
>>> xen/include/xen/mm.h __XEN_MM_H__
>>> xen/include/xen/dm.h __XEN_DM_H__
>>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
>>>
>>> So I think we should do for consistency:
>>> xen/include/xen/blah.h __XEN_BLAH_H__
>>>
>>> Even if we know the leading underscore are undesirable, in this case I
>>> would prefer consistency.
>>
>> I'm kind of okay with that. FTAOD - here and below you mean to make this
>> one explicit first exception from the "no new leading underscores" goal,
>> for newly added headers?
> 
> Yes. The reason is for consistency with the existing header files.
> 
> 
>>> On the other hand looking at ARM examples:
>>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
>>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
>>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
>>> xen/arch/arm/include/asm/io.h _ASM_IO_H
>>>
>>> And also looking at x86 examples:
>>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
>>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
>>> xen/arch/x86/include/asm/io.h _ASM_IO_H
>>>
>>> Thet are very inconsistent.
>>>
>>>
>>> So for ARM and X86 headers I think we are free to pick anything we want,
>>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
>>> me.
>>
>> To be honest, I'd prefer a global underlying pattern, i.e. if common
>> headers are "fine" to use leading underscores for guards, arch header
>> should, too.
> 
> I am OK with that too. We could go with:
> __ASM_ARM_BLAH_H__
> __ASM_X86_BLAH_H__
> 
> I used "ASM" to make it easier to differentiate with the private headers
> below. Also the version without "ASM" would work but it would only
> differ with the private headers in terms of leading underscores. I
> thought that also having "ASM" would help readability and help avoid
> confusion.
> 
> 
>>> For private headers such as:
>>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
>>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
>>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
>>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
>>>
>>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
>>> ARCH_X86_BLAH_H for new headers.
>>
>> I'm afraid I don't like this, as deeper paths would lead to unwieldy
>> guard names. If we continue to use double-underscore prefixed names
>> in common and arch headers, why don't we demand no leading underscores
>> and no path-derived prefixes in private headers? That'll avoid any
>> collisions between the two groups.
> 
> OK, so for private headers:
> 
> ARM_BLAH_H
> X86_BLAH_H
> 
> What that work for you?

What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
differently, how would you see e.g. common/decompress.h's guard named?

Jan

On Thu, 19 Oct 2023, Jan Beulich wrote:
> On 19.10.2023 02:44, Stefano Stabellini wrote:
> > On Wed, 18 Oct 2023, Jan Beulich wrote:
> >> On 18.10.2023 02:48, Stefano Stabellini wrote:
> >>> On Mon, 16 Oct 2023, Jan Beulich wrote:
> >>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
> >>>>> 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.
> >>>
> >>> Looking at concrete examples under xen/include/xen:
> >>> xen/include/xen/mm.h __XEN_MM_H__
> >>> xen/include/xen/dm.h __XEN_DM_H__
> >>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
> >>>
> >>> So I think we should do for consistency:
> >>> xen/include/xen/blah.h __XEN_BLAH_H__
> >>>
> >>> Even if we know the leading underscore are undesirable, in this case I
> >>> would prefer consistency.
> >>
> >> I'm kind of okay with that. FTAOD - here and below you mean to make this
> >> one explicit first exception from the "no new leading underscores" goal,
> >> for newly added headers?
> > 
> > Yes. The reason is for consistency with the existing header files.
> > 
> > 
> >>> On the other hand looking at ARM examples:
> >>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
> >>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
> >>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
> >>> xen/arch/arm/include/asm/io.h _ASM_IO_H
> >>>
> >>> And also looking at x86 examples:
> >>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
> >>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
> >>> xen/arch/x86/include/asm/io.h _ASM_IO_H
> >>>
> >>> Thet are very inconsistent.
> >>>
> >>>
> >>> So for ARM and X86 headers I think we are free to pick anything we want,
> >>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
> >>> me.
> >>
> >> To be honest, I'd prefer a global underlying pattern, i.e. if common
> >> headers are "fine" to use leading underscores for guards, arch header
> >> should, too.
> > 
> > I am OK with that too. We could go with:
> > __ASM_ARM_BLAH_H__
> > __ASM_X86_BLAH_H__
> > 
> > I used "ASM" to make it easier to differentiate with the private headers
> > below. Also the version without "ASM" would work but it would only
> > differ with the private headers in terms of leading underscores. I
> > thought that also having "ASM" would help readability and help avoid
> > confusion.
> > 
> > 
> >>> For private headers such as:
> >>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
> >>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
> >>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
> >>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
> >>>
> >>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
> >>> ARCH_X86_BLAH_H for new headers.
> >>
> >> I'm afraid I don't like this, as deeper paths would lead to unwieldy
> >> guard names. If we continue to use double-underscore prefixed names
> >> in common and arch headers, why don't we demand no leading underscores
> >> and no path-derived prefixes in private headers? That'll avoid any
> >> collisions between the two groups.
> > 
> > OK, so for private headers:
> > 
> > ARM_BLAH_H
> > X86_BLAH_H
> > 
> > What that work for you?
> 
> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
> differently, how would you see e.g. common/decompress.h's guard named?

I meant that:

xen/arch/arm/blah.h would use ARM_BLAH_H
xen/arch/x86/blah.h would use X86_BLAH_H

You have a good question on something like xen/common/decompress.h and
xen/common/event_channel.h.  What do you think about:

COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H

otherwise:

XEN_BLAH_H, so specifically XEN_DECOMPRESS_H

I prefer COMMON_BLAH_H but I think both versions are OK.

On 19.10.2023 18:19, Stefano Stabellini wrote:
> On Thu, 19 Oct 2023, Jan Beulich wrote:
>> On 19.10.2023 02:44, Stefano Stabellini wrote:
>>> On Wed, 18 Oct 2023, Jan Beulich wrote:
>>>> On 18.10.2023 02:48, Stefano Stabellini wrote:
>>>>> On Mon, 16 Oct 2023, Jan Beulich wrote:
>>>>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
>>>>>>> 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.
>>>>>
>>>>> Looking at concrete examples under xen/include/xen:
>>>>> xen/include/xen/mm.h __XEN_MM_H__
>>>>> xen/include/xen/dm.h __XEN_DM_H__
>>>>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
>>>>>
>>>>> So I think we should do for consistency:
>>>>> xen/include/xen/blah.h __XEN_BLAH_H__
>>>>>
>>>>> Even if we know the leading underscore are undesirable, in this case I
>>>>> would prefer consistency.
>>>>
>>>> I'm kind of okay with that. FTAOD - here and below you mean to make this
>>>> one explicit first exception from the "no new leading underscores" goal,
>>>> for newly added headers?
>>>
>>> Yes. The reason is for consistency with the existing header files.
>>>
>>>
>>>>> On the other hand looking at ARM examples:
>>>>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
>>>>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
>>>>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
>>>>> xen/arch/arm/include/asm/io.h _ASM_IO_H
>>>>>
>>>>> And also looking at x86 examples:
>>>>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
>>>>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
>>>>> xen/arch/x86/include/asm/io.h _ASM_IO_H
>>>>>
>>>>> Thet are very inconsistent.
>>>>>
>>>>>
>>>>> So for ARM and X86 headers I think we are free to pick anything we want,
>>>>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
>>>>> me.
>>>>
>>>> To be honest, I'd prefer a global underlying pattern, i.e. if common
>>>> headers are "fine" to use leading underscores for guards, arch header
>>>> should, too.
>>>
>>> I am OK with that too. We could go with:
>>> __ASM_ARM_BLAH_H__
>>> __ASM_X86_BLAH_H__
>>>
>>> I used "ASM" to make it easier to differentiate with the private headers
>>> below. Also the version without "ASM" would work but it would only
>>> differ with the private headers in terms of leading underscores. I
>>> thought that also having "ASM" would help readability and help avoid
>>> confusion.
>>>
>>>
>>>>> For private headers such as:
>>>>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
>>>>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
>>>>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
>>>>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
>>>>>
>>>>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
>>>>> ARCH_X86_BLAH_H for new headers.
>>>>
>>>> I'm afraid I don't like this, as deeper paths would lead to unwieldy
>>>> guard names. If we continue to use double-underscore prefixed names
>>>> in common and arch headers, why don't we demand no leading underscores
>>>> and no path-derived prefixes in private headers? That'll avoid any
>>>> collisions between the two groups.
>>>
>>> OK, so for private headers:
>>>
>>> ARM_BLAH_H
>>> X86_BLAH_H
>>>
>>> What that work for you?
>>
>> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
>> differently, how would you see e.g. common/decompress.h's guard named?
> 
> I meant that:
> 
> xen/arch/arm/blah.h would use ARM_BLAH_H
> xen/arch/x86/blah.h would use X86_BLAH_H
> 
> You have a good question on something like xen/common/decompress.h and
> xen/common/event_channel.h.  What do you think about:
> 
> COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H
> 
> otherwise:
> 
> XEN_BLAH_H, so specifically XEN_DECOMPRESS_H
> 
> I prefer COMMON_BLAH_H but I think both versions are OK.

IOW you disagree with my earlier "... and no path-derived prefixes",
and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence?
FTAOD my earlier suggestion was simply based on the observation that
the deeper the location of a header in the tree, the more unwieldy
its guard name would end up being if path prefixes were to be used.

Jan

On Fri, 20 Oct 2023, Jan Beulich wrote:
> On 19.10.2023 18:19, Stefano Stabellini wrote:
> > On Thu, 19 Oct 2023, Jan Beulich wrote:
> >> On 19.10.2023 02:44, Stefano Stabellini wrote:
> >>> On Wed, 18 Oct 2023, Jan Beulich wrote:
> >>>> On 18.10.2023 02:48, Stefano Stabellini wrote:
> >>>>> On Mon, 16 Oct 2023, Jan Beulich wrote:
> >>>>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
> >>>>>>> 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.
> >>>>>
> >>>>> Looking at concrete examples under xen/include/xen:
> >>>>> xen/include/xen/mm.h __XEN_MM_H__
> >>>>> xen/include/xen/dm.h __XEN_DM_H__
> >>>>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
> >>>>>
> >>>>> So I think we should do for consistency:
> >>>>> xen/include/xen/blah.h __XEN_BLAH_H__
> >>>>>
> >>>>> Even if we know the leading underscore are undesirable, in this case I
> >>>>> would prefer consistency.
> >>>>
> >>>> I'm kind of okay with that. FTAOD - here and below you mean to make this
> >>>> one explicit first exception from the "no new leading underscores" goal,
> >>>> for newly added headers?
> >>>
> >>> Yes. The reason is for consistency with the existing header files.
> >>>
> >>>
> >>>>> On the other hand looking at ARM examples:
> >>>>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
> >>>>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
> >>>>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
> >>>>> xen/arch/arm/include/asm/io.h _ASM_IO_H
> >>>>>
> >>>>> And also looking at x86 examples:
> >>>>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
> >>>>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
> >>>>> xen/arch/x86/include/asm/io.h _ASM_IO_H
> >>>>>
> >>>>> Thet are very inconsistent.
> >>>>>
> >>>>>
> >>>>> So for ARM and X86 headers I think we are free to pick anything we want,
> >>>>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
> >>>>> me.
> >>>>
> >>>> To be honest, I'd prefer a global underlying pattern, i.e. if common
> >>>> headers are "fine" to use leading underscores for guards, arch header
> >>>> should, too.
> >>>
> >>> I am OK with that too. We could go with:
> >>> __ASM_ARM_BLAH_H__
> >>> __ASM_X86_BLAH_H__
> >>>
> >>> I used "ASM" to make it easier to differentiate with the private headers
> >>> below. Also the version without "ASM" would work but it would only
> >>> differ with the private headers in terms of leading underscores. I
> >>> thought that also having "ASM" would help readability and help avoid
> >>> confusion.
> >>>
> >>>
> >>>>> For private headers such as:
> >>>>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
> >>>>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
> >>>>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
> >>>>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
> >>>>>
> >>>>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
> >>>>> ARCH_X86_BLAH_H for new headers.
> >>>>
> >>>> I'm afraid I don't like this, as deeper paths would lead to unwieldy
> >>>> guard names. If we continue to use double-underscore prefixed names
> >>>> in common and arch headers, why don't we demand no leading underscores
> >>>> and no path-derived prefixes in private headers? That'll avoid any
> >>>> collisions between the two groups.
> >>>
> >>> OK, so for private headers:
> >>>
> >>> ARM_BLAH_H
> >>> X86_BLAH_H
> >>>
> >>> What that work for you?
> >>
> >> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
> >> differently, how would you see e.g. common/decompress.h's guard named?
> > 
> > I meant that:
> > 
> > xen/arch/arm/blah.h would use ARM_BLAH_H
> > xen/arch/x86/blah.h would use X86_BLAH_H
> > 
> > You have a good question on something like xen/common/decompress.h and
> > xen/common/event_channel.h.  What do you think about:
> > 
> > COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H
> > 
> > otherwise:
> > 
> > XEN_BLAH_H, so specifically XEN_DECOMPRESS_H
> > 
> > I prefer COMMON_BLAH_H but I think both versions are OK.
> 
> IOW you disagree with my earlier "... and no path-derived prefixes",
> and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence?
> FTAOD my earlier suggestion was simply based on the observation that
> the deeper the location of a header in the tree, the more unwieldy
> its guard name would end up being if path prefixes were to be used.

I don't have a strong opinion on "path-derived prefixes". I prefer
consistency and easy-to-figure-out guidelines over shortness.

The advantage of a path-derived prefix is that it is trivial to figure
out at first glance. If we can come up with another system that is also
easy then fine. Do you have a suggestion? If so, sorry I missed it.

On 21.10.2023 01:26, Stefano Stabellini wrote:
> On Fri, 20 Oct 2023, Jan Beulich wrote:
>> On 19.10.2023 18:19, Stefano Stabellini wrote:
>>> On Thu, 19 Oct 2023, Jan Beulich wrote:
>>>> On 19.10.2023 02:44, Stefano Stabellini wrote:
>>>>> On Wed, 18 Oct 2023, Jan Beulich wrote:
>>>>>> On 18.10.2023 02:48, Stefano Stabellini wrote:
>>>>>>> On Mon, 16 Oct 2023, Jan Beulich wrote:
>>>>>>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
>>>>>>>>> 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.
>>>>>>>
>>>>>>> Looking at concrete examples under xen/include/xen:
>>>>>>> xen/include/xen/mm.h __XEN_MM_H__
>>>>>>> xen/include/xen/dm.h __XEN_DM_H__
>>>>>>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
>>>>>>>
>>>>>>> So I think we should do for consistency:
>>>>>>> xen/include/xen/blah.h __XEN_BLAH_H__
>>>>>>>
>>>>>>> Even if we know the leading underscore are undesirable, in this case I
>>>>>>> would prefer consistency.
>>>>>>
>>>>>> I'm kind of okay with that. FTAOD - here and below you mean to make this
>>>>>> one explicit first exception from the "no new leading underscores" goal,
>>>>>> for newly added headers?
>>>>>
>>>>> Yes. The reason is for consistency with the existing header files.
>>>>>
>>>>>
>>>>>>> On the other hand looking at ARM examples:
>>>>>>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
>>>>>>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
>>>>>>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
>>>>>>> xen/arch/arm/include/asm/io.h _ASM_IO_H
>>>>>>>
>>>>>>> And also looking at x86 examples:
>>>>>>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
>>>>>>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
>>>>>>> xen/arch/x86/include/asm/io.h _ASM_IO_H
>>>>>>>
>>>>>>> Thet are very inconsistent.
>>>>>>>
>>>>>>>
>>>>>>> So for ARM and X86 headers I think we are free to pick anything we want,
>>>>>>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
>>>>>>> me.
>>>>>>
>>>>>> To be honest, I'd prefer a global underlying pattern, i.e. if common
>>>>>> headers are "fine" to use leading underscores for guards, arch header
>>>>>> should, too.
>>>>>
>>>>> I am OK with that too. We could go with:
>>>>> __ASM_ARM_BLAH_H__
>>>>> __ASM_X86_BLAH_H__
>>>>>
>>>>> I used "ASM" to make it easier to differentiate with the private headers
>>>>> below. Also the version without "ASM" would work but it would only
>>>>> differ with the private headers in terms of leading underscores. I
>>>>> thought that also having "ASM" would help readability and help avoid
>>>>> confusion.
>>>>>
>>>>>
>>>>>>> For private headers such as:
>>>>>>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
>>>>>>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
>>>>>>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
>>>>>>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
>>>>>>>
>>>>>>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
>>>>>>> ARCH_X86_BLAH_H for new headers.
>>>>>>
>>>>>> I'm afraid I don't like this, as deeper paths would lead to unwieldy
>>>>>> guard names. If we continue to use double-underscore prefixed names
>>>>>> in common and arch headers, why don't we demand no leading underscores
>>>>>> and no path-derived prefixes in private headers? That'll avoid any
>>>>>> collisions between the two groups.
>>>>>
>>>>> OK, so for private headers:
>>>>>
>>>>> ARM_BLAH_H
>>>>> X86_BLAH_H
>>>>>
>>>>> What that work for you?
>>>>
>>>> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
>>>> differently, how would you see e.g. common/decompress.h's guard named?
>>>
>>> I meant that:
>>>
>>> xen/arch/arm/blah.h would use ARM_BLAH_H
>>> xen/arch/x86/blah.h would use X86_BLAH_H
>>>
>>> You have a good question on something like xen/common/decompress.h and
>>> xen/common/event_channel.h.  What do you think about:
>>>
>>> COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H
>>>
>>> otherwise:
>>>
>>> XEN_BLAH_H, so specifically XEN_DECOMPRESS_H
>>>
>>> I prefer COMMON_BLAH_H but I think both versions are OK.
>>
>> IOW you disagree with my earlier "... and no path-derived prefixes",
>> and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence?
>> FTAOD my earlier suggestion was simply based on the observation that
>> the deeper the location of a header in the tree, the more unwieldy
>> its guard name would end up being if path prefixes were to be used.
> 
> I don't have a strong opinion on "path-derived prefixes". I prefer
> consistency and easy-to-figure-out guidelines over shortness.
> 
> The advantage of a path-derived prefix is that it is trivial to figure
> out at first glance. If we can come up with another system that is also
> easy then fine. Do you have a suggestion? If so, sorry I missed it.

Well, I kind of implicitly suggested "no path derived prefixes for private
headers", albeit realizing that there's a chance then of guards colliding.
I can't think of a good scheme which would fit all goals (no collisions,
uniformity, and not unduly long).

Jan

On Mon, 23 Oct 2023, Jan Beulich wrote:
> On 21.10.2023 01:26, Stefano Stabellini wrote:
> > On Fri, 20 Oct 2023, Jan Beulich wrote:
> >> On 19.10.2023 18:19, Stefano Stabellini wrote:
> >>> On Thu, 19 Oct 2023, Jan Beulich wrote:
> >>>> On 19.10.2023 02:44, Stefano Stabellini wrote:
> >>>>> On Wed, 18 Oct 2023, Jan Beulich wrote:
> >>>>>> On 18.10.2023 02:48, Stefano Stabellini wrote:
> >>>>>>> On Mon, 16 Oct 2023, Jan Beulich wrote:
> >>>>>>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
> >>>>>>>>> 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.
> >>>>>>>
> >>>>>>> Looking at concrete examples under xen/include/xen:
> >>>>>>> xen/include/xen/mm.h __XEN_MM_H__
> >>>>>>> xen/include/xen/dm.h __XEN_DM_H__
> >>>>>>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
> >>>>>>>
> >>>>>>> So I think we should do for consistency:
> >>>>>>> xen/include/xen/blah.h __XEN_BLAH_H__
> >>>>>>>
> >>>>>>> Even if we know the leading underscore are undesirable, in this case I
> >>>>>>> would prefer consistency.
> >>>>>>
> >>>>>> I'm kind of okay with that. FTAOD - here and below you mean to make this
> >>>>>> one explicit first exception from the "no new leading underscores" goal,
> >>>>>> for newly added headers?
> >>>>>
> >>>>> Yes. The reason is for consistency with the existing header files.
> >>>>>
> >>>>>
> >>>>>>> On the other hand looking at ARM examples:
> >>>>>>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
> >>>>>>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
> >>>>>>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
> >>>>>>> xen/arch/arm/include/asm/io.h _ASM_IO_H
> >>>>>>>
> >>>>>>> And also looking at x86 examples:
> >>>>>>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
> >>>>>>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
> >>>>>>> xen/arch/x86/include/asm/io.h _ASM_IO_H
> >>>>>>>
> >>>>>>> Thet are very inconsistent.
> >>>>>>>
> >>>>>>>
> >>>>>>> So for ARM and X86 headers I think we are free to pick anything we want,
> >>>>>>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
> >>>>>>> me.
> >>>>>>
> >>>>>> To be honest, I'd prefer a global underlying pattern, i.e. if common
> >>>>>> headers are "fine" to use leading underscores for guards, arch header
> >>>>>> should, too.
> >>>>>
> >>>>> I am OK with that too. We could go with:
> >>>>> __ASM_ARM_BLAH_H__
> >>>>> __ASM_X86_BLAH_H__
> >>>>>
> >>>>> I used "ASM" to make it easier to differentiate with the private headers
> >>>>> below. Also the version without "ASM" would work but it would only
> >>>>> differ with the private headers in terms of leading underscores. I
> >>>>> thought that also having "ASM" would help readability and help avoid
> >>>>> confusion.
> >>>>>
> >>>>>
> >>>>>>> For private headers such as:
> >>>>>>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
> >>>>>>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
> >>>>>>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
> >>>>>>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
> >>>>>>>
> >>>>>>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
> >>>>>>> ARCH_X86_BLAH_H for new headers.
> >>>>>>
> >>>>>> I'm afraid I don't like this, as deeper paths would lead to unwieldy
> >>>>>> guard names. If we continue to use double-underscore prefixed names
> >>>>>> in common and arch headers, why don't we demand no leading underscores
> >>>>>> and no path-derived prefixes in private headers? That'll avoid any
> >>>>>> collisions between the two groups.
> >>>>>
> >>>>> OK, so for private headers:
> >>>>>
> >>>>> ARM_BLAH_H
> >>>>> X86_BLAH_H
> >>>>>
> >>>>> What that work for you?
> >>>>
> >>>> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
> >>>> differently, how would you see e.g. common/decompress.h's guard named?
> >>>
> >>> I meant that:
> >>>
> >>> xen/arch/arm/blah.h would use ARM_BLAH_H
> >>> xen/arch/x86/blah.h would use X86_BLAH_H
> >>>
> >>> You have a good question on something like xen/common/decompress.h and
> >>> xen/common/event_channel.h.  What do you think about:
> >>>
> >>> COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H
> >>>
> >>> otherwise:
> >>>
> >>> XEN_BLAH_H, so specifically XEN_DECOMPRESS_H
> >>>
> >>> I prefer COMMON_BLAH_H but I think both versions are OK.
> >>
> >> IOW you disagree with my earlier "... and no path-derived prefixes",
> >> and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence?
> >> FTAOD my earlier suggestion was simply based on the observation that
> >> the deeper the location of a header in the tree, the more unwieldy
> >> its guard name would end up being if path prefixes were to be used.
> > 
> > I don't have a strong opinion on "path-derived prefixes". I prefer
> > consistency and easy-to-figure-out guidelines over shortness.
> > 
> > The advantage of a path-derived prefix is that it is trivial to figure
> > out at first glance. If we can come up with another system that is also
> > easy then fine. Do you have a suggestion? If so, sorry I missed it.
> 
> Well, I kind of implicitly suggested "no path derived prefixes for private
> headers", albeit realizing that there's a chance then of guards colliding.
> I can't think of a good scheme which would fit all goals (no collisions,
> uniformity, and not unduly long).

Here I think we would benefit from a third opinion. Julien? Anyone?

Hi Stefano,

On 23/10/2023 21:47, Stefano Stabellini wrote:
> On Mon, 23 Oct 2023, Jan Beulich wrote:
>> On 21.10.2023 01:26, Stefano Stabellini wrote:
>>> On Fri, 20 Oct 2023, Jan Beulich wrote:
>>>> On 19.10.2023 18:19, Stefano Stabellini wrote:
>>>>> On Thu, 19 Oct 2023, Jan Beulich wrote:
>>>>>> On 19.10.2023 02:44, Stefano Stabellini wrote:
>>>>>>> On Wed, 18 Oct 2023, Jan Beulich wrote:
>>>>>>>> On 18.10.2023 02:48, Stefano Stabellini wrote:
>>>>>>>>> On Mon, 16 Oct 2023, Jan Beulich wrote:
>>>>>>>>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
>>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> Looking at concrete examples under xen/include/xen:
>>>>>>>>> xen/include/xen/mm.h __XEN_MM_H__
>>>>>>>>> xen/include/xen/dm.h __XEN_DM_H__
>>>>>>>>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
>>>>>>>>>
>>>>>>>>> So I think we should do for consistency:
>>>>>>>>> xen/include/xen/blah.h __XEN_BLAH_H__
>>>>>>>>>
>>>>>>>>> Even if we know the leading underscore are undesirable, in this case I
>>>>>>>>> would prefer consistency.
>>>>>>>>
>>>>>>>> I'm kind of okay with that. FTAOD - here and below you mean to make this
>>>>>>>> one explicit first exception from the "no new leading underscores" goal,
>>>>>>>> for newly added headers?
>>>>>>>
>>>>>>> Yes. The reason is for consistency with the existing header files.
>>>>>>>
>>>>>>>
>>>>>>>>> On the other hand looking at ARM examples:
>>>>>>>>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
>>>>>>>>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
>>>>>>>>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
>>>>>>>>> xen/arch/arm/include/asm/io.h _ASM_IO_H
>>>>>>>>>
>>>>>>>>> And also looking at x86 examples:
>>>>>>>>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
>>>>>>>>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
>>>>>>>>> xen/arch/x86/include/asm/io.h _ASM_IO_H
>>>>>>>>>
>>>>>>>>> Thet are very inconsistent.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> So for ARM and X86 headers I think we are free to pick anything we want,
>>>>>>>>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
>>>>>>>>> me.
>>>>>>>>
>>>>>>>> To be honest, I'd prefer a global underlying pattern, i.e. if common
>>>>>>>> headers are "fine" to use leading underscores for guards, arch header
>>>>>>>> should, too.
>>>>>>>
>>>>>>> I am OK with that too. We could go with:
>>>>>>> __ASM_ARM_BLAH_H__
>>>>>>> __ASM_X86_BLAH_H__
>>>>>>>
>>>>>>> I used "ASM" to make it easier to differentiate with the private headers
>>>>>>> below. Also the version without "ASM" would work but it would only
>>>>>>> differ with the private headers in terms of leading underscores. I
>>>>>>> thought that also having "ASM" would help readability and help avoid
>>>>>>> confusion.
>>>>>>>
>>>>>>>
>>>>>>>>> For private headers such as:
>>>>>>>>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
>>>>>>>>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
>>>>>>>>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
>>>>>>>>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
>>>>>>>>>
>>>>>>>>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
>>>>>>>>> ARCH_X86_BLAH_H for new headers.
>>>>>>>>
>>>>>>>> I'm afraid I don't like this, as deeper paths would lead to unwieldy
>>>>>>>> guard names. If we continue to use double-underscore prefixed names
>>>>>>>> in common and arch headers, why don't we demand no leading underscores
>>>>>>>> and no path-derived prefixes in private headers? That'll avoid any
>>>>>>>> collisions between the two groups.
>>>>>>>
>>>>>>> OK, so for private headers:
>>>>>>>
>>>>>>> ARM_BLAH_H
>>>>>>> X86_BLAH_H
>>>>>>>
>>>>>>> What that work for you?
>>>>>>
>>>>>> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
>>>>>> differently, how would you see e.g. common/decompress.h's guard named?
>>>>>
>>>>> I meant that:
>>>>>
>>>>> xen/arch/arm/blah.h would use ARM_BLAH_H
>>>>> xen/arch/x86/blah.h would use X86_BLAH_H
>>>>>
>>>>> You have a good question on something like xen/common/decompress.h and
>>>>> xen/common/event_channel.h.  What do you think about:
>>>>>
>>>>> COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H
>>>>>
>>>>> otherwise:
>>>>>
>>>>> XEN_BLAH_H, so specifically XEN_DECOMPRESS_H
>>>>>
>>>>> I prefer COMMON_BLAH_H but I think both versions are OK.
>>>>
>>>> IOW you disagree with my earlier "... and no path-derived prefixes",
>>>> and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence?
>>>> FTAOD my earlier suggestion was simply based on the observation that
>>>> the deeper the location of a header in the tree, the more unwieldy
>>>> its guard name would end up being if path prefixes were to be used.
>>>
>>> I don't have a strong opinion on "path-derived prefixes". I prefer
>>> consistency and easy-to-figure-out guidelines over shortness.

We adopted the MISRA Rule 5.4 which imposed us a limit (40 for Xen) on 
the number of characters for macros. AFAIU, this would apply to guards.

In the example provided by Jan (DRIVERS_PASSTHROUGH_VTD_DMAR_H), this is 
already 31 characters. So this is quite close to the limit.

>>>
>>> The advantage of a path-derived prefix is that it is trivial to figure
>>> out at first glance. If we can come up with another system that is also
>>> easy then fine. Do you have a suggestion? If so, sorry I missed it.
>>
>> Well, I kind of implicitly suggested "no path derived prefixes for private
>> headers", albeit realizing that there's a chance then of guards colliding.
>> I can't think of a good scheme which would fit all goals (no collisions,
>> uniformity, and not unduly long).
> 
> Here I think we would benefit from a third opinion. Julien? Anyone?

Just to confirm, the opinion is only for private headers. You have an 
agreement for the rest, is it correct?

If so, then I think we need to have shorter names for guard to avoid 
hitting the 40 characters limit. I can't think of a way to have a common 
scheme between private and common headers. So I would consider to have a 
separate scheme.

I am not sure if you or Jan already proposed an alternative scheme.

Cheers,

-- 
Julien Grall

On 24.10.2023 15:31, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/10/2023 21:47, Stefano Stabellini wrote:
>> On Mon, 23 Oct 2023, Jan Beulich wrote:
>>> On 21.10.2023 01:26, Stefano Stabellini wrote:
>>>> On Fri, 20 Oct 2023, Jan Beulich wrote:
>>>>> On 19.10.2023 18:19, Stefano Stabellini wrote:
>>>>>> On Thu, 19 Oct 2023, Jan Beulich wrote:
>>>>>>> On 19.10.2023 02:44, Stefano Stabellini wrote:
>>>>>>>> On Wed, 18 Oct 2023, Jan Beulich wrote:
>>>>>>>>> On 18.10.2023 02:48, Stefano Stabellini wrote:
>>>>>>>>>> On Mon, 16 Oct 2023, Jan Beulich wrote:
>>>>>>>>>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
>>>>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>>> Looking at concrete examples under xen/include/xen:
>>>>>>>>>> xen/include/xen/mm.h __XEN_MM_H__
>>>>>>>>>> xen/include/xen/dm.h __XEN_DM_H__
>>>>>>>>>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
>>>>>>>>>>
>>>>>>>>>> So I think we should do for consistency:
>>>>>>>>>> xen/include/xen/blah.h __XEN_BLAH_H__
>>>>>>>>>>
>>>>>>>>>> Even if we know the leading underscore are undesirable, in this case I
>>>>>>>>>> would prefer consistency.
>>>>>>>>>
>>>>>>>>> I'm kind of okay with that. FTAOD - here and below you mean to make this
>>>>>>>>> one explicit first exception from the "no new leading underscores" goal,
>>>>>>>>> for newly added headers?
>>>>>>>>
>>>>>>>> Yes. The reason is for consistency with the existing header files.
>>>>>>>>
>>>>>>>>
>>>>>>>>>> On the other hand looking at ARM examples:
>>>>>>>>>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
>>>>>>>>>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
>>>>>>>>>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
>>>>>>>>>> xen/arch/arm/include/asm/io.h _ASM_IO_H
>>>>>>>>>>
>>>>>>>>>> And also looking at x86 examples:
>>>>>>>>>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
>>>>>>>>>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
>>>>>>>>>> xen/arch/x86/include/asm/io.h _ASM_IO_H
>>>>>>>>>>
>>>>>>>>>> Thet are very inconsistent.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> So for ARM and X86 headers I think we are free to pick anything we want,
>>>>>>>>>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
>>>>>>>>>> me.
>>>>>>>>>
>>>>>>>>> To be honest, I'd prefer a global underlying pattern, i.e. if common
>>>>>>>>> headers are "fine" to use leading underscores for guards, arch header
>>>>>>>>> should, too.
>>>>>>>>
>>>>>>>> I am OK with that too. We could go with:
>>>>>>>> __ASM_ARM_BLAH_H__
>>>>>>>> __ASM_X86_BLAH_H__
>>>>>>>>
>>>>>>>> I used "ASM" to make it easier to differentiate with the private headers
>>>>>>>> below. Also the version without "ASM" would work but it would only
>>>>>>>> differ with the private headers in terms of leading underscores. I
>>>>>>>> thought that also having "ASM" would help readability and help avoid
>>>>>>>> confusion.
>>>>>>>>
>>>>>>>>
>>>>>>>>>> For private headers such as:
>>>>>>>>>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
>>>>>>>>>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
>>>>>>>>>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
>>>>>>>>>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
>>>>>>>>>>
>>>>>>>>>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
>>>>>>>>>> ARCH_X86_BLAH_H for new headers.
>>>>>>>>>
>>>>>>>>> I'm afraid I don't like this, as deeper paths would lead to unwieldy
>>>>>>>>> guard names. If we continue to use double-underscore prefixed names
>>>>>>>>> in common and arch headers, why don't we demand no leading underscores
>>>>>>>>> and no path-derived prefixes in private headers? That'll avoid any
>>>>>>>>> collisions between the two groups.
>>>>>>>>
>>>>>>>> OK, so for private headers:
>>>>>>>>
>>>>>>>> ARM_BLAH_H
>>>>>>>> X86_BLAH_H
>>>>>>>>
>>>>>>>> What that work for you?
>>>>>>>
>>>>>>> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
>>>>>>> differently, how would you see e.g. common/decompress.h's guard named?
>>>>>>
>>>>>> I meant that:
>>>>>>
>>>>>> xen/arch/arm/blah.h would use ARM_BLAH_H
>>>>>> xen/arch/x86/blah.h would use X86_BLAH_H
>>>>>>
>>>>>> You have a good question on something like xen/common/decompress.h and
>>>>>> xen/common/event_channel.h.  What do you think about:
>>>>>>
>>>>>> COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H
>>>>>>
>>>>>> otherwise:
>>>>>>
>>>>>> XEN_BLAH_H, so specifically XEN_DECOMPRESS_H
>>>>>>
>>>>>> I prefer COMMON_BLAH_H but I think both versions are OK.
>>>>>
>>>>> IOW you disagree with my earlier "... and no path-derived prefixes",
>>>>> and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence?
>>>>> FTAOD my earlier suggestion was simply based on the observation that
>>>>> the deeper the location of a header in the tree, the more unwieldy
>>>>> its guard name would end up being if path prefixes were to be used.
>>>>
>>>> I don't have a strong opinion on "path-derived prefixes". I prefer
>>>> consistency and easy-to-figure-out guidelines over shortness.
> 
> We adopted the MISRA Rule 5.4 which imposed us a limit (40 for Xen) on 
> the number of characters for macros. AFAIU, this would apply to guards.
> 
> In the example provided by Jan (DRIVERS_PASSTHROUGH_VTD_DMAR_H), this is 
> already 31 characters. So this is quite close to the limit.
> 
>>>>
>>>> The advantage of a path-derived prefix is that it is trivial to figure
>>>> out at first glance. If we can come up with another system that is also
>>>> easy then fine. Do you have a suggestion? If so, sorry I missed it.
>>>
>>> Well, I kind of implicitly suggested "no path derived prefixes for private
>>> headers", albeit realizing that there's a chance then of guards colliding.
>>> I can't think of a good scheme which would fit all goals (no collisions,
>>> uniformity, and not unduly long).
>>
>> Here I think we would benefit from a third opinion. Julien? Anyone?
> 
> Just to confirm, the opinion is only for private headers. You have an 
> agreement for the rest, is it correct?
> 
> If so, then I think we need to have shorter names for guard to avoid 
> hitting the 40 characters limit. I can't think of a way to have a common 
> scheme between private and common headers. So I would consider to have a 
> separate scheme.
> 
> I am not sure if you or Jan already proposed an alternative scheme.

Well, my suggestion was to derive from just the file name (no path
components) for them. But I pointed out that this may lead to collisions
when two or more private headers of the same name exist, and a CU ends
up wanting to include any two of them. Adding in the leaf-most path
component only might get us far enough to avoid collisions in practice,
while at the same time not resulting in overly long guard names.

Jan

On Tue, 24 Oct 2023, Jan Beulich wrote:
> On 24.10.2023 15:31, Julien Grall wrote:
> > Hi Stefano,
> > 
> > On 23/10/2023 21:47, Stefano Stabellini wrote:
> >> On Mon, 23 Oct 2023, Jan Beulich wrote:
> >>> On 21.10.2023 01:26, Stefano Stabellini wrote:
> >>>> On Fri, 20 Oct 2023, Jan Beulich wrote:
> >>>>> On 19.10.2023 18:19, Stefano Stabellini wrote:
> >>>>>> On Thu, 19 Oct 2023, Jan Beulich wrote:
> >>>>>>> On 19.10.2023 02:44, Stefano Stabellini wrote:
> >>>>>>>> On Wed, 18 Oct 2023, Jan Beulich wrote:
> >>>>>>>>> On 18.10.2023 02:48, Stefano Stabellini wrote:
> >>>>>>>>>> On Mon, 16 Oct 2023, Jan Beulich wrote:
> >>>>>>>>>>> On 29.09.2023 00:24, Stefano Stabellini wrote:
> >>>>>>>>>>>> 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.
> >>>>>>>>>>
> >>>>>>>>>> Looking at concrete examples under xen/include/xen:
> >>>>>>>>>> xen/include/xen/mm.h __XEN_MM_H__
> >>>>>>>>>> xen/include/xen/dm.h __XEN_DM_H__
> >>>>>>>>>> xen/include/xen/hypfs.h __XEN_HYPFS_H__
> >>>>>>>>>>
> >>>>>>>>>> So I think we should do for consistency:
> >>>>>>>>>> xen/include/xen/blah.h __XEN_BLAH_H__
> >>>>>>>>>>
> >>>>>>>>>> Even if we know the leading underscore are undesirable, in this case I
> >>>>>>>>>> would prefer consistency.
> >>>>>>>>>
> >>>>>>>>> I'm kind of okay with that. FTAOD - here and below you mean to make this
> >>>>>>>>> one explicit first exception from the "no new leading underscores" goal,
> >>>>>>>>> for newly added headers?
> >>>>>>>>
> >>>>>>>> Yes. The reason is for consistency with the existing header files.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>> On the other hand looking at ARM examples:
> >>>>>>>>>> xen/arch/arm/include/asm/traps.h __ASM_ARM_TRAPS__
> >>>>>>>>>> xen/arch/arm/include/asm/time.h __ARM_TIME_H__
> >>>>>>>>>> xen/arch/arm/include/asm/sysregs.h __ASM_ARM_SYSREGS_H
> >>>>>>>>>> xen/arch/arm/include/asm/io.h _ASM_IO_H
> >>>>>>>>>>
> >>>>>>>>>> And also looking at x86 examples:
> >>>>>>>>>> xen/arch/x86/include/asm/paging.h _XEN_PAGING_H
> >>>>>>>>>> xen/arch/x86/include/asm/p2m.h _XEN_ASM_X86_P2M_H
> >>>>>>>>>> xen/arch/x86/include/asm/io.h _ASM_IO_H
> >>>>>>>>>>
> >>>>>>>>>> Thet are very inconsistent.
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> So for ARM and X86 headers I think we are free to pick anything we want,
> >>>>>>>>>> including your suggested ARM_BLAH_H and X86_BLAH_H. Those are fine by
> >>>>>>>>>> me.
> >>>>>>>>>
> >>>>>>>>> To be honest, I'd prefer a global underlying pattern, i.e. if common
> >>>>>>>>> headers are "fine" to use leading underscores for guards, arch header
> >>>>>>>>> should, too.
> >>>>>>>>
> >>>>>>>> I am OK with that too. We could go with:
> >>>>>>>> __ASM_ARM_BLAH_H__
> >>>>>>>> __ASM_X86_BLAH_H__
> >>>>>>>>
> >>>>>>>> I used "ASM" to make it easier to differentiate with the private headers
> >>>>>>>> below. Also the version without "ASM" would work but it would only
> >>>>>>>> differ with the private headers in terms of leading underscores. I
> >>>>>>>> thought that also having "ASM" would help readability and help avoid
> >>>>>>>> confusion.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>> For private headers such as:
> >>>>>>>>>> xen/arch/arm/vuart.h __ARCH_ARM_VUART_H__
> >>>>>>>>>> xen/arch/arm/decode.h __ARCH_ARM_DECODE_H_
> >>>>>>>>>> xen/arch/x86/mm/p2m.h __ARCH_MM_P2M_H__
> >>>>>>>>>> xen/arch/x86/hvm/viridian/private.h X86_HVM_VIRIDIAN_PRIVATE_H
> >>>>>>>>>>
> >>>>>>>>>> More similar but still inconsistent. I would go with ARCH_ARM_BLAH_H and
> >>>>>>>>>> ARCH_X86_BLAH_H for new headers.
> >>>>>>>>>
> >>>>>>>>> I'm afraid I don't like this, as deeper paths would lead to unwieldy
> >>>>>>>>> guard names. If we continue to use double-underscore prefixed names
> >>>>>>>>> in common and arch headers, why don't we demand no leading underscores
> >>>>>>>>> and no path-derived prefixes in private headers? That'll avoid any
> >>>>>>>>> collisions between the two groups.
> >>>>>>>>
> >>>>>>>> OK, so for private headers:
> >>>>>>>>
> >>>>>>>> ARM_BLAH_H
> >>>>>>>> X86_BLAH_H
> >>>>>>>>
> >>>>>>>> What that work for you?
> >>>>>>>
> >>>>>>> What are the ARM_ and X86_ prefixes supposed to indicate here? Or to ask
> >>>>>>> differently, how would you see e.g. common/decompress.h's guard named?
> >>>>>>
> >>>>>> I meant that:
> >>>>>>
> >>>>>> xen/arch/arm/blah.h would use ARM_BLAH_H
> >>>>>> xen/arch/x86/blah.h would use X86_BLAH_H
> >>>>>>
> >>>>>> You have a good question on something like xen/common/decompress.h and
> >>>>>> xen/common/event_channel.h.  What do you think about:
> >>>>>>
> >>>>>> COMMON_BLAH_H, so specifically COMMON_DECOMPRESS_H
> >>>>>>
> >>>>>> otherwise:
> >>>>>>
> >>>>>> XEN_BLAH_H, so specifically XEN_DECOMPRESS_H
> >>>>>>
> >>>>>> I prefer COMMON_BLAH_H but I think both versions are OK.
> >>>>>
> >>>>> IOW you disagree with my earlier "... and no path-derived prefixes",
> >>>>> and you prefer e.g. DRIVERS_PASSTHROUGH_VTD_DMAR_H as a consequence?
> >>>>> FTAOD my earlier suggestion was simply based on the observation that
> >>>>> the deeper the location of a header in the tree, the more unwieldy
> >>>>> its guard name would end up being if path prefixes were to be used.
> >>>>
> >>>> I don't have a strong opinion on "path-derived prefixes". I prefer
> >>>> consistency and easy-to-figure-out guidelines over shortness.
> > 
> > We adopted the MISRA Rule 5.4 which imposed us a limit (40 for Xen) on 
> > the number of characters for macros. AFAIU, this would apply to guards.
> > 
> > In the example provided by Jan (DRIVERS_PASSTHROUGH_VTD_DMAR_H), this is 
> > already 31 characters. So this is quite close to the limit.
> > 
> >>>>
> >>>> The advantage of a path-derived prefix is that it is trivial to figure
> >>>> out at first glance. If we can come up with another system that is also
> >>>> easy then fine. Do you have a suggestion? If so, sorry I missed it.
> >>>
> >>> Well, I kind of implicitly suggested "no path derived prefixes for private
> >>> headers", albeit realizing that there's a chance then of guards colliding.
> >>> I can't think of a good scheme which would fit all goals (no collisions,
> >>> uniformity, and not unduly long).
> >>
> >> Here I think we would benefit from a third opinion. Julien? Anyone?
> > 
> > Just to confirm, the opinion is only for private headers. You have an 
> > agreement for the rest, is it correct?

Yes


> > If so, then I think we need to have shorter names for guard to avoid 
> > hitting the 40 characters limit. I can't think of a way to have a common 
> > scheme between private and common headers. So I would consider to have a 
> > separate scheme.
> > 
> > I am not sure if you or Jan already proposed an alternative scheme.
> 
> Well, my suggestion was to derive from just the file name (no path
> components) for them. But I pointed out that this may lead to collisions
> when two or more private headers of the same name exist, and a CU ends
> up wanting to include any two of them. Adding in the leaf-most path
> component only might get us far enough to avoid collisions in practice,
> while at the same time not resulting in overly long guard names.

If I understood correctly I am fine with that. To make sure we are all
on the same page, can you provide a couple of samples?

On 24.10.2023 21:59, Stefano Stabellini wrote:
> If I understood correctly I am fine with that. To make sure we are all
> on the same page, can you provide a couple of samples?

Taking the earlier example, instead of DRIVERS_PASSTHROUGH_VTD_DMAR_H it
would then be VTD_DMAR_H. arch/x86/pv/mm.h would use PV_MM_H, but then
you can already see that a hypothetical arch/x86/mm.h would use X86_MM_H,
thus colliding with what your proposal would also yield for
arch/x86/include/asm/mm.h. So maybe private header guards should come
with e.g. a trailing underscore? Or double underscores as component
separators, where .../include/... use only single underscores? Or
headers in arch/*/include/asm/ use ASM_<name>_H (i.e. not making the
architecture explicit in the guard name, on the grounds that headers
from multiple architectures shouldn't be included at the same time)?

Jan

Hi,

On 25/10/2023 09:18, Jan Beulich wrote:
> On 24.10.2023 21:59, Stefano Stabellini wrote:
>> If I understood correctly I am fine with that. To make sure we are all
>> on the same page, can you provide a couple of samples?
> 
> Taking the earlier example, instead of DRIVERS_PASSTHROUGH_VTD_DMAR_H it
> would then be VTD_DMAR_H. arch/x86/pv/mm.h would use PV_MM_H, but then
> you can already see that a hypothetical arch/x86/mm.h would use X86_MM_H,
> thus colliding with what your proposal would also yield for
> arch/x86/include/asm/mm.h. So maybe private header guards should come
> with e.g. a trailing underscore? Or double underscores as component
> separators, where .../include/... use only single underscores? Or
> headers in arch/*/include/asm/ use ASM_<name>_H (i.e. not making the
> architecture explicit in the guard name, on the grounds that headers
> from multiple architectures shouldn't be included at the same time)?
Thanks for providing some details.  The proposal for private headers 
make sense. For arch/.../include/asm/ headers, I would strongly prefer 
if we use prefix them with ASM_*.

As a side note, I am guessing for asm-generic, we would want to use 
ASM_GENERIC_* for the guard prefix. Is that correct?

Cheers,

-- 
Julien Grall

On 25.10.2023 17:58, Julien Grall wrote:
> On 25/10/2023 09:18, Jan Beulich wrote:
>> On 24.10.2023 21:59, Stefano Stabellini wrote:
>>> If I understood correctly I am fine with that. To make sure we are all
>>> on the same page, can you provide a couple of samples?
>>
>> Taking the earlier example, instead of DRIVERS_PASSTHROUGH_VTD_DMAR_H it
>> would then be VTD_DMAR_H. arch/x86/pv/mm.h would use PV_MM_H, but then
>> you can already see that a hypothetical arch/x86/mm.h would use X86_MM_H,
>> thus colliding with what your proposal would also yield for
>> arch/x86/include/asm/mm.h. So maybe private header guards should come
>> with e.g. a trailing underscore? Or double underscores as component
>> separators, where .../include/... use only single underscores? Or
>> headers in arch/*/include/asm/ use ASM_<name>_H (i.e. not making the
>> architecture explicit in the guard name, on the grounds that headers
>> from multiple architectures shouldn't be included at the same time)?
> Thanks for providing some details.  The proposal for private headers 
> make sense. For arch/.../include/asm/ headers, I would strongly prefer 
> if we use prefix them with ASM_*.
> 
> As a side note, I am guessing for asm-generic, we would want to use 
> ASM_GENERIC_* for the guard prefix. Is that correct?

That was an assumption I was working from, yes. Could also be just
GENERIC_ afaic.

Jan

On 25/10/2023 17:01, Jan Beulich wrote:
> On 25.10.2023 17:58, Julien Grall wrote:
>> On 25/10/2023 09:18, Jan Beulich wrote:
>>> On 24.10.2023 21:59, Stefano Stabellini wrote:
>>>> If I understood correctly I am fine with that. To make sure we are all
>>>> on the same page, can you provide a couple of samples?
>>>
>>> Taking the earlier example, instead of DRIVERS_PASSTHROUGH_VTD_DMAR_H it
>>> would then be VTD_DMAR_H. arch/x86/pv/mm.h would use PV_MM_H, but then
>>> you can already see that a hypothetical arch/x86/mm.h would use X86_MM_H,
>>> thus colliding with what your proposal would also yield for
>>> arch/x86/include/asm/mm.h. So maybe private header guards should come
>>> with e.g. a trailing underscore? Or double underscores as component
>>> separators, where .../include/... use only single underscores? Or
>>> headers in arch/*/include/asm/ use ASM_<name>_H (i.e. not making the
>>> architecture explicit in the guard name, on the grounds that headers
>>> from multiple architectures shouldn't be included at the same time)?
>> Thanks for providing some details.  The proposal for private headers
>> make sense. For arch/.../include/asm/ headers, I would strongly prefer
>> if we use prefix them with ASM_*.
>>
>> As a side note, I am guessing for asm-generic, we would want to use
>> ASM_GENERIC_* for the guard prefix. Is that correct?
> 
> That was an assumption I was working from, yes. Could also be just
> GENERIC_ afaic.

Thanks for the confirmation. I am fine with either GENERIC_ or ASM_GENERIC_.

Cheers,

-- 
Julien Grall

On Wed, 25 Oct 2023, Julien Grall wrote:
> On 25/10/2023 17:01, Jan Beulich wrote:
> > On 25.10.2023 17:58, Julien Grall wrote:
> > > On 25/10/2023 09:18, Jan Beulich wrote:
> > > > On 24.10.2023 21:59, Stefano Stabellini wrote:
> > > > > If I understood correctly I am fine with that. To make sure we are all
> > > > > on the same page, can you provide a couple of samples?
> > > > 
> > > > Taking the earlier example, instead of DRIVERS_PASSTHROUGH_VTD_DMAR_H it
> > > > would then be VTD_DMAR_H. arch/x86/pv/mm.h would use PV_MM_H, but then
> > > > you can already see that a hypothetical arch/x86/mm.h would use
> > > > X86_MM_H,
> > > > thus colliding with what your proposal would also yield for
> > > > arch/x86/include/asm/mm.h. So maybe private header guards should come
> > > > with e.g. a trailing underscore? Or double underscores as component
> > > > separators, where .../include/... use only single underscores? Or
> > > > headers in arch/*/include/asm/ use ASM_<name>_H (i.e. not making the
> > > > architecture explicit in the guard name, on the grounds that headers
> > > > from multiple architectures shouldn't be included at the same time)?
> > > Thanks for providing some details.  The proposal for private headers
> > > make sense. For arch/.../include/asm/ headers, I would strongly prefer
> > > if we use prefix them with ASM_*.
> > > 
> > > As a side note, I am guessing for asm-generic, we would want to use
> > > ASM_GENERIC_* for the guard prefix. Is that correct?
> > 
> > That was an assumption I was working from, yes. Could also be just
> > GENERIC_ afaic.
> 
> Thanks for the confirmation. I am fine with either GENERIC_ or ASM_GENERIC_.

OK. So in summary:
- arch/.../include/asm/ headers would use ASM_<filename>_H
- private headers would use <dir>_<filename>_H
- asm-generic headers would use ASM_GENERIC_<filename>_H

If that's agreed, we can move forward with the patch following this
scheme.

On 25.10.2023 23:12, Stefano Stabellini wrote:
> On Wed, 25 Oct 2023, Julien Grall wrote:
>> On 25/10/2023 17:01, Jan Beulich wrote:
>>> On 25.10.2023 17:58, Julien Grall wrote:
>>>> On 25/10/2023 09:18, Jan Beulich wrote:
>>>>> On 24.10.2023 21:59, Stefano Stabellini wrote:
>>>>>> If I understood correctly I am fine with that. To make sure we are all
>>>>>> on the same page, can you provide a couple of samples?
>>>>>
>>>>> Taking the earlier example, instead of DRIVERS_PASSTHROUGH_VTD_DMAR_H it
>>>>> would then be VTD_DMAR_H. arch/x86/pv/mm.h would use PV_MM_H, but then
>>>>> you can already see that a hypothetical arch/x86/mm.h would use
>>>>> X86_MM_H,
>>>>> thus colliding with what your proposal would also yield for
>>>>> arch/x86/include/asm/mm.h. So maybe private header guards should come
>>>>> with e.g. a trailing underscore? Or double underscores as component
>>>>> separators, where .../include/... use only single underscores? Or
>>>>> headers in arch/*/include/asm/ use ASM_<name>_H (i.e. not making the
>>>>> architecture explicit in the guard name, on the grounds that headers
>>>>> from multiple architectures shouldn't be included at the same time)?
>>>> Thanks for providing some details.  The proposal for private headers
>>>> make sense. For arch/.../include/asm/ headers, I would strongly prefer
>>>> if we use prefix them with ASM_*.
>>>>
>>>> As a side note, I am guessing for asm-generic, we would want to use
>>>> ASM_GENERIC_* for the guard prefix. Is that correct?
>>>
>>> That was an assumption I was working from, yes. Could also be just
>>> GENERIC_ afaic.
>>
>> Thanks for the confirmation. I am fine with either GENERIC_ or ASM_GENERIC_.
> 
> OK. So in summary:
> - arch/.../include/asm/ headers would use ASM_<filename>_H
> - private headers would use <dir>_<filename>_H
> - asm-generic headers would use ASM_GENERIC_<filename>_H
> 
> If that's agreed, we can move forward with the patch following this
> scheme.

FTAOD - just as long as <dir> is clarified to mean only the leaf-most
directory component (assuming we're still talking about the most
recently proposed scheme and we deem the risk of collisions low enough
there).

Jan

On Thu, 26 Oct 2023, Jan Beulich wrote:
> On 25.10.2023 23:12, Stefano Stabellini wrote:
> > On Wed, 25 Oct 2023, Julien Grall wrote:
> >> On 25/10/2023 17:01, Jan Beulich wrote:
> >>> On 25.10.2023 17:58, Julien Grall wrote:
> >>>> On 25/10/2023 09:18, Jan Beulich wrote:
> >>>>> On 24.10.2023 21:59, Stefano Stabellini wrote:
> >>>>>> If I understood correctly I am fine with that. To make sure we are all
> >>>>>> on the same page, can you provide a couple of samples?
> >>>>>
> >>>>> Taking the earlier example, instead of DRIVERS_PASSTHROUGH_VTD_DMAR_H it
> >>>>> would then be VTD_DMAR_H. arch/x86/pv/mm.h would use PV_MM_H, but then
> >>>>> you can already see that a hypothetical arch/x86/mm.h would use
> >>>>> X86_MM_H,
> >>>>> thus colliding with what your proposal would also yield for
> >>>>> arch/x86/include/asm/mm.h. So maybe private header guards should come
> >>>>> with e.g. a trailing underscore? Or double underscores as component
> >>>>> separators, where .../include/... use only single underscores? Or
> >>>>> headers in arch/*/include/asm/ use ASM_<name>_H (i.e. not making the
> >>>>> architecture explicit in the guard name, on the grounds that headers
> >>>>> from multiple architectures shouldn't be included at the same time)?
> >>>> Thanks for providing some details.  The proposal for private headers
> >>>> make sense. For arch/.../include/asm/ headers, I would strongly prefer
> >>>> if we use prefix them with ASM_*.
> >>>>
> >>>> As a side note, I am guessing for asm-generic, we would want to use
> >>>> ASM_GENERIC_* for the guard prefix. Is that correct?
> >>>
> >>> That was an assumption I was working from, yes. Could also be just
> >>> GENERIC_ afaic.
> >>
> >> Thanks for the confirmation. I am fine with either GENERIC_ or ASM_GENERIC_.
> > 
> > OK. So in summary:
> > - arch/.../include/asm/ headers would use ASM_<filename>_H
> > - private headers would use <dir>_<filename>_H
> > - asm-generic headers would use ASM_GENERIC_<filename>_H
> > 
> > If that's agreed, we can move forward with the patch following this
> > scheme.
> 
> FTAOD - just as long as <dir> is clarified to mean only the leaf-most
> directory component (assuming we're still talking about the most
> recently proposed scheme and we deem the risk of collisions low enough
> there).

Yes, that's what I meant.

On 27/10/23 00:50, Stefano Stabellini wrote:
> On Thu, 26 Oct 2023, Jan Beulich wrote:
>> On 25.10.2023 23:12, Stefano Stabellini wrote:
>>> On Wed, 25 Oct 2023, Julien Grall wrote:
>>>> On 25/10/2023 17:01, Jan Beulich wrote:
>>>>> On 25.10.2023 17:58, Julien Grall wrote:
>>>>>> On 25/10/2023 09:18, Jan Beulich wrote:
>>>>>>> On 24.10.2023 21:59, Stefano Stabellini wrote:
>>>>>>>> If I understood correctly I am fine with that. To make sure we are all
>>>>>>>> on the same page, can you provide a couple of samples?
>>>>>>>
>>>>>>> Taking the earlier example, instead of DRIVERS_PASSTHROUGH_VTD_DMAR_H it
>>>>>>> would then be VTD_DMAR_H. arch/x86/pv/mm.h would use PV_MM_H, but then
>>>>>>> you can already see that a hypothetical arch/x86/mm.h would use
>>>>>>> X86_MM_H,
>>>>>>> thus colliding with what your proposal would also yield for
>>>>>>> arch/x86/include/asm/mm.h. So maybe private header guards should come
>>>>>>> with e.g. a trailing underscore? Or double underscores as component
>>>>>>> separators, where .../include/... use only single underscores? Or
>>>>>>> headers in arch/*/include/asm/ use ASM_<name>_H (i.e. not making the
>>>>>>> architecture explicit in the guard name, on the grounds that headers
>>>>>>> from multiple architectures shouldn't be included at the same time)?
>>>>>> Thanks for providing some details.  The proposal for private headers
>>>>>> make sense. For arch/.../include/asm/ headers, I would strongly prefer
>>>>>> if we use prefix them with ASM_*.
>>>>>>
>>>>>> As a side note, I am guessing for asm-generic, we would want to use
>>>>>> ASM_GENERIC_* for the guard prefix. Is that correct?
>>>>>
>>>>> That was an assumption I was working from, yes. Could also be just
>>>>> GENERIC_ afaic.
>>>>
>>>> Thanks for the confirmation. I am fine with either GENERIC_ or ASM_GENERIC_.
>>>
>>> OK. So in summary:
>>> - arch/.../include/asm/ headers would use ASM_<filename>_H
>>> - private headers would use <dir>_<filename>_H
>>> - asm-generic headers would use ASM_GENERIC_<filename>_H
>>>
>>> If that's agreed, we can move forward with the patch following this
>>> scheme.
>>
>> FTAOD - just as long as <dir> is clarified to mean only the leaf-most
>> directory component (assuming we're still talking about the most
>> recently proposed scheme and we deem the risk of collisions low enough
>> there).
> 
> Yes, that's what I meant.

Ok, I'll work on the patch using this schema.

-- 
Simone Ballarin, M.Sc.

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

On 29/09/23 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.
> 

Yes, it is.

Rule 21.2, found in Set3, addresses this Undef.-b.:
A reserved identifier or reserved macro name shall not be declared.

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

-- 
Simone Ballarin, M.Sc.

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

On Fri, 29 Sep 2023, Simone Ballarin wrote:
> On 29/09/23 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.
> > 
> 
> Yes, it is.
> 
> Rule 21.2, found in Set3, addresses this Undef.-b.:
> A reserved identifier or reserved macro name shall not be declared.

OK. Adding Roberto in CC. I think we should discuss 21.2 during the next
MISRA C meeting. If 21.2 is accepted then we should go down the route of
a global rename here which would also benefit consistency across the
codebase.


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

On 28/09/23 17:00, 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.
> 
> Jan

I suggest starting using this new naming schema just on the files 
touched by this series: I do not think is a good idea changing all
guards now.

This should be done in any case if you will decide to be compliant
with Rule 21.2 (Set3):
(required) A reserved identifier or reserved macro name shall not be 
declared.



-- 
Simone Ballarin, M.Sc.

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