| Message ID | fe1768342df0cd9315af87c83cc6d8d09f61b983.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 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"). > > Also C files, if included somewhere, need to comply with the guideline. > > Mechanical change. > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/common/compat/grant_table.c | 7 +++++++ > xen/common/coverage/gcc_4_7.c | 5 +++++ > xen/common/decompress.h | 5 +++++ > xen/common/event_channel.h | 5 +++++ > xen/common/multicall.c | 5 +++++ > 5 files changed, 27 insertions(+) > > diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c > index f8177c84c0..614ad71a59 100644 > --- a/xen/common/compat/grant_table.c > +++ b/xen/common/compat/grant_table.c > @@ -3,6 +3,10 @@ > * > */ > > + > +#ifndef __COMMON_COMPAT_GRANT_TABLE_C__ > +#define __COMMON_COMPAT_GRANT_TABLE_C__ > + > #include <xen/hypercall.h> > #include <compat/grant_table.h> > > @@ -331,6 +335,9 @@ int compat_grant_table_op( > return rc; > } > > + > +#endif /* __COMMON_COMPAT_GRANT_TABLE_C__ */ > + > /* > * Local variables: > * mode: C > diff --git a/xen/common/coverage/gcc_4_7.c b/xen/common/coverage/gcc_4_7.c > index 25b4a8bcdc..12e4ec8cbb 100644 > --- a/xen/common/coverage/gcc_4_7.c > +++ b/xen/common/coverage/gcc_4_7.c > @@ -14,6 +14,9 @@ > * Wei Liu <wei.liu2@citrix.com> > */ > > +#ifndef __COMMON_COVERAGE_GCC_4_7_C__ > +#define __COMMON_COVERAGE_GCC_4_7_C__ > + > #include <xen/string.h> > > #include "gcov.h" > @@ -193,6 +196,8 @@ size_t gcov_info_to_gcda(char *buffer, const struct gcov_info *info) > return pos; > } > > +#endif /* __COMMON_COVERAGE_GCC_4_7_C__ */ > + > /* > * Local variables: > * mode: C > 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 > diff --git a/xen/common/multicall.c b/xen/common/multicall.c > index 1f0cc4cb26..421bb25b70 100644 > --- a/xen/common/multicall.c > +++ b/xen/common/multicall.c > @@ -2,6 +2,9 @@ > * multicall.c > */ > > +#ifndef __COMMON_MULTICALL_C__ > +#define __COMMON_MULTICALL_C__ > + > #include <xen/types.h> > #include <xen/lib.h> > #include <xen/mm.h> > @@ -124,6 +127,8 @@ ret_t do_multicall( > __HYPERVISOR_multicall, "hi", call_list, nr_calls-i); > } > > +#endif /* __COMMON_MULTICALL_C__ */ > + > /* > * Local variables: > * mode: C > -- > 2.34.1 >
On 28.08.2023 15:20, 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"). > > Also C files, if included somewhere, need to comply with the guideline. > > Mechanical change. > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > --- > xen/common/compat/grant_table.c | 7 +++++++ > xen/common/coverage/gcc_4_7.c | 5 +++++ > xen/common/decompress.h | 5 +++++ > xen/common/event_channel.h | 5 +++++ > xen/common/multicall.c | 5 +++++ > 5 files changed, 27 insertions(+) As already said in reply to another patch, imo .c files shouldn't gain such guards. These are commonly referred to as "header guards" for a reason. > --- a/xen/common/compat/grant_table.c > +++ b/xen/common/compat/grant_table.c > @@ -3,6 +3,10 @@ > * > */ > > + Nit: No double blank lines please. > +#ifndef __COMMON_COMPAT_GRANT_TABLE_C__ > +#define __COMMON_COMPAT_GRANT_TABLE_C__ > + > #include <xen/hypercall.h> > #include <compat/grant_table.h> > > @@ -331,6 +335,9 @@ int compat_grant_table_op( > return rc; > } > > + Again here (at least). Jan
On 29/08/23 08:50, Jan Beulich wrote: > On 28.08.2023 15:20, 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"). >> >> Also C files, if included somewhere, need to comply with the guideline. >> >> Mechanical change. >> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> --- >> xen/common/compat/grant_table.c | 7 +++++++ >> xen/common/coverage/gcc_4_7.c | 5 +++++ >> xen/common/decompress.h | 5 +++++ >> xen/common/event_channel.h | 5 +++++ >> xen/common/multicall.c | 5 +++++ >> 5 files changed, 27 insertions(+) > > As already said in reply to another patch, imo .c files shouldn't gain such > guards. These are commonly referred to as "header guards" for a reason. > This is the MISRA's definition of "header file" (MISRA C:2012 Revision 1, Appendix J): "A header file is any file that is the subject of a #include directive. Note: the filename extension is not significant." So, the guards are required if we want to comply with the directive, otherwise we can raise a deviation. The danger of multi-inclusion also exists for .c files, why do you want to avoid guards for them? >> --- a/xen/common/compat/grant_table.c >> +++ b/xen/common/compat/grant_table.c >> @@ -3,6 +3,10 @@ >> * >> */ >> >> + > > Nit: No double blank lines please. > >> +#ifndef __COMMON_COMPAT_GRANT_TABLE_C__ >> +#define __COMMON_COMPAT_GRANT_TABLE_C__ >> + >> #include <xen/hypercall.h> >> #include <compat/grant_table.h> >> >> @@ -331,6 +335,9 @@ int compat_grant_table_op( >> return rc; >> } >> >> + > > Again here (at least). > > Jan
On 31.08.2023 12:08, Simone Ballarin wrote: > On 29/08/23 08:50, Jan Beulich wrote: >> On 28.08.2023 15:20, 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"). >>> >>> Also C files, if included somewhere, need to comply with the guideline. >>> >>> Mechanical change. >>> >>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >>> --- >>> xen/common/compat/grant_table.c | 7 +++++++ >>> xen/common/coverage/gcc_4_7.c | 5 +++++ >>> xen/common/decompress.h | 5 +++++ >>> xen/common/event_channel.h | 5 +++++ >>> xen/common/multicall.c | 5 +++++ >>> 5 files changed, 27 insertions(+) >> >> As already said in reply to another patch, imo .c files shouldn't gain such >> guards. These are commonly referred to as "header guards" for a reason. >> > > This is the MISRA's definition of "header file" (MISRA C:2012 Revision > 1, Appendix J): > > "A header file is any file that is the subject of a #include > directive. > Note: the filename extension is not significant." That's completely misleading terminology then. > So, the guards are required if we want to comply with the directive, > otherwise we can raise a deviation. > > The danger of multi-inclusion also exists for .c files, why do you want > to avoid guards for them? Counter question: Why only add guards to some of them? (My personal answer is "Because it's extra clutter.") Jan
On 31/08/23 13:10, Jan Beulich wrote: > On 31.08.2023 12:08, Simone Ballarin wrote: >> On 29/08/23 08:50, Jan Beulich wrote: >>> On 28.08.2023 15:20, 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"). >>>> >>>> Also C files, if included somewhere, need to comply with the guideline. >>>> >>>> Mechanical change. >>>> >>>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >>>> --- >>>> xen/common/compat/grant_table.c | 7 +++++++ >>>> xen/common/coverage/gcc_4_7.c | 5 +++++ >>>> xen/common/decompress.h | 5 +++++ >>>> xen/common/event_channel.h | 5 +++++ >>>> xen/common/multicall.c | 5 +++++ >>>> 5 files changed, 27 insertions(+) >>> >>> As already said in reply to another patch, imo .c files shouldn't gain such >>> guards. These are commonly referred to as "header guards" for a reason. >>> >> >> This is the MISRA's definition of "header file" (MISRA C:2012 Revision >> 1, Appendix J): >> >> "A header file is any file that is the subject of a #include >> directive. >> Note: the filename extension is not significant." > > That's completely misleading terminology then. > I might agree with you on this, but either way this is the definition to apply when reading the guideline. IMHO here, the best would be to use a separate extension for "C files intended to be included" (but that's not the point). >> So, the guards are required if we want to comply with the directive, >> otherwise we can raise a deviation. >> >> The danger of multi-inclusion also exists for .c files, why do you want >> to avoid guards for them? > > Counter question: Why only add guards to some of them? (My personal > answer is "Because it's extra clutter.") > > Jan > It's not "some of them", it's exactly the ones used in an #include directive, so I'm not getting your objection. By the way, if the community agrees, I can deviate all C files and drop the changes.
On 31.08.2023 14:54, Simone Ballarin wrote: > On 31/08/23 13:10, Jan Beulich wrote: >> On 31.08.2023 12:08, Simone Ballarin wrote: >>> The danger of multi-inclusion also exists for .c files, why do you want >>> to avoid guards for them? >> >> Counter question: Why only add guards to some of them? (My personal >> answer is "Because it's extra clutter.") > > It's not "some of them", it's exactly the ones used in an #include > directive, so I'm not getting your objection. My point is that by adding guards only for files we presently use in some #include directive, we set us up for introducing new violations as soon as another .c file becomes the subject of an #include. The more that it is unusual to add guards in .c files, i.e. it is to be expected that people wouldn't think about this extra Misra requirement. Jan
On 31/08/23 15:05, Jan Beulich wrote: > On 31.08.2023 14:54, Simone Ballarin wrote: >> On 31/08/23 13:10, Jan Beulich wrote: >>> On 31.08.2023 12:08, Simone Ballarin wrote: >>>> The danger of multi-inclusion also exists for .c files, why do you want >>>> to avoid guards for them? >>> >>> Counter question: Why only add guards to some of them? (My personal >>> answer is "Because it's extra clutter.") >> >> It's not "some of them", it's exactly the ones used in an #include >> directive, so I'm not getting your objection. > > My point is that by adding guards only for files we presently use in some > #include directive, we set us up for introducing new violations as soon > as another .c file becomes the subject of an #include.The more that it > is unusual to add guards in .c files, i.e. it is to be expected that > people wouldn't think about this extra Misra requirement. > > Jan I can agree to partially adopt the directive: I will add a deviation for C files in rules.txt.
On Thu, 31 Aug 2023, Simone Ballarin wrote: > On 31/08/23 15:05, Jan Beulich wrote: > > On 31.08.2023 14:54, Simone Ballarin wrote: > > > On 31/08/23 13:10, Jan Beulich wrote: > > > > On 31.08.2023 12:08, Simone Ballarin wrote: > > > > > The danger of multi-inclusion also exists for .c files, why do you > > > > > want > > > > > to avoid guards for them? > > > > > > > > Counter question: Why only add guards to some of them? (My personal > > > > answer is "Because it's extra clutter.") > > > > > > It's not "some of them", it's exactly the ones used in an #include > > > directive, so I'm not getting your objection. > > > > My point is that by adding guards only for files we presently use in some > > #include directive, we set us up for introducing new violations as soon > > as another .c file becomes the subject of an #include.The more that it > > is unusual to add guards in .c files, i.e. it is to be expected that > > people wouldn't think about this extra Misra requirement. > > > > Jan > > I can agree to partially adopt the directive: I will add a deviation for C > files in rules.txt. Sorry for the late reply as I was OOO. Please hold on before adding a deviation for C files. In general, I think including .c files is not common behavior, and should be restricted to special cases. We could say that exactly because they are special, they follow different rules so we can skip the guards. Or we could say that they are still at risk of double-inclusion, hence we should be consistent and protect them too. I think we should discuss the topic during the next MISRA C meeting.
On 06.09.2023 00:18, Stefano Stabellini wrote: > On Thu, 31 Aug 2023, Simone Ballarin wrote: >> On 31/08/23 15:05, Jan Beulich wrote: >>> On 31.08.2023 14:54, Simone Ballarin wrote: >>>> On 31/08/23 13:10, Jan Beulich wrote: >>>>> On 31.08.2023 12:08, Simone Ballarin wrote: >>>>>> The danger of multi-inclusion also exists for .c files, why do you >>>>>> want >>>>>> to avoid guards for them? >>>>> >>>>> Counter question: Why only add guards to some of them? (My personal >>>>> answer is "Because it's extra clutter.") >>>> >>>> It's not "some of them", it's exactly the ones used in an #include >>>> directive, so I'm not getting your objection. >>> >>> My point is that by adding guards only for files we presently use in some >>> #include directive, we set us up for introducing new violations as soon >>> as another .c file becomes the subject of an #include.The more that it >>> is unusual to add guards in .c files, i.e. it is to be expected that >>> people wouldn't think about this extra Misra requirement. >> >> I can agree to partially adopt the directive: I will add a deviation for C >> files in rules.txt. > > Sorry for the late reply as I was OOO. Please hold on before adding a > deviation for C files. > > In general, I think including .c files is not common behavior, and > should be restricted to special cases. We could say that exactly because > they are special, they follow different rules so we can skip the guards. > Or we could say that they are still at risk of double-inclusion, hence > we should be consistent and protect them too. I think we should discuss > the topic during the next MISRA C meeting. Just one further remark right here: While including a header file a 2nd time stands a fair chance of working (i.e. the compiler not spitting out errors), that would be very unusual for a .c file: Every function or data item it defines would (in the common case) be already defined. Jan
On 06/09/23 00:18, Stefano Stabellini wrote: > On Thu, 31 Aug 2023, Simone Ballarin wrote: >> On 31/08/23 15:05, Jan Beulich wrote: >>> On 31.08.2023 14:54, Simone Ballarin wrote: >>>> On 31/08/23 13:10, Jan Beulich wrote: >>>>> On 31.08.2023 12:08, Simone Ballarin wrote: >>>>>> The danger of multi-inclusion also exists for .c files, why do you >>>>>> want >>>>>> to avoid guards for them? >>>>> >>>>> Counter question: Why only add guards to some of them? (My personal >>>>> answer is "Because it's extra clutter.") >>>> >>>> It's not "some of them", it's exactly the ones used in an #include >>>> directive, so I'm not getting your objection. >>> >>> My point is that by adding guards only for files we presently use in some >>> #include directive, we set us up for introducing new violations as soon >>> as another .c file becomes the subject of an #include.The more that it >>> is unusual to add guards in .c files, i.e. it is to be expected that >>> people wouldn't think about this extra Misra requirement. >>> >>> Jan >> >> I can agree to partially adopt the directive: I will add a deviation for C >> files in rules.txt. > > Sorry for the late reply as I was OOO. Please hold on before adding a > deviation for C files. > > In general, I think including .c files is not common behavior, and > should be restricted to special cases. We could say that exactly because > they are special, they follow different rules so we can skip the guards. > Or we could say that they are still at risk of double-inclusion, hence > we should be consistent and protect them too. I think we should discuss > the topic during the next MISRA C meeting. > Ok, I will drop changes in C files without adding the deviation.
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c index f8177c84c0..614ad71a59 100644 --- a/xen/common/compat/grant_table.c +++ b/xen/common/compat/grant_table.c @@ -3,6 +3,10 @@ * */ + +#ifndef __COMMON_COMPAT_GRANT_TABLE_C__ +#define __COMMON_COMPAT_GRANT_TABLE_C__ + #include <xen/hypercall.h> #include <compat/grant_table.h> @@ -331,6 +335,9 @@ int compat_grant_table_op( return rc; } + +#endif /* __COMMON_COMPAT_GRANT_TABLE_C__ */ + /* * Local variables: * mode: C diff --git a/xen/common/coverage/gcc_4_7.c b/xen/common/coverage/gcc_4_7.c index 25b4a8bcdc..12e4ec8cbb 100644 --- a/xen/common/coverage/gcc_4_7.c +++ b/xen/common/coverage/gcc_4_7.c @@ -14,6 +14,9 @@ * Wei Liu <wei.liu2@citrix.com> */ +#ifndef __COMMON_COVERAGE_GCC_4_7_C__ +#define __COMMON_COVERAGE_GCC_4_7_C__ + #include <xen/string.h> #include "gcov.h" @@ -193,6 +196,8 @@ size_t gcov_info_to_gcda(char *buffer, const struct gcov_info *info) return pos; } +#endif /* __COMMON_COVERAGE_GCC_4_7_C__ */ + /* * Local variables: * mode: C 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 diff --git a/xen/common/multicall.c b/xen/common/multicall.c index 1f0cc4cb26..421bb25b70 100644 --- a/xen/common/multicall.c +++ b/xen/common/multicall.c @@ -2,6 +2,9 @@ * multicall.c */ +#ifndef __COMMON_MULTICALL_C__ +#define __COMMON_MULTICALL_C__ + #include <xen/types.h> #include <xen/lib.h> #include <xen/mm.h> @@ -124,6 +127,8 @@ ret_t do_multicall( __HYPERVISOR_multicall, "hi", call_list, nr_calls-i); } +#endif /* __COMMON_MULTICALL_C__ */ + /* * Local variables: * mode: C
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"). Also C files, if included somewhere, need to comply with the guideline. Mechanical change. Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> --- xen/common/compat/grant_table.c | 7 +++++++ xen/common/coverage/gcc_4_7.c | 5 +++++ xen/common/decompress.h | 5 +++++ xen/common/event_channel.h | 5 +++++ xen/common/multicall.c | 5 +++++ 5 files changed, 27 insertions(+)