|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v2 00/10] address violations of MISRA C:2012 Directive 4.10
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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |