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

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

[Simone Ballarin]

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(-)

-- 
2.34.1

[XEN PATCH v2 01/10] misra: add deviation for headers that explicitly avoid guards

[Simone Ballarin]

Some headers, under specific circumstances (documented in a comment at
the beginning of the file), explicitly avoid inclusion guards: the caller
is responsible for including them correctly.

These files are not supposed to comply with Directive 4.10:
"Precautions shall be taken in order to prevent the contents of a header
file being included more than once"

This patch adds deviation cooments for headers that avoid guards.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- use the format introduced with doc/misra/safe.json instead of
  a generic text-based deviation
---
 docs/misra/safe.json                        | 8 ++++++++
 xen/include/public/arch-x86/cpufeatureset.h | 1 +
 xen/include/public/errno.h                  | 1 +
 3 files changed, 10 insertions(+)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 39c5c056c7..db438c9770 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -20,6 +20,14 @@
         },
         {
             "id": "SAF-2-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: headers that leave it up to the caller to include them correctly",
+            "text": "Headers that deliberatively avoid inclusion guards explicitly leaving responsibility to the caller are allowed."
+        },
+        {
+            "id": "SAF-3-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 6b6ce2745c..eac1ae4b2a 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -23,6 +23,7 @@
  * their XEN_CPUFEATURE() being appropriate in the included context.
  */
 
+/* SAF-1-safe header that leaves it up to the caller to include them correctly */
 #ifndef XEN_CPUFEATURE
 
 /*
diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
index 5a78a7607c..8b60ac74ae 100644
--- a/xen/include/public/errno.h
+++ b/xen/include/public/errno.h
@@ -17,6 +17,7 @@
  * will unilaterally #undef XEN_ERRNO().
  */
 
+/* SAF-1-safe header that leaves it up to the caller to include them correctly */
 #ifndef XEN_ERRNO
 
 /*
-- 
2.34.1

[XEN PATCH v2 02/10] misra: modify deviations for empty and generated headers

[Simone Ballarin]

This patch modifies deviations for Directive 4.10:
"Precautions shall be taken in order to prevent the contents of
a header file being included more than once"

This patch avoids the file-based deviation for empty headers, and
replaces it with a comment-based one using the format specified in
docs/misra/safe.json.

Generated headers are not generally safe against multi-inclusions,
whether a header is safe depends on the nature of the generated code
in the header. For that reason, this patch drops the deviation for
generated headers.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- use the format introduced with doc/misra/safe.json instead of
  a file-based deviation for empty headers
- remove deviation for generated headers
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 7 -------
 docs/misra/safe.json                             | 8 ++++++++
 xen/arch/arm/efi/runtime.h                       | 1 +
 xen/include/Makefile                             | 2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index d8170106b4..9313027af1 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -78,13 +78,6 @@ inline functions."
 -config=MC3R1.D4.9,macros+={deliberate, "loc(file(api:public))"}
 -doc_end
 
--doc_begin="This header file is autogenerated or empty, therefore it poses no
-risk if included more than once."
--file_tag+={empty_header, "^xen/arch/arm/efi/runtime\\.h$"}
--file_tag+={autogen_headers, "^xen/include/xen/compile\\.h$||^xen/include/generated/autoconf.h$||^xen/include/xen/hypercall-defs.h$"}
--config=MC3R1.D4.10,reports+={safe, "all_area(all_loc(file(empty_header||autogen_headers)))"}
--doc_end
-
 -doc_begin="Files that are intended to be included more than once do not need to
 conform to the directive."
 -config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* This file is legitimately included multiple times\\. \\*/$, begin-4))"}
diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index db438c9770..e8e200cb0a 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -28,6 +28,14 @@
         },
         {
             "id": "SAF-3-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: empty headers",
+            "text": "Empty headers pose no risk if included more than once."
+        },
+        {
+            "id": "SAF-4-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/arch/arm/efi/runtime.h b/xen/arch/arm/efi/runtime.h
index 25afcebed1..5e35184ff4 100644
--- a/xen/arch/arm/efi/runtime.h
+++ b/xen/arch/arm/efi/runtime.h
@@ -1 +1,2 @@
+/* SAF-2-safe empty header */
 /* Placeholder for ARM-specific runtime include/declarations */
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 2e61b50139..31782fb177 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -53,7 +53,7 @@ cmd_compat_h = \
     mv -f $@.new $@
 
 quiet_cmd_stub_h = GEN     $@
-cmd_stub_h = echo '/* empty */' >$@
+cmd_stub_h = echo '/* SAF-2-safe empty header */' >$@
 
 quiet_cmd_compat_i = CPP     $@
 cmd_compat_i = $(CPP) $(filter-out -Wa$(comma)% -include %/include/xen/config.h,$(XEN_CFLAGS)) $(cppflags-y) -o $@ $<
-- 
2.34.1

[XEN PATCH v2 03/10] misra: add deviations for direct inclusion guards

[Simone Ballarin]

Add deviation comments 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).

This patch adds deviation comments using the format specified
in docs/misra/safe.json for headers with just the direct
inclusion guard before the inclusion guard since they are
safe and not supposed to comply with the directive.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
The patch has been introduced in v2.
---
 docs/misra/safe.json                 | 8 ++++++++
 xen/arch/arm/include/asm/hypercall.h | 1 +
 xen/arch/x86/include/asm/hypercall.h | 1 +
 xen/include/xen/unaligned.h          | 1 +
 4 files changed, 11 insertions(+)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index e8e200cb0a..0ec594f6bf 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -36,6 +36,14 @@
         },
         {
             "id": "SAF-4-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: direct inclusion guard before",
+            "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
+        },
+        {
+            "id": "SAF-5-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
index ccd26c5184..24f8c61a73 100644
--- a/xen/arch/arm/include/asm/hypercall.h
+++ b/xen/arch/arm/include/asm/hypercall.h
@@ -1,3 +1,4 @@
+/* SAF-3-safe direct inclusion guard before */
 #ifndef __XEN_HYPERCALL_H__
 #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
 #endif
diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
index ec2edc771e..dfdfe80021 100644
--- a/xen/arch/x86/include/asm/hypercall.h
+++ b/xen/arch/x86/include/asm/hypercall.h
@@ -2,6 +2,7 @@
  * asm-x86/hypercall.h
  */
 
+/* SAF-3-safe direct inclusion guard before */
 #ifndef __XEN_HYPERCALL_H__
 #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
 #endif
diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
index 0a2b16d05d..190ada7800 100644
--- a/xen/include/xen/unaligned.h
+++ b/xen/include/xen/unaligned.h
@@ -3,6 +3,7 @@
  * without faulting, and at least reasonably efficiently.  Other architectures
  * will need to have a custom asm/unaligned.h.
  */
+/* SAF-3-safe direct inclusion guard before */
 #ifndef __ASM_UNALIGNED_H__
 #error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
 #endif
-- 
2.34.1

[XEN PATCH v2 04/10] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add inclusion guard 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").

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- drop changes in xen/arch/arm/include/asm/hypercall.h
- drop changes in xen/arch/arm/include/asm/iocap.h since they are
  not related with the directive
---
 xen/arch/arm/efi/efi-boot.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 1c3640bb65..aaa468f186 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -3,6 +3,10 @@
  * is intended to be included by common/efi/boot.c _only_, and
  * therefore can define arch specific global variables.
  */
+
+#ifndef __ARM_EFI_EFI_BOOT_H__
+#define __ARM_EFI_EFI_BOOT_H__
+
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
 #include <asm/setup.h>
@@ -1003,6 +1007,8 @@ static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size)
     __flush_dcache_area(vaddr, size);
 }
 
