diff mbox series

[14/17] xen: add SAF deviation for MISRA C Dir 4.10

Message ID b64a6b53de8bcf14c91a1534bb57b001efc12cce.1719829101.git.alessandro.zucchelli@bugseng.com (mailing list archive)
State Superseded
Headers show
Series xen: address violation of MISRA C:2012 Directive 4.10 | expand

Commit Message

Alessandro Zucchelli July 1, 2024, 1:45 p.m. UTC
From: Nicola Vetrini <nicola.vetrini@bugseng.com>

Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

---
 docs/misra/safe.json              | 10 +++++++++-
 xen/include/public/arch-x86/xen.h |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Jan Beulich July 3, 2024, 1:23 p.m. UTC | #1
On 01.07.2024 15:45, Alessandro Zucchelli wrote:
> From: Nicola Vetrini <nicola.vetrini@bugseng.com>
> 
> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>

So no description at all for a somewhat unobvious issue with, I think,
a pretty obvious (but entirely different) solution? And that (obvious)
alternative not even being mentioned, together with why it was not
possible to use? Neither ...

> --- a/docs/misra/safe.json
> +++ b/docs/misra/safe.json
> @@ -99,7 +99,15 @@
>              "text": "Files intended for multiple inclusion are not supposed to comply with D4.10."
>          },
>          {
> -            "id": "SAF-11-safe",
> +            "id": "SAF-12-safe",
> +            "analyser": {
> +                "eclair": "MC3R1.D4.10"
> +            },
> +            "name": "Dir 4.10: arch-x86/xen.h include before guard",
> +            "text": "This file needs to start with #include ../xen.h to generate preprocessed code in the correct order."

... here nor ...

> +        },
> +        {
> +            "id": "SAF-13-safe",
>              "analyser": {},
>              "name": "Sentinel",
>              "text": "Next ID to be used"
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -7,6 +7,7 @@
>   * Copyright (c) 2004-2006, K A Fraser
>   */
>  
> +/* SAF-12-safe include before guard needed for correct code generation */
>  #include "../xen.h"
>  
>  #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__

... here is really becomes clear what "correct" is, or what breaks if this
was moved. However, thinking of moving as the first obvious alternative I
checked other arch-specific headers. None includes ../xen.h (really just
xen.h, as the others all live right in public/) like this. Which made me
conclude that maybe there's something wrong with the x86 header doing so.
And indeed, according to my build testing the #include can simple be
dropped, with just one further change elsewhere; see below.

Jan

public/x86: don't include common xen.h from arch-specific one

No other arch-*.h does so, and arch-x86/xen.h really just takes the role
of arch-x86_32.h and arch-x86_64.h (by those two forwarding there). With
xen.h itself including the per-arch headers, doing so is also kind of
backwards anyway, and just calling for problems. There's exactly one
place where arch-x86/xen.h is included when really xen.h is meant (for
wanting XEN_GUEST_HANDLE_64() to be made available, the default
definition of which lives in the common xen.h).

This then addresses a violation 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").

Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -7,8 +7,6 @@
  * Copyright (c) 2004-2006, K A Fraser
  */
 
-#include "../xen.h"
-
 #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__
 #define __XEN_PUBLIC_ARCH_X86_XEN_H__
 
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -525,7 +525,7 @@ void x86_cpu_policy_bound_max_leaves(str
 void x86_cpu_policy_shrink_max_leaves(struct cpu_policy *p);
 
 #ifdef __XEN__
-#include <public/arch-x86/xen.h>
+#include <public/xen.h>
 typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
 typedef XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_entry_buffer_t;
 #else
Stefano Stabellini July 12, 2024, 10:28 p.m. UTC | #2
On Wed, 3 Jul 2024, Jan Beulich wrote:
> public/x86: don't include common xen.h from arch-specific one
> 
> No other arch-*.h does so, and arch-x86/xen.h really just takes the role
> of arch-x86_32.h and arch-x86_64.h (by those two forwarding there). With
> xen.h itself including the per-arch headers, doing so is also kind of
> backwards anyway, and just calling for problems. There's exactly one
> place where arch-x86/xen.h is included when really xen.h is meant (for
> wanting XEN_GUEST_HANDLE_64() to be made available, the default
> definition of which lives in the common xen.h).
> 
> This then addresses a violation 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").
> 
> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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


> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -7,8 +7,6 @@
>   * Copyright (c) 2004-2006, K A Fraser
>   */
>  
> -#include "../xen.h"
> -
>  #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__
>  #define __XEN_PUBLIC_ARCH_X86_XEN_H__
>  
> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -525,7 +525,7 @@ void x86_cpu_policy_bound_max_leaves(str
>  void x86_cpu_policy_shrink_max_leaves(struct cpu_policy *p);
>  
>  #ifdef __XEN__
> -#include <public/arch-x86/xen.h>
> +#include <public/xen.h>
>  typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
>  typedef XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_entry_buffer_t;
>  #else
> 
>
Alessandro Zucchelli July 22, 2024, 8:54 a.m. UTC | #3
On 2024-07-13 00:28, Stefano Stabellini wrote:
> On Wed, 3 Jul 2024, Jan Beulich wrote:
>> public/x86: don't include common xen.h from arch-specific one
>> 
>> No other arch-*.h does so, and arch-x86/xen.h really just takes the 
>> role
>> of arch-x86_32.h and arch-x86_64.h (by those two forwarding there). 
>> With
>> xen.h itself including the per-arch headers, doing so is also kind of
>> backwards anyway, and just calling for problems. There's exactly one
>> place where arch-x86/xen.h is included when really xen.h is meant (for
>> wanting XEN_GUEST_HANDLE_64() to be made available, the default
>> definition of which lives in the common xen.h).
>> 
>> This then addresses a violation 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").
>> 
>> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

One question: when making the new version of the patch series should I
revert this commit as Jan made the patch for it himself, or should Jan's 
fixes
be integrated in the patch series?

Many thanks in advance,
Alessandro Zucchelli

>> --- a/xen/include/public/arch-x86/xen.h
>> +++ b/xen/include/public/arch-x86/xen.h
>> @@ -7,8 +7,6 @@
>>   * Copyright (c) 2004-2006, K A Fraser
>>   */
>> 
>> -#include "../xen.h"
>> -
>>  #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__
>>  #define __XEN_PUBLIC_ARCH_X86_XEN_H__
>> 
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -525,7 +525,7 @@ void x86_cpu_policy_bound_max_leaves(str
>>  void x86_cpu_policy_shrink_max_leaves(struct cpu_policy *p);
>> 
>>  #ifdef __XEN__
>> -#include <public/arch-x86/xen.h>
>> +#include <public/xen.h>
>>  typedef XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_leaf_buffer_t;
>>  typedef XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_entry_buffer_t;
>>  #else
>> 
>>
Jan Beulich July 22, 2024, 9:14 a.m. UTC | #4
On 22.07.2024 10:54, Alessandro Zucchelli wrote:
> On 2024-07-13 00:28, Stefano Stabellini wrote:
>> On Wed, 3 Jul 2024, Jan Beulich wrote:
>>> public/x86: don't include common xen.h from arch-specific one
>>>
>>> No other arch-*.h does so, and arch-x86/xen.h really just takes the 
>>> role
>>> of arch-x86_32.h and arch-x86_64.h (by those two forwarding there). 
>>> With
>>> xen.h itself including the per-arch headers, doing so is also kind of
>>> backwards anyway, and just calling for problems. There's exactly one
>>> place where arch-x86/xen.h is included when really xen.h is meant (for
>>> wanting XEN_GUEST_HANDLE_64() to be made available, the default
>>> definition of which lives in the common xen.h).
>>>
>>> This then addresses a violation 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").
>>>
>>> Reported-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> One question: when making the new version of the patch series should I
> revert this commit as Jan made the patch for it himself, or should Jan's 
> fixes
> be integrated in the patch series?

You certainly want to leave out this patch. Whether you want to add my
alternative into the series is really up to you. I intend to commit it
relatively soon anyway.

Jan
diff mbox series

Patch

diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 0739eac806..a1cd821aea 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -99,7 +99,15 @@ 
             "text": "Files intended for multiple inclusion are not supposed to comply with D4.10."
         },
         {
-            "id": "SAF-11-safe",
+            "id": "SAF-12-safe",
+            "analyser": {
+                "eclair": "MC3R1.D4.10"
+            },
+            "name": "Dir 4.10: arch-x86/xen.h include before guard",
+            "text": "This file needs to start with #include ../xen.h to generate preprocessed code in the correct order."
+        },
+        {
+            "id": "SAF-13-safe",
             "analyser": {},
             "name": "Sentinel",
             "text": "Next ID to be used"
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index a9a87d9b50..d8ad935af3 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -7,6 +7,7 @@ 
  * Copyright (c) 2004-2006, K A Fraser
  */
 
+/* SAF-12-safe include before guard needed for correct code generation */
 #include "../xen.h"
 
 #ifndef __XEN_PUBLIC_ARCH_X86_XEN_H__