| Message ID | 2d37ca48cf6556ebfcb4b17cddd2446fa549f689.1693228255.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address violations of MISRA C:2012 Directive 4.10 | expand |
On Mon, 28 Aug 2023, 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). > > Mechanical change. > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/arm/efi/efi-boot.h | 6 ++++++ > xen/arch/arm/include/asm/hypercall.h | 6 +++--- > xen/arch/arm/include/asm/iocap.h | 6 +++--- > 3 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index 1c3640bb65..aba522ead5 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 > diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h > index ccd26c5184..4f4d96f1f2 100644 > --- a/xen/arch/arm/include/asm/hypercall.h > +++ b/xen/arch/arm/include/asm/hypercall.h > @@ -1,10 +1,10 @@ > +#ifndef __ASM_ARM_HYPERCALL_H__ > +#define __ASM_ARM_HYPERCALL_H__ > + > #ifndef __XEN_HYPERCALL_H__ > #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" > #endif > > -#ifndef __ASM_ARM_HYPERCALL_H__ > -#define __ASM_ARM_HYPERCALL_H__ > - > #include <public/domctl.h> /* for arch_do_domctl */ > > long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d, > diff --git a/xen/arch/arm/include/asm/iocap.h b/xen/arch/arm/include/asm/iocap.h > index 276fefbc59..4db1b16839 100644 > --- a/xen/arch/arm/include/asm/iocap.h > +++ b/xen/arch/arm/include/asm/iocap.h > @@ -1,10 +1,10 @@ > -#ifndef __X86_IOCAP_H__ > -#define __X86_IOCAP_H__ > +#ifndef __ASM_ARM_IOCAP_H__ > +#define __ASM_ARM_IOCAP_H__ > > #define cache_flush_permitted(d) \ > (!rangeset_is_empty((d)->iomem_caps)) > > -#endif > +#endif /* __ASM_ARM_IOCAP_H__ */ > > /* > * Local variables: > -- > 2.34.1 >
Hi, On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@bugseng.com> 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). > > Mechanical change. > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > --- > xen/arch/arm/efi/efi-boot.h | 6 ++++++ > xen/arch/arm/include/asm/hypercall.h | 6 +++--- > xen/arch/arm/include/asm/iocap.h | 6 +++--- > 3 files changed, 12 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h > index 1c3640bb65..aba522ead5 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 > diff --git a/xen/arch/arm/include/asm/hypercall.h > b/xen/arch/arm/include/asm/hypercall.h > index ccd26c5184..4f4d96f1f2 100644 > --- a/xen/arch/arm/include/asm/hypercall.h > +++ b/xen/arch/arm/include/asm/hypercall.h > @@ -1,10 +1,10 @@ > +#ifndef __ASM_ARM_HYPERCALL_H__ > +#define __ASM_ARM_HYPERCALL_H__ > + > #ifndef __XEN_HYPERCALL_H__ > #error "asm/hypercall.h should not be included directly - include > xen/hypercall.h instead" > #endif > > -#ifndef __ASM_ARM_HYPERCALL_H__ > -#define __ASM_ARM_HYPERCALL_H__ > - I understand that you are trying to fix a misra violation. However, this feels like it was done on purpose. With the new change, you would not always check that the file were included at the correct place. I am not against this change but this ought to be explained. > #include <public/domctl.h> /* for arch_do_domctl */ > > long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d, > diff --git a/xen/arch/arm/include/asm/iocap.h > b/xen/arch/arm/include/asm/iocap.h > index 276fefbc59..4db1b16839 100644 > --- a/xen/arch/arm/include/asm/iocap.h > +++ b/xen/arch/arm/include/asm/iocap.h > @@ -1,10 +1,10 @@ > -#ifndef __X86_IOCAP_H__ > -#define __X86_IOCAP_H__ > +#ifndef __ASM_ARM_IOCAP_H__ > +#define __ASM_ARM_IOCAP_H__ > > #define cache_flush_permitted(d) \ > (!rangeset_is_empty((d)->iomem_caps)) > > -#endif > +#endif /* __ASM_ARM_IOCAP_H__ */ I don’t understand how this is related to the rest of the patch. You wrote that inclusion must appear first and this is the case here. However the name is technically not correct. Is this really related to directive 4.10? If so, this should be clarified in the commit message. If not, then I think this should be in a separate commit. Cheers, > > /* > * Local variables: > -- > 2.34.1 > >
On 29/08/23 00:10, Julien Grall wrote: > Hi, > > On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@bugseng.com> > 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). >> >> Mechanical change. >> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> --- >> xen/arch/arm/efi/efi-boot.h | 6 ++++++ >> xen/arch/arm/include/asm/hypercall.h | 6 +++--- >> xen/arch/arm/include/asm/iocap.h | 6 +++--- >> 3 files changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h >> index 1c3640bb65..aba522ead5 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 >> diff --git a/xen/arch/arm/include/asm/hypercall.h >> b/xen/arch/arm/include/asm/hypercall.h >> index ccd26c5184..4f4d96f1f2 100644 >> --- a/xen/arch/arm/include/asm/hypercall.h >> +++ b/xen/arch/arm/include/asm/hypercall.h >> @@ -1,10 +1,10 @@ >> +#ifndef __ASM_ARM_HYPERCALL_H__ >> +#define __ASM_ARM_HYPERCALL_H__ >> + >> #ifndef __XEN_HYPERCALL_H__ >> #error "asm/hypercall.h should not be included directly - include >> xen/hypercall.h instead" >> #endif >> >> -#ifndef __ASM_ARM_HYPERCALL_H__ >> -#define __ASM_ARM_HYPERCALL_H__ >> - > > > I understand that you are trying to fix a misra violation. However, this > feels like it was done on purpose. > > With the new change, you would not always check that the file were included > at the correct place. I am not against this change but this ought to be > explained. I don't think the semantics have changed. Please correct me if I'm wrong. With this change, the only situation where the check is not performed is when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be useless. The same thing happened with the code before the change: if I include the file after xen/hypercall.h, the check will always succeed. > > >> #include <public/domctl.h> /* for arch_do_domctl */ >> >> long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d, >> diff --git a/xen/arch/arm/include/asm/iocap.h >> b/xen/arch/arm/include/asm/iocap.h >> index 276fefbc59..4db1b16839 100644 >> --- a/xen/arch/arm/include/asm/iocap.h >> +++ b/xen/arch/arm/include/asm/iocap.h >> @@ -1,10 +1,10 @@ >> -#ifndef __X86_IOCAP_H__ >> -#define __X86_IOCAP_H__ >> +#ifndef __ASM_ARM_IOCAP_H__ >> +#define __ASM_ARM_IOCAP_H__ >> >> #define cache_flush_permitted(d) \ >> (!rangeset_is_empty((d)->iomem_caps)) >> >> -#endif >> +#endif /* __ASM_ARM_IOCAP_H__ */ > > > I don’t understand how this is related to the rest of the patch. You wrote > that inclusion must appear first and this is the case here. > > However the name is technically not correct. Is this really related to > directive 4.10? If so, this should be clarified in the commit message. If > not, then I think this should be in a separate commit. > Yes, you are right. This is not correlated to this series. I will put it on a separate commit. > Cheers, > > >> >> /* >> * Local variables: >> -- >> 2.34.1 >> >> >
On 30.08.2023 14:53, Simone Ballarin wrote: > On 29/08/23 00:10, Julien Grall wrote: >> On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@bugseng.com> >> wrote: >>> --- a/xen/arch/arm/include/asm/hypercall.h >>> +++ b/xen/arch/arm/include/asm/hypercall.h >>> @@ -1,10 +1,10 @@ >>> +#ifndef __ASM_ARM_HYPERCALL_H__ >>> +#define __ASM_ARM_HYPERCALL_H__ >>> + >>> #ifndef __XEN_HYPERCALL_H__ >>> #error "asm/hypercall.h should not be included directly - include >>> xen/hypercall.h instead" >>> #endif >>> >>> -#ifndef __ASM_ARM_HYPERCALL_H__ >>> -#define __ASM_ARM_HYPERCALL_H__ >>> - >> >> >> I understand that you are trying to fix a misra violation. However, this >> feels like it was done on purpose. >> >> With the new change, you would not always check that the file were included >> at the correct place. I am not against this change but this ought to be >> explained. > I don't think the semantics have changed. Please correct me if I'm wrong. > > With this change, the only situation where the check is not performed is > when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been > successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is > defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be > useless. > > The same thing happened with the code before the change: if I include > the file after xen/hypercall.h, the check will always succeed. Hmm, I think you're right, but I draw a different conclusion: The check fails to work as intended. And this can only be repaired without your adjustment. Jan
On 30/08/23 15:01, Jan Beulich wrote: > On 30.08.2023 14:53, Simone Ballarin wrote: >> On 29/08/23 00:10, Julien Grall wrote: >>> On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@bugseng.com> >>> wrote: >>>> --- a/xen/arch/arm/include/asm/hypercall.h >>>> +++ b/xen/arch/arm/include/asm/hypercall.h >>>> @@ -1,10 +1,10 @@ >>>> +#ifndef __ASM_ARM_HYPERCALL_H__ >>>> +#define __ASM_ARM_HYPERCALL_H__ >>>> + >>>> #ifndef __XEN_HYPERCALL_H__ >>>> #error "asm/hypercall.h should not be included directly - include >>>> xen/hypercall.h instead" >>>> #endif >>>> >>>> -#ifndef __ASM_ARM_HYPERCALL_H__ >>>> -#define __ASM_ARM_HYPERCALL_H__ >>>> - >>> >>> >>> I understand that you are trying to fix a misra violation. However, this >>> feels like it was done on purpose. >>> >>> With the new change, you would not always check that the file were included >>> at the correct place. I am not against this change but this ought to be >>> explained. >> I don't think the semantics have changed. Please correct me if I'm wrong. >> >> With this change, the only situation where the check is not performed is >> when __ASM_ARM_HYPERCALL_H__ is defined (i.e. the file has already been >> successfully included). This implies that if __ASM_ARM_HYPERCALL_H__ is >> defined, then __XEN_HYPERCALL_H__ is also defined, so the check would be >> useless. >> >> The same thing happened with the code before the change: if I include >> the file after xen/hypercall.h, the check will always succeed. > > Hmm, I think you're right, but I draw a different conclusion: The check > fails to work as intended. And this can only be repaired without your > adjustment. > Ok, I will just deviate these cases. > Jan
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index 1c3640bb65..aba522ead5 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 diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h index ccd26c5184..4f4d96f1f2 100644 --- a/xen/arch/arm/include/asm/hypercall.h +++ b/xen/arch/arm/include/asm/hypercall.h @@ -1,10 +1,10 @@ +#ifndef __ASM_ARM_HYPERCALL_H__ +#define __ASM_ARM_HYPERCALL_H__ + #ifndef __XEN_HYPERCALL_H__ #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" #endif -#ifndef __ASM_ARM_HYPERCALL_H__ -#define __ASM_ARM_HYPERCALL_H__ - #include <public/domctl.h> /* for arch_do_domctl */ long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d, diff --git a/xen/arch/arm/include/asm/iocap.h b/xen/arch/arm/include/asm/iocap.h index 276fefbc59..4db1b16839 100644 --- a/xen/arch/arm/include/asm/iocap.h +++ b/xen/arch/arm/include/asm/iocap.h @@ -1,10 +1,10 @@ -#ifndef __X86_IOCAP_H__ -#define __X86_IOCAP_H__ +#ifndef __ASM_ARM_IOCAP_H__ +#define __ASM_ARM_IOCAP_H__ #define cache_flush_permitted(d) \ (!rangeset_is_empty((d)->iomem_caps)) -#endif +#endif /* __ASM_ARM_IOCAP_H__ */ /* * Local variables:
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> --- xen/arch/arm/efi/efi-boot.h | 6 ++++++ xen/arch/arm/include/asm/hypercall.h | 6 +++--- xen/arch/arm/include/asm/iocap.h | 6 +++--- 3 files changed, 12 insertions(+), 6 deletions(-)