+#endif /* __ARM_EFI_EFI_BOOT_H__ */
+
 /*
  * Local variables:
  * mode: C
-- 
2.34.1

[XEN PATCH v2 05/10] xen/x86: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

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).

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- remove extra blanks
- drop changes in C files

Note:
Changes in Makefile were not strictly necessary in v1 and a maintainer
asked to removing them since there was a deviation for generated headers.
Now, in v2, they are required since the deviation has been removed by
another patch of this series.
---
 xen/arch/x86/Makefile              | 8 ++++----
 xen/arch/x86/cpu/cpu.h             | 5 +++++
 xen/arch/x86/x86_64/mmconfig.h     | 5 +++++
 xen/arch/x86/x86_emulate/private.h | 5 +++++
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index f3abdf9cd1..40d0cb76af 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -260,17 +260,17 @@ $(objtree)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i $(src)/Makefil
 	$(call filechk,asm-macros.h)
 
 define filechk_asm-macros.h
+    echo '#ifndef __ASM_MACROS_H__'; \
+    echo '#define __ASM_MACROS_H__'; \
     echo '#if 0'; \
     echo '.if 0'; \
     echo '#endif'; \
-    echo '#ifndef __ASM_MACROS_H__'; \
-    echo '#define __ASM_MACROS_H__'; \
     echo 'asm ( ".include \"$@\"" );'; \
-    echo '#endif /* __ASM_MACROS_H__ */'; \
     echo '#if 0'; \
     echo '.endif'; \
     cat $<; \
-    echo '#endif'
+    echo '#endif'; \
+    echo '#endif /* __ASM_MACROS_H__ */'
 endef
 
 $(obj)/efi.lds: AFLAGS-y += -DEFI
diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h
index e3d06278b3..95939c7fb6 100644
--- a/xen/arch/x86/cpu/cpu.h
+++ b/xen/arch/x86/cpu/cpu.h
@@ -1,3 +1,6 @@
+#ifndef __X86_CPU_CPU_H__
+#define __X86_CPU_CPU_H__
+
 /* attempt to consolidate cpu attributes */
 struct cpu_dev {
 	void		(*c_early_init)(struct cpuinfo_x86 *c);
@@ -24,3 +27,5 @@ void amd_init_lfence(struct cpuinfo_x86 *c);
 void amd_init_ssbd(const struct cpuinfo_x86 *c);
 void amd_init_spectral_chicken(void);
 void detect_zen2_null_seg_behaviour(void);
+
+#endif /* __X86_CPU_CPU_H__ */
diff --git a/xen/arch/x86/x86_64/mmconfig.h b/xen/arch/x86/x86_64/mmconfig.h
index 2d49fc79a0..c562879c76 100644
--- a/xen/arch/x86/x86_64/mmconfig.h
+++ b/xen/arch/x86/x86_64/mmconfig.h
@@ -5,6 +5,9 @@
  * Author: Allen Kay <allen.m.kay@intel.com> - adapted from linux
  */
 
+#ifndef __X86_X86_64_MMCONFIG_H__
+#define __X86_X86_64_MMCONFIG_H__
+
 #define PCI_DEVICE_ID_INTEL_E7520_MCH    0x3590
 #define PCI_DEVICE_ID_INTEL_82945G_HB    0x2770
 
@@ -72,3 +75,5 @@ int pci_mmcfg_reserved(uint64_t address, unsigned int segment,
 int pci_mmcfg_arch_init(void);
 int pci_mmcfg_arch_enable(unsigned int);
 void pci_mmcfg_arch_disable(unsigned int);
+
+#endif /* __X86_X86_64_MMCONFIG_H__ */
diff --git a/xen/arch/x86/x86_emulate/private.h b/xen/arch/x86/x86_emulate/private.h
index 719dad59cd..ffa134f297 100644
--- a/xen/arch/x86/x86_emulate/private.h
+++ b/xen/arch/x86/x86_emulate/private.h
@@ -6,6 +6,9 @@
  * Copyright (c) 2005-2007 XenSource Inc.
  */
 
+#ifndef __X86_X86_EMULATE_PRIVATE_H__
+#define __X86_X86_EMULATE_PRIVATE_H__
+
 #ifdef __XEN__
 
 # include <xen/kernel.h>
@@ -831,3 +834,5 @@ static inline int read_ulong(enum x86_segment seg,
     *val = 0;
     return ops->read(seg, offset, val, bytes, ctxt);
 }
+
+#endif /* __X86_X86_EMULATE_PRIVATE_H__ */
-- 
2.34.1

[XEN PATCH v2 06/10] x86/EFI: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add inclusion guard 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").

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- remove changes in "xen/arch/x86/efi/efi-boot.h"

Note:
Changes in efi-boot.h have been removed since the file is
intenteded to be included by common/efi/boot.c only. This motivation
is not enough to raise a deviation record, so the violation is
still present.
---
 xen/arch/x86/efi/runtime.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/efi/runtime.h b/xen/arch/x86/efi/runtime.h
index 77866c5f21..10b36bcb89 100644
--- a/xen/arch/x86/efi/runtime.h
+++ b/xen/arch/x86/efi/runtime.h
@@ -1,3 +1,6 @@
+#ifndef __X86_EFI_RUNTIME_H__
+#define __X86_EFI_RUNTIME_H__
+
 #include <xen/domain_page.h>
 #include <xen/mm.h>
 #include <asm/atomic.h>
@@ -17,3 +20,5 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e)
     }
 }
 #endif
