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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?