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