+
+#endif /* __X86_EFI_RUNTIME_H__ */
-- 
2.34.1

[XEN PATCH v2 07/10] xen/common: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add 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").

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- drop changes in C files
---
 xen/common/decompress.h    | 5 +++++
 xen/common/event_channel.h | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/xen/common/decompress.h b/xen/common/decompress.h
index e8195b353a..da3c3abb6a 100644
--- a/xen/common/decompress.h
+++ b/xen/common/decompress.h
@@ -1,3 +1,6 @@
+#ifndef __COMMON_DECOMPRESS_H__
+#define __COMMON_DECOMPRESS_H__
+
 #ifdef __XEN__
 
 #include <xen/cache.h>
@@ -23,3 +26,5 @@
 #define large_free free
 
 #endif
+
+#endif /* __COMMON_DECOMPRESS_H__ */
diff --git a/xen/common/event_channel.h b/xen/common/event_channel.h
index 45219ca67c..040bad77f9 100644
--- a/xen/common/event_channel.h
+++ b/xen/common/event_channel.h
@@ -1,5 +1,8 @@
 /* Event channel handling private header. */
 
+#ifndef __COMMON_EVENT_CHANNEL_H__
+#define __COMMON_EVENT_CHANNEL_H__
+
 #include <xen/event.h>
 
 static inline unsigned int max_evtchns(const struct domain *d)
@@ -52,6 +55,8 @@ int evtchn_fifo_init_control(struct evtchn_init_control *init_control);
 int evtchn_fifo_expand_array(const struct evtchn_expand_array *expand_array);
 void evtchn_fifo_destroy(struct domain *d);
 
+#endif /* __COMMON_EVENT_CHANNEL_H__ */
+
 /*
  * Local variables:
  * mode: C
-- 
2.34.1

[XEN PATCH v2 08/10] xen/efi: address violations of MISRA C:2012 Directive 4.10

[Simone Ballarin]

Add 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").

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- drop changes in C files
---
 xen/common/efi/efi.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index c02fbb7b69..cef9381d30 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -1,3 +1,6 @@
+#ifndef __COMMON_EFI_EFI_H__
+#define __COMMON_EFI_EFI_H__
+
 #include <asm/efibind.h>
 #include <efi/efidef.h>
 #include <efi/efierr.h>
@@ -51,3 +54,5 @@ void free_ebmalloc_unused_mem(void);
 
 const void *pe_find_section(const void *image, const UINTN image_size,
                             const CHAR16 *section_name, UINTN *size_out);
+
+#endif /* __COMMON_EFI_EFI_H__ */
-- 
2.34.1

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

[Simone Ballarin]

Amended 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.

Mechanical change.

---
Changes in v2:
- drop changes in xen/include/xen/unaligned.h since this second
  series adds a comment-based deviation in a separate patch
- use #ifndef instead of #if !defined()
---
 xen/include/xen/err.h     | 4 +++-
 xen/include/xen/pci_ids.h | 5 +++++
 xen/include/xen/softirq.h | 4 +++-
 xen/include/xen/vmap.h    | 4 +++-
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/err.h b/xen/include/xen/err.h
index 2f29b57d28..646c20ab4a 100644
--- a/xen/include/xen/err.h
+++ b/xen/include/xen/err.h
@@ -1,5 +1,6 @@
-#if !defined(__XEN_ERR_H__) && !defined(__ASSEMBLY__)
+#ifndef __XEN_ERR_H__
 #define __XEN_ERR_H__
+#ifndef __ASSEMBLY__
 
 #include <xen/compiler.h>
 #include <xen/errno.h>
@@ -54,4 +55,5 @@ static inline int __must_check PTR_RET(const void *ptr)
 	return IS_ERR(ptr) ? PTR_ERR(ptr) : 0;
 }
 
+#endif /* __ASSEMBLY__ */
 #endif /* __XEN_ERR_H__ */
diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
index e798477a7e..1a739d4c92 100644
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -1,3 +1,6 @@
+#ifndef __XEN_PCI_IDS_H__
+#define __XEN_PCI_IDS_H__
+
 #define PCI_VENDOR_ID_AMD                0x1022
 
 #define PCI_VENDOR_ID_NVIDIA             0x10de
@@ -11,3 +14,5 @@
 #define PCI_VENDOR_ID_BROADCOM           0x14e4
 
 #define PCI_VENDOR_ID_INTEL              0x8086
+
+#endif /* __XEN_PCI_IDS_H__ */
diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h
index 33d6f2ecd2..4c7655188a 100644
--- a/xen/include/xen/softirq.h
+++ b/xen/include/xen/softirq.h
@@ -1,5 +1,6 @@
-#if !defined(__XEN_SOFTIRQ_H__) && !defined(__ASSEMBLY__)
+#ifndef __XEN_SOFTIRQ_H__
 #define __XEN_SOFTIRQ_H__
+#ifndef __ASSEMBLY__
 
 /* Low-latency softirqs come first in the following list. */
 enum {
@@ -40,4 +41,5 @@ void cpu_raise_softirq_batch_finish(void);
  */
 void process_pending_softirqs(void);
 
+#endif /* __ASSEMBLY__ */
 #endif /* __XEN_SOFTIRQ_H__ */
diff --git a/xen/include/xen/vmap.h b/xen/include/xen/vmap.h
index b0f7632e89..03868b0b1a 100644
--- a/xen/include/xen/vmap.h
+++ b/xen/include/xen/vmap.h
@@ -1,5 +1,6 @@
-#if !defined(__XEN_VMAP_H__) && defined(VMAP_VIRT_START)
+#ifndef __XEN_VMAP_H__
 #define __XEN_VMAP_H__
+#ifdef VMAP_VIRT_START
 
 #include <xen/mm-frame.h>
 #include <xen/page-size.h>
@@ -38,4 +39,5 @@ static inline void vm_init(void)
     vm_init_type(VMAP_DEFAULT, (void *)VMAP_VIRT_START, arch_vmap_virt_end());
 }
 
