| Message ID | cover.1694510856.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | address violations of MISRA C:2012 Directive 4.10 | expand |
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?
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
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 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.
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/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__ ?
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 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,
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,
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,
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.
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(-)