[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


  • To: Stefano Stabellini <stefano.stabellini@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 23 Oct 2023 08:31:47 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=0/ITUx92bYGTk2BROQ8KItH10vIzvSEokoXlX1fSj9A=; b=d1HWVE4UlqR8MTyJ3B4BJdsR5BoT+SM8GAcYmE4x6fqz46zoUIPWCb5NoUdfZLa9/0BE4I/dxS68SPGd2xdEdzmtl6k8ubbnp/I5npa4cMyiwieUksQVGvFUA5UmoYBXlpE7HsPeaAM7SHdKP8QZunon1lLSi81RbJyGRqAZ2Bz4kgniCbFjO34j/DAlMfitGbJahL2vx5tVEIs3kv6L+NmLkswoO4M9JgaDkCrcdQq70ojkDVidYltpqfPC84SXU3B77BzL2HLGuFFiYOj6jXUMaZhd47uiQQc6nGeylGT22e5WOmYYtCeCPOWw8HAGsStWPEsHUxhkvwALGJehUQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Os15VfVs2iA1i7q9Xwh2d+rWv1r01/lpw54cnxJtT/rM4/DG0kqTbTQvA7VNpu2Q6qCqj0b+CrRkMy2nBfwBVC9ClY0K+2WvXU4CTazZ1IalW3hrkf0GeLK/SREmNgMfwPvydklQHm46U4dFy988VRV1s2B87FZY4ohmMMJPofKGTev38O7nTVbkJkyiPqdZsa65DN2nyYvQPkJrfjkXyFR0UM1tCFNc7BnX40k6hyv3E/SHib28TJLlQeCc345prnoeKBW/zOTS3PUohaG7OxGItd+ltiq+6U70QuXfkR16QFd1ydXeGvB8QvjSKgmMZkOwLwXA+e/4xqsij9HvcA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Simone Ballarin <simone.ballarin@xxxxxxxxxxx>, consulting@xxxxxxxxxxx, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Doug Goldstein <cardoe@xxxxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 23 Oct 2023 06:32:23 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.