+#endif /* VMAP_VIRT_START */
 #endif /* __XEN_VMAP_H__ */
-- 
2.34.1

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

[Simone Ballarin]

Amend generation script, add 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").

This patch amends the Makefile adding the required inclusion guards
for xlat.h.

Add deviation comment for files intended for multiple inclusion.

Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

---
Changes in v2:
- merge patches 7/13 and 13/13 of v1 as they had the same
  commit message
- amend the Makefile to produce the required inclusion guard
- use the format introduced with doc/misra/safe.json instead of
  a generic text-based deviation
---
 docs/misra/safe.json                   | 8 ++++++++
 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/include/Makefile                   | 8 ++++++--
 5 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 0ec594f6bf..82c636ee94 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -44,6 +44,14 @@
         },
         {
             "id": "SAF-5-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: file intended for multiple inclusion",
+            "text": "Files intended for multiple inclusion are not supposed to comply with D4.10."
+        },
+        {
+            "id": "SAF-6-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/arch/x86/include/asm/compat.h b/xen/arch/x86/include/asm/compat.h
index 818cad87db..3d3891d061 100644
--- a/xen/arch/x86/include/asm/compat.h
+++ b/xen/arch/x86/include/asm/compat.h
@@ -2,6 +2,9 @@
  * compat.h
  */
 
+#ifndef __ASM_X86_COMPAT_H__
+#define __ASM_X86_COMPAT_H__
+
 #ifdef CONFIG_COMPAT
 
 #define COMPAT_BITS_PER_LONG 32
@@ -18,3 +21,5 @@ int switch_compat(struct domain *);
 #include <xen/errno.h>
 static inline int switch_compat(struct domain *d) { return -EOPNOTSUPP; }
 #endif
+
+#endif /* __ASM_X86_COMPAT_H__ */
diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
index da0593de85..39b15e463a 100644
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -1,7 +1,4 @@
-/*
- * Explicitly intended for multiple inclusion.
- */
-
+/* SAF-4-safe file intended for multiple inclusion */
 #include <xen/lib/x86/cpuid-autogen.h>
 
 /* Number of capability words covered by the featureset words. */
diff --git a/xen/arch/x86/include/asm/efibind.h b/xen/arch/x86/include/asm/efibind.h
index bce02f3707..f2eb8b5496 100644
--- a/xen/arch/x86/include/asm/efibind.h
+++ b/xen/arch/x86/include/asm/efibind.h
@@ -1,2 +1,7 @@
+#ifndef __ASM_X86_EFIBIND_H__
+#define __ASM_X86_EFIBIND_H__
+
 #include <xen/types.h>
 #include <asm/x86_64/efibind.h>
+
+#endif /* __ASM_X86_EFIBIND_H__ */
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 31782fb177..b2f9576362 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -105,9 +105,13 @@ xlat-y := $(shell sed -ne 's,@arch@,$(compat-arch-y),g' -re 's,^[?!][[:blank:]]+
 xlat-y := $(filter $(patsubst compat/%,%,$(headers-y)),$(xlat-y))
 
 quiet_cmd_xlat_h = GEN     $@
-cmd_xlat_h = \
-	cat $(filter %.h,$^) >$@.new; \
+define cmd_xlat_h
+	echo "#ifndef _COMPAT_XLAT_H" > $@.new; \
+	echo "#define _COMPAT_XLAT_H" >> $@.new; \
+	cat $(filter %.h,$^) >> $@.new; \
+	echo "#endif /* _COMPAT_XLAT_H */" >> $@.new; \
 	mv -f $@.new $@
+endef
 
 $(obj)/compat/xlat.h: $(addprefix $(obj)/compat/.xlat/,$(xlat-y)) FORCE
 	$(call if_changed,xlat_h)
-- 
2.34.1

Re: [XEN PATCH v2 01/10] misra: add deviation for headers that explicitly avoid guards

[Jan Beulich]

On 12.09.2023 11:36, Simone Ballarin wrote:
> Some headers, under specific circumstances (documented in a comment at
> the beginning of the file), explicitly avoid inclusion guards: the caller
> is responsible for including them correctly.
> 
> These files are not supposed to comply with Directive 4.10:
> "Precautions shall be taken in order to prevent the contents of a header
> file being included more than once"
> 
> This patch adds deviation cooments for headers that avoid guards.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> ---
> Changes in v2:
> - use the format introduced with doc/misra/safe.json instead of
>   a generic text-based deviation
> ---
>  docs/misra/safe.json                        | 8 ++++++++
>  xen/include/public/arch-x86/cpufeatureset.h | 1 +
>  xen/include/public/errno.h                  | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index 39c5c056c7..db438c9770 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -20,6 +20,14 @@
>          },
>          {
>              "id": "SAF-2-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.D4.10"
> +            },
> +            "name": "Dir 4.10: headers that leave it up to the caller to include them correctly",
> +            "text": "Headers that deliberatively avoid inclusion guards explicitly leaving responsibility to the caller are allowed."
> +        },

With this ...

