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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.