> +        {
> +            "id": "SAF-3-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
> index 6b6ce2745c..eac1ae4b2a 100644
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -23,6 +23,7 @@
>   * their XEN_CPUFEATURE() being appropriate in the included context.
>   */
>  
> +/* SAF-1-safe header that leaves it up to the caller to include them correctly */
>  #ifndef XEN_CPUFEATURE
>  
>  /*
> diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
> index 5a78a7607c..8b60ac74ae 100644
> --- a/xen/include/public/errno.h
> +++ b/xen/include/public/errno.h
> @@ -17,6 +17,7 @@
>   * will unilaterally #undef XEN_ERRNO().
>   */
>  
> +/* SAF-1-safe header that leaves it up to the caller to include them correctly */
>  #ifndef XEN_ERRNO
>  
>  /*

... you mean SAF-2-safe in both code comments. I did point out the problem
with the sequential numbering (and resulting rebasing mistakes) when the
scheme was introduced.

I also think the comments are too verbose. I don't mind them having an
indication what specific issue they are about, but it shouldn't be more
than a couple of words. Here maybe "omitted inclusion guard".

Jan

Re: [XEN PATCH v2 02/10] misra: modify deviations for empty and generated headers

[Jan Beulich]

On 12.09.2023 11:36, Simone Ballarin wrote:
> This patch modifies deviations for Directive 4.10:
> "Precautions shall be taken in order to prevent the contents of
> a header file being included more than once"
> 
> This patch avoids the file-based deviation for empty headers, and
> replaces it with a comment-based one using the format specified in
> docs/misra/safe.json.
> 
> Generated headers are not generally safe against multi-inclusions,
> whether a header is safe depends on the nature of the generated code
> in the header. For that reason, this patch drops the deviation for
> generated headers.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> ---
> Changes in v2:
> - use the format introduced with doc/misra/safe.json instead of
>   a file-based deviation for empty headers
> - remove deviation for generated headers
> ---
>  automation/eclair_analysis/ECLAIR/deviations.ecl | 7 -------
>  docs/misra/safe.json                             | 8 ++++++++
>  xen/arch/arm/efi/runtime.h                       | 1 +
>  xen/include/Makefile                             | 2 +-
>  4 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
> index d8170106b4..9313027af1 100644
> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl
> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
> @@ -78,13 +78,6 @@ inline functions."
>  -config=MC3R1.D4.9,macros+={deliberate, "loc(file(api:public))"}
>  -doc_end
>  
> --doc_begin="This header file is autogenerated or empty, therefore it poses no
> -risk if included more than once."
> --file_tag+={empty_header, "^xen/arch/arm/efi/runtime\\.h$"}
> --file_tag+={autogen_headers, "^xen/include/xen/compile\\.h$||^xen/include/generated/autoconf.h$||^xen/include/xen/hypercall-defs.h$"}
> --config=MC3R1.D4.10,reports+={safe, "all_area(all_loc(file(empty_header||autogen_headers)))"}
> --doc_end
> -
>  -doc_begin="Files that are intended to be included more than once do not need to
>  conform to the directive."
>  -config=MC3R1.D4.10,reports+={safe, "first_area(text(^/\\* This file is legitimately included multiple times\\. \\*/$, begin-4))"}
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index db438c9770..e8e200cb0a 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -28,6 +28,14 @@
>          },
>          {
>              "id": "SAF-3-safe",

Noting this, ...

> +            "analyser": {
> +                "eclair": "MC3R1.D4.10"
> +            },
> +            "name": "Dir 4.10: empty headers",
> +            "text": "Empty headers pose no risk if included more than once."
> +        },
> +        {
> +            "id": "SAF-4-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> --- a/xen/arch/arm/efi/runtime.h
> +++ b/xen/arch/arm/efi/runtime.h
> @@ -1 +1,2 @@
> +/* SAF-2-safe empty header */
>  /* Placeholder for ARM-specific runtime include/declarations */
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -53,7 +53,7 @@ cmd_compat_h = \
>      mv -f $@.new $@
>  
>  quiet_cmd_stub_h = GEN     $@
> -cmd_stub_h = echo '/* empty */' >$@
> +cmd_stub_h = echo '/* SAF-2-safe empty header */' >$@

... there's the same off-by-1 here as there was in patch 1.

Jan

Re: [XEN PATCH v2 01/10] misra: add deviation for headers that explicitly avoid guards

[Simone Ballarin]

On 12/09/23 11:46, Jan Beulich wrote:
> On 12.09.2023 11:36, Simone Ballarin wrote:
>> Some headers, under specific circumstances (documented in a comment at
>> the beginning of the file), explicitly avoid inclusion guards: the caller
>> is responsible for including them correctly.
>>
>> These files are not supposed to comply with Directive 4.10:
>> "Precautions shall be taken in order to prevent the contents of a header
>> file being included more than once"
>>
>> This patch adds deviation cooments for headers that avoid guards.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>>
>> ---
>> Changes in v2:
>> - use the format introduced with doc/misra/safe.json instead of
>>    a generic text-based deviation
>> ---
>>   docs/misra/safe.json                        | 8 ++++++++
>>   xen/include/public/arch-x86/cpufeatureset.h | 1 +
>>   xen/include/public/errno.h                  | 1 +
>>   3 files changed, 10 insertions(+)
>>
>> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
>> index 39c5c056c7..db438c9770 100644
>> --- a/docs/misra/safe.json
>> +++ b/docs/misra/safe.json
>> @@ -20,6 +20,14 @@
>>           },
>>           {
>>               "id": "SAF-2-safe",
>> +            "analyser": {
>> +                "eclair": "MC3R1.D4.10"
>> +            },
>> +            "name": "Dir 4.10: headers that leave it up to the caller to include them correctly",
>> +            "text": "Headers that deliberatively avoid inclusion guards explicitly leaving responsibility to the caller are allowed."
>> +        },
> 
> With this ...
> 
>> +        {
>> +            "id": "SAF-3-safe",
>>               "analyser": {},
>>               "name": "Sentinel",
>>               "text": "Next ID to be used"
>> diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
>> index 6b6ce2745c..eac1ae4b2a 100644
>> --- a/xen/include/public/arch-x86/cpufeatureset.h
>> +++ b/xen/include/public/arch-x86/cpufeatureset.h
>> @@ -23,6 +23,7 @@
>>    * their XEN_CPUFEATURE() being appropriate in the included context.
>>    */
>>   
>> +/* SAF-1-safe header that leaves it up to the caller to include them correctly */
>>   #ifndef XEN_CPUFEATURE
>>   
>>   /*
>> diff --git a/xen/include/public/errno.h b/xen/include/public/errno.h
>> index 5a78a7607c..8b60ac74ae 100644
>> --- a/xen/include/public/errno.h
>> +++ b/xen/include/public/errno.h
>> @@ -17,6 +17,7 @@
>>    * will unilaterally #undef XEN_ERRNO().
>>    */
>>   
>> +/* SAF-1-safe header that leaves it up to the caller to include them correctly */
>>   #ifndef XEN_ERRNO
>>   
>>   /*
> 
> ... you mean SAF-2-safe in both code comments. I did point out the problem
> with the sequential numbering (and resulting rebasing mistakes) when the
> scheme was introduced.
> 
> I also think the comments are too verbose. I don't mind them having an
> indication what specific issue they are about, but it shouldn't be more
> than a couple of words. Here maybe "omitted inclusion guard".
> 
> Jan

Yes, you are right: I've made a mistake when rebasing against 
origin/staging.

I will wait more comments on the series, then I will submit v3
with the correct IDs.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH v2 03/10] misra: add deviations for direct inclusion guards

[Jan Beulich]

On 12.09.2023 11:36, Simone Ballarin wrote:
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -36,6 +36,14 @@
>          },
>          {
>              "id": "SAF-4-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.D4.10"
> +            },
> +            "name": "Dir 4.10: direct inclusion guard before",
> +            "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
> +        },
> +        {
> +            "id": "SAF-5-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
> index ccd26c5184..24f8c61a73 100644
> --- a/xen/arch/arm/include/asm/hypercall.h
> +++ b/xen/arch/arm/include/asm/hypercall.h
> @@ -1,3 +1,4 @@
> +/* SAF-3-safe direct inclusion guard before */
>  #ifndef __XEN_HYPERCALL_H__
>  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>  #endif
> diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
> index ec2edc771e..dfdfe80021 100644
> --- a/xen/arch/x86/include/asm/hypercall.h
> +++ b/xen/arch/x86/include/asm/hypercall.h
> @@ -2,6 +2,7 @@
>   * asm-x86/hypercall.h
>   */
>  
> +/* SAF-3-safe direct inclusion guard before */
>  #ifndef __XEN_HYPERCALL_H__
>  #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>  #endif
> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
> index 0a2b16d05d..190ada7800 100644
> --- a/xen/include/xen/unaligned.h
> +++ b/xen/include/xen/unaligned.h
> @@ -3,6 +3,7 @@
>   * without faulting, and at least reasonably efficiently.  Other architectures
>   * will need to have a custom asm/unaligned.h.
>   */
> +/* SAF-3-safe direct inclusion guard before */
>  #ifndef __ASM_UNALIGNED_H__
>  #error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
>  #endif

Apart from the recurring off-by-1, will this have the intended effect of
Eclair still choking if there's then no inclusion guard following these
early constructs?

Jan

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

[Jan Beulich]

On 12.09.2023 11:36, Simone Ballarin wrote:
> --- a/xen/arch/x86/efi/runtime.h
> +++ b/xen/arch/x86/efi/runtime.h
> @@ -1,3 +1,6 @@
> +#ifndef __X86_EFI_RUNTIME_H__
> +#define __X86_EFI_RUNTIME_H__
> +
>  #include <xen/domain_page.h>
>  #include <xen/mm.h>
>  #include <asm/atomic.h>
> @@ -17,3 +20,5 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t l4e)
>      }
>  }
>  #endif
> +
> +#endif /* __X86_EFI_RUNTIME_H__ */

Leaving aside that I remain unconvinced of the usefulness of these in
(at least some) private headers, I think there's a naming issue to be
solved first: How do we distinguish guards of headers in xen/include/
and xen/arch/*/include/ from ones living elsewhere? If we don't set
forth a rule, the guard above might be re-used in a hypothetical
xen/arch/x86/include/asm/efi/runtime.h, with potentially interesting
effects. At a first glance it might work to simply omit "component"
identifiers, i.e. just use __RUNTIME_H__ here. Provided suitable
prefixes are used in all non-private headers. But I may of course be
overlooking pitfalls ...

Jan

Re: [XEN PATCH v2 03/10] misra: add deviations for direct inclusion guards

[Simone Ballarin]

On 12/09/23 11:52, Jan Beulich wrote:
> On 12.09.2023 11:36, Simone Ballarin wrote:
>> --- a/docs/misra/safe.json
>> +++ b/docs/misra/safe.json
>> @@ -36,6 +36,14 @@
>>           },
>>           {
>>               "id": "SAF-4-safe",
>> +            "analyser": {
>> +                "eclair": "MC3R1.D4.10"
>> +            },
>> +            "name": "Dir 4.10: direct inclusion guard before",
>> +            "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
>> +        },
>> +        {
>> +            "id": "SAF-5-safe",
>>               "analyser": {},
>>               "name": "Sentinel",
>>               "text": "Next ID to be used"
>> diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
>> index ccd26c5184..24f8c61a73 100644
>> --- a/xen/arch/arm/include/asm/hypercall.h
>> +++ b/xen/arch/arm/include/asm/hypercall.h
>> @@ -1,3 +1,4 @@
>> +/* SAF-3-safe direct inclusion guard before */
>>   #ifndef __XEN_HYPERCALL_H__
>>   #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>   #endif
>> diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
>> index ec2edc771e..dfdfe80021 100644
>> --- a/xen/arch/x86/include/asm/hypercall.h
>> +++ b/xen/arch/x86/include/asm/hypercall.h
>> @@ -2,6 +2,7 @@
>>    * asm-x86/hypercall.h
>>    */
>>   
>> +/* SAF-3-safe direct inclusion guard before */
>>   #ifndef __XEN_HYPERCALL_H__
>>   #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>   #endif
>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>> index 0a2b16d05d..190ada7800 100644
>> --- a/xen/include/xen/unaligned.h
>> +++ b/xen/include/xen/unaligned.h
>> @@ -3,6 +3,7 @@
>>    * without faulting, and at least reasonably efficiently.  Other architectures
>>    * will need to have a custom asm/unaligned.h.
>>    */
>> +/* SAF-3-safe direct inclusion guard before */
>>   #ifndef __ASM_UNALIGNED_H__
>>   #error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
>>   #endif
> 
> Apart from the recurring off-by-1, will this have the intended effect of
> Eclair still choking if there's then no inclusion guard following these
> early constructs?
> 
> Jan
> 

No, if you put something between the direct inclusion guard and the 
inclusion guard, no violation will be generated.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

Re: [XEN PATCH v2 03/10] misra: add deviations for direct inclusion guards

[Jan Beulich]

On 12.09.2023 12:05, Simone Ballarin wrote:
> On 12/09/23 11:52, Jan Beulich wrote:
>> On 12.09.2023 11:36, Simone Ballarin wrote:
>>> --- a/docs/misra/safe.json
>>> +++ b/docs/misra/safe.json
>>> @@ -36,6 +36,14 @@
>>>           },
>>>           {
>>>               "id": "SAF-4-safe",
>>> +            "analyser": {
>>> +                "eclair": "MC3R1.D4.10"
>>> +            },
>>> +            "name": "Dir 4.10: direct inclusion guard before",
>>> +            "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
>>> +        },
>>> +        {
>>> +            "id": "SAF-5-safe",
>>>               "analyser": {},
>>>               "name": "Sentinel",
>>>               "text": "Next ID to be used"
>>> diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
>>> index ccd26c5184..24f8c61a73 100644
>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>> @@ -1,3 +1,4 @@
>>> +/* SAF-3-safe direct inclusion guard before */
>>>   #ifndef __XEN_HYPERCALL_H__
>>>   #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>>   #endif
>>> diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
>>> index ec2edc771e..dfdfe80021 100644
>>> --- a/xen/arch/x86/include/asm/hypercall.h
>>> +++ b/xen/arch/x86/include/asm/hypercall.h
>>> @@ -2,6 +2,7 @@
>>>    * asm-x86/hypercall.h
>>>    */
>>>   
>>> +/* SAF-3-safe direct inclusion guard before */
>>>   #ifndef __XEN_HYPERCALL_H__
>>>   #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>>   #endif
>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>> index 0a2b16d05d..190ada7800 100644
>>> --- a/xen/include/xen/unaligned.h
>>> +++ b/xen/include/xen/unaligned.h
>>> @@ -3,6 +3,7 @@
>>>    * without faulting, and at least reasonably efficiently.  Other architectures
>>>    * will need to have a custom asm/unaligned.h.
>>>    */
>>> +/* SAF-3-safe direct inclusion guard before */
>>>   #ifndef __ASM_UNALIGNED_H__
>>>   #error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
>>>   #endif
>>
>> Apart from the recurring off-by-1, will this have the intended effect of
>> Eclair still choking if there's then no inclusion guard following these
>> early constructs?
> 
> No, if you put something between the direct inclusion guard and the 
> inclusion guard, no violation will be generated.

Hmm, that's not good. But the question was also the other way around: Will
there be a violation reported if the ordinary inclusion guard is missing
altogether? I.e. will the tool continue looking for the guard it expects
despite the SAF-<n>-safe comment?

Jan

Re: [XEN PATCH v2 03/10] misra: add deviations for direct inclusion guards

[Simone Ballarin]

On 12/09/23 12:19, Jan Beulich wrote:
> On 12.09.2023 12:05, Simone Ballarin wrote:
>> On 12/09/23 11:52, Jan Beulich wrote:
>>> On 12.09.2023 11:36, Simone Ballarin wrote:
>>>> --- a/docs/misra/safe.json
>>>> +++ b/docs/misra/safe.json
>>>> @@ -36,6 +36,14 @@
>>>>            },
>>>>            {
>>>>                "id": "SAF-4-safe",
>>>> +            "analyser": {
>>>> +                "eclair": "MC3R1.D4.10"
>>>> +            },
>>>> +            "name": "Dir 4.10: direct inclusion guard before",
>>>> +            "text": "Headers with just the direct inclusion guard before the inclusion guard are safe."
>>>> +        },
>>>> +        {
>>>> +            "id": "SAF-5-safe",
>>>>                "analyser": {},
>>>>                "name": "Sentinel",
>>>>                "text": "Next ID to be used"
>>>> diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
>>>> index ccd26c5184..24f8c61a73 100644
>>>> --- a/xen/arch/arm/include/asm/hypercall.h
>>>> +++ b/xen/arch/arm/include/asm/hypercall.h
>>>> @@ -1,3 +1,4 @@
>>>> +/* SAF-3-safe direct inclusion guard before */
>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>    #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>>>    #endif
>>>> diff --git a/xen/arch/x86/include/asm/hypercall.h b/xen/arch/x86/include/asm/hypercall.h
>>>> index ec2edc771e..dfdfe80021 100644
>>>> --- a/xen/arch/x86/include/asm/hypercall.h
>>>> +++ b/xen/arch/x86/include/asm/hypercall.h
>>>> @@ -2,6 +2,7 @@
>>>>     * asm-x86/hypercall.h
>>>>     */
>>>>    
>>>> +/* SAF-3-safe direct inclusion guard before */
>>>>    #ifndef __XEN_HYPERCALL_H__
>>>>    #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
>>>>    #endif
>>>> diff --git a/xen/include/xen/unaligned.h b/xen/include/xen/unaligned.h
>>>> index 0a2b16d05d..190ada7800 100644
>>>> --- a/xen/include/xen/unaligned.h
>>>> +++ b/xen/include/xen/unaligned.h
>>>> @@ -3,6 +3,7 @@
>>>>     * without faulting, and at least reasonably efficiently.  Other architectures
>>>>     * will need to have a custom asm/unaligned.h.
>>>>     */
>>>> +/* SAF-3-safe direct inclusion guard before */
>>>>    #ifndef __ASM_UNALIGNED_H__
>>>>    #error "xen/unaligned.h should not be included directly - include asm/unaligned.h instead"
>>>>    #endif
>>>
>>> Apart from the recurring off-by-1, will this have the intended effect of
>>> Eclair still choking if there's then no inclusion guard following these
>>> early constructs?
>>
>> No, if you put something between the direct inclusion guard and the
>> inclusion guard, no violation will be generated.
> 
> Hmm, that's not good. But the question was also the other way around: Will
> there be a violation reported if the ordinary inclusion guard is missing
> altogether? I.e. will the tool continue looking for the guard it expects
> despite the SAF-<n>-safe comment?
> 
> Jan
> 

The comment-based deviations currently work as follows:
each report (of the specified service) that has its location in the same 
line or in the line following the comment is deviated.

In this case, the location is the first token of the file: the "#ifndef" 
of the direct inclusion guard.

Every change made after the direct inclusion guard will not change the
location of the violation, so the deviation still applies.

The only change that will cause a violation would be adding something
(that is not a compliant inclusion guard) before the comment.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

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

[Stefano Stabellini]

On Tue, 12 Sep 2023, Simone Ballarin wrote:
> Add inclusion guard 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").
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Changes in v2:
> - drop changes in xen/arch/arm/include/asm/hypercall.h
> - drop changes in xen/arch/arm/include/asm/iocap.h since they are
>   not related with the directive
> ---
>  xen/arch/arm/efi/efi-boot.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 1c3640bb65..aaa468f186 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -3,6 +3,10 @@
>   * is intended to be included by common/efi/boot.c _only_, and
>   * therefore can define arch specific global variables.
>   */
> +
> +#ifndef __ARM_EFI_EFI_BOOT_H__
> +#define __ARM_EFI_EFI_BOOT_H__
> +
>  #include <xen/device_tree.h>
>  #include <xen/libfdt/libfdt.h>
>  #include <asm/setup.h>
> @@ -1003,6 +1007,8 @@ static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size)
>      __flush_dcache_area(vaddr, size);
>  }
>  
> +#endif /* __ARM_EFI_EFI_BOOT_H__ */
> +
>  /*
>   * Local variables:
>   * mode: C
> -- 
> 2.34.1
> 

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

[Stefano Stabellini]

On Tue, 12 Sep 2023, Simone Ballarin wrote:
> Add 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").
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

[Stefano Stabellini]

On Tue, 12 Sep 2023, Simone Ballarin wrote:
> Add 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").
> 
> Mechanical change.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

[Stefano Stabellini]

On Tue, 12 Sep 2023, Simone Ballarin wrote:
> Amended 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.
> 
> Mechanical change.

Missing signed-off-by line

Other than that:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

[Stefano Stabellini]

On Tue, 12 Sep 2023, Simone Ballarin wrote:
> Amend generation script, add 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").
> 
> This patch amends the Makefile adding the required inclusion guards
> for xlat.h.
> 
> Add deviation comment for files intended for multiple inclusion.
> 
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
> 
> ---
> Changes in v2:
> - merge patches 7/13 and 13/13 of v1 as they had the same
>   commit message
> - amend the Makefile to produce the required inclusion guard
> - use the format introduced with doc/misra/safe.json instead of
>   a generic text-based deviation
> ---
>  docs/misra/safe.json                   | 8 ++++++++
>  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/include/Makefile                   | 8 ++++++--
>  5 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/docs/misra/safe.json b/docs/misra/safe.json
> index 0ec594f6bf..82c636ee94 100644
> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -44,6 +44,14 @@
>          },
>          {
>              "id": "SAF-5-safe",

You might want to double-check the SAF id here as well


> +            "analyser": {
> +                "eclair": "MC3R1.D4.10"
> +            },
> +            "name": "Dir 4.10: file intended for multiple inclusion",
> +            "text": "Files intended for multiple inclusion are not supposed to comply with D4.10."
> +        },
> +        {
> +            "id": "SAF-6-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> diff --git a/xen/arch/x86/include/asm/compat.h b/xen/arch/x86/include/asm/compat.h
> index 818cad87db..3d3891d061 100644
> --- a/xen/arch/x86/include/asm/compat.h
> +++ b/xen/arch/x86/include/asm/compat.h
> @@ -2,6 +2,9 @@
>   * compat.h
>   */
>  
> +#ifndef __ASM_X86_COMPAT_H__
> +#define __ASM_X86_COMPAT_H__
> +
>  #ifdef CONFIG_COMPAT
>  
>  #define COMPAT_BITS_PER_LONG 32
> @@ -18,3 +21,5 @@ int switch_compat(struct domain *);
>  #include <xen/errno.h>
>  static inline int switch_compat(struct domain *d) { return -EOPNOTSUPP; }
>  #endif
> +
> +#endif /* __ASM_X86_COMPAT_H__ */
> diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
> index da0593de85..39b15e463a 100644
> --- a/xen/arch/x86/include/asm/cpufeatures.h
> +++ b/xen/arch/x86/include/asm/cpufeatures.h
> @@ -1,7 +1,4 @@
> -/*
> - * Explicitly intended for multiple inclusion.
> - */
> -
> +/* SAF-4-safe file intended for multiple inclusion */
>  #include <xen/lib/x86/cpuid-autogen.h>
>  
>  /* Number of capability words covered by the featureset words. */
> diff --git a/xen/arch/x86/include/asm/efibind.h b/xen/arch/x86/include/asm/efibind.h
> index bce02f3707..f2eb8b5496 100644
> --- a/xen/arch/x86/include/asm/efibind.h
> +++ b/xen/arch/x86/include/asm/efibind.h
> @@ -1,2 +1,7 @@
> +#ifndef __ASM_X86_EFIBIND_H__
> +#define __ASM_X86_EFIBIND_H__
> +
>  #include <xen/types.h>
>  #include <asm/x86_64/efibind.h>
> +
> +#endif /* __ASM_X86_EFIBIND_H__ */
> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index 31782fb177..b2f9576362 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -105,9 +105,13 @@ xlat-y := $(shell sed -ne 's,@arch@,$(compat-arch-y),g' -re 's,^[?!][[:blank:]]+
>  xlat-y := $(filter $(patsubst compat/%,%,$(headers-y)),$(xlat-y))
>  
>  quiet_cmd_xlat_h = GEN     $@
> -cmd_xlat_h = \
> -	cat $(filter %.h,$^) >$@.new; \
> +define cmd_xlat_h
> +	echo "#ifndef _COMPAT_XLAT_H" > $@.new; \
> +	echo "#define _COMPAT_XLAT_H" >> $@.new; \
> +	cat $(filter %.h,$^) >> $@.new; \
> +	echo "#endif /* _COMPAT_XLAT_H */" >> $@.new; \
>  	mv -f $@.new $@
> +endef
>  
>  $(obj)/compat/xlat.h: $(addprefix $(obj)/compat/.xlat/,$(xlat-y)) FORCE
>  	$(call if_changed,xlat_h)

I checked that everything works as expected with this change and it does. I am not
sure about the choice of name "_COMPAT_XLAT_H" but anyway:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

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

[Jan Beulich]

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

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

[Simone Ballarin]

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?

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

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

[Jan Beulich]

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

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

[Simone Ballarin]

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



-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

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

[Jan Beulich]

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

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

[Simone Ballarin]

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.



-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

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

[Stefano Stabellini]

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__ ?

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

[Simone Ballarin]

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__ ?

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)

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

[Stefano Stabellini]

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__ ?

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

[Jan Beulich]

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

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

[Stefano Stabellini]

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?

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

[Jan Beulich]

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

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

[Stefano Stabellini]

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?

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

[Jan Beulich]

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

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

[Stefano Stabellini]

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.

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

[Jan Beulich]

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

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

[Stefano Stabellini]

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.

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

[Jan Beulich]

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

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

[Stefano Stabellini]

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?

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Jan Beulich]

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

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

[Stefano Stabellini]

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?

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

[Jan Beulich]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Jan Beulich]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Stefano Stabellini]

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.

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

[Jan Beulich]

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

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

[Stefano Stabellini]

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.

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

[Simone Ballarin]

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.

-- 
Simone Ballarin, M.Sc.

Field Application Engineer, BUGSENG (https://bugseng.com)