| Message ID | 1fdfec12fd2207c294f50d01d8ec32f890b915d7.1710145041.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address violation of MISRA C:2012 Directive 4.10 | expand |
On 11.03.2024 09:59, Simone Ballarin wrote: > 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> > > --- > Changes in v3: > - fix inconsistent deviation ID > 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 + > 3 files changed, 10 insertions(+) What about asm-generic/hypercall.h? > --- a/xen/arch/arm/include/asm/hypercall.h > +++ b/xen/arch/arm/include/asm/hypercall.h > @@ -1,3 +1,4 @@ > +/* SAF-5-safe direct inclusion guard before */ > #ifndef __XEN_HYPERCALL_H__ > #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" > #endif > --- 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-5-safe direct inclusion guard before */ > #ifndef __XEN_HYPERCALL_H__ > #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" > #endif Iirc it was said that this way checking for correct guards is suppressed altogether in Eclair, which is not what we want. Can you clarify this, please? Jan
On 11/03/24 11:08, Jan Beulich wrote: > On 11.03.2024 09:59, Simone Ballarin wrote: >> 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> >> >> --- >> Changes in v3: >> - fix inconsistent deviation ID >> 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 + >> 3 files changed, 10 insertions(+) > > What about asm-generic/hypercall.h? Apparently the file is not part of the analysed build. > >> --- a/xen/arch/arm/include/asm/hypercall.h >> +++ b/xen/arch/arm/include/asm/hypercall.h >> @@ -1,3 +1,4 @@ >> +/* SAF-5-safe direct inclusion guard before */ >> #ifndef __XEN_HYPERCALL_H__ >> #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" >> #endif >> --- 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-5-safe direct inclusion guard before */ >> #ifndef __XEN_HYPERCALL_H__ >> #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" >> #endif > > Iirc it was said that this way checking for correct guards is suppressed > altogether in Eclair, which is not what we want. Can you clarify this, > please? > My first change was moving this check inside the guard. You commented my patch saying that this would be an error because someone can include it directly if it has already been included indirectly. I replied telling that this was the case also before the change. You agreed with me, and we decided that the correct thing would be fixing the check and not apply my temporary change to address the finding. Considering that the code should be amended, a SAF deviation seems to me the most appropriate way for suppressing these findings. > Jan >
On 11.03.2024 13:00, Simone Ballarin wrote: > On 11/03/24 11:08, Jan Beulich wrote: >> On 11.03.2024 09:59, Simone Ballarin wrote: >>> --- a/xen/arch/arm/include/asm/hypercall.h >>> +++ b/xen/arch/arm/include/asm/hypercall.h >>> @@ -1,3 +1,4 @@ >>> +/* SAF-5-safe direct inclusion guard before */ >>> #ifndef __XEN_HYPERCALL_H__ >>> #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" >>> #endif >>> --- 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-5-safe direct inclusion guard before */ >>> #ifndef __XEN_HYPERCALL_H__ >>> #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" >>> #endif >> >> Iirc it was said that this way checking for correct guards is suppressed >> altogether in Eclair, which is not what we want. Can you clarify this, >> please? >> > > My first change was moving this check inside the guard. > You commented my patch saying that this would be an error because someone can > include it directly if it has already been included indirectly. > I replied telling that this was the case also before the change. > You agreed with me, and we decided that the correct thing would be fixing the > check and not apply my temporary change to address the finding. > > Considering that the code should be amended, a SAF deviation seems to me > the most appropriate way for suppressing these findings. Since I don't feel your reply addresses my question, asking differently: With your change in place, will failure to have proper guards (later) in these headers still be reported by Eclair? Jan
On 11/03/24 14:56, Jan Beulich wrote: > On 11.03.2024 13:00, Simone Ballarin wrote: >> On 11/03/24 11:08, Jan Beulich wrote: >>> On 11.03.2024 09:59, Simone Ballarin wrote: >>>> --- a/xen/arch/arm/include/asm/hypercall.h >>>> +++ b/xen/arch/arm/include/asm/hypercall.h >>>> @@ -1,3 +1,4 @@ >>>> +/* SAF-5-safe direct inclusion guard before */ >>>> #ifndef __XEN_HYPERCALL_H__ >>>> #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" >>>> #endif >>>> --- 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-5-safe direct inclusion guard before */ >>>> #ifndef __XEN_HYPERCALL_H__ >>>> #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" >>>> #endif >>> >>> Iirc it was said that this way checking for correct guards is suppressed >>> altogether in Eclair, which is not what we want. Can you clarify this, >>> please? >>> >> >> My first change was moving this check inside the guard. >> You commented my patch saying that this would be an error because someone can >> include it directly if it has already been included indirectly. >> I replied telling that this was the case also before the change. >> You agreed with me, and we decided that the correct thing would be fixing the >> check and not apply my temporary change to address the finding. >> >> Considering that the code should be amended, a SAF deviation seems to me >> the most appropriate way for suppressing these findings. > > Since I don't feel your reply addresses my question, asking differently: With > your change in place, will failure to have proper guards (later) in these > headers still be reported by Eclair? > > Jan > No, if you put something between the check and the guard, no violation will be reported.
On Mon, 11 Mar 2024, Simone Ballarin wrote: > On 11/03/24 14:56, Jan Beulich wrote: > > On 11.03.2024 13:00, Simone Ballarin wrote: > > > On 11/03/24 11:08, Jan Beulich wrote: > > > > On 11.03.2024 09:59, Simone Ballarin wrote: > > > > > --- a/xen/arch/arm/include/asm/hypercall.h > > > > > +++ b/xen/arch/arm/include/asm/hypercall.h > > > > > @@ -1,3 +1,4 @@ > > > > > +/* SAF-5-safe direct inclusion guard before */ > > > > > #ifndef __XEN_HYPERCALL_H__ > > > > > #error "asm/hypercall.h should not be included directly - include > > > > > xen/hypercall.h instead" > > > > > #endif > > > > > --- 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-5-safe direct inclusion guard before */ > > > > > #ifndef __XEN_HYPERCALL_H__ > > > > > #error "asm/hypercall.h should not be included directly - include > > > > > xen/hypercall.h instead" > > > > > #endif > > > > > > > > Iirc it was said that this way checking for correct guards is suppressed > > > > altogether in Eclair, which is not what we want. Can you clarify this, > > > > please? > > > > > > > > > > My first change was moving this check inside the guard. > > > You commented my patch saying that this would be an error because someone > > > can > > > include it directly if it has already been included indirectly. > > > I replied telling that this was the case also before the change. > > > You agreed with me, and we decided that the correct thing would be fixing > > > the > > > check and not apply my temporary change to address the finding. > > > > > > Considering that the code should be amended, a SAF deviation seems to me > > > the most appropriate way for suppressing these findings. > > > > Since I don't feel your reply addresses my question, asking differently: > > With > > your change in place, will failure to have proper guards (later) in these > > headers still be reported by Eclair? > > > > Jan > > > > No, if you put something between the check and the guard, > no violation will be reported. From this email exchange I cannot under if Jan is OK with this patch or not.
On 14.03.2024 23:59, Stefano Stabellini wrote: > On Mon, 11 Mar 2024, Simone Ballarin wrote: >> On 11/03/24 14:56, Jan Beulich wrote: >>> On 11.03.2024 13:00, Simone Ballarin wrote: >>>> On 11/03/24 11:08, Jan Beulich wrote: >>>>> On 11.03.2024 09:59, Simone Ballarin wrote: >>>>>> --- a/xen/arch/arm/include/asm/hypercall.h >>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h >>>>>> @@ -1,3 +1,4 @@ >>>>>> +/* SAF-5-safe direct inclusion guard before */ >>>>>> #ifndef __XEN_HYPERCALL_H__ >>>>>> #error "asm/hypercall.h should not be included directly - include >>>>>> xen/hypercall.h instead" >>>>>> #endif >>>>>> --- 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-5-safe direct inclusion guard before */ >>>>>> #ifndef __XEN_HYPERCALL_H__ >>>>>> #error "asm/hypercall.h should not be included directly - include >>>>>> xen/hypercall.h instead" >>>>>> #endif >>>>> >>>>> Iirc it was said that this way checking for correct guards is suppressed >>>>> altogether in Eclair, which is not what we want. Can you clarify this, >>>>> please? >>>>> >>>> >>>> My first change was moving this check inside the guard. >>>> You commented my patch saying that this would be an error because someone >>>> can >>>> include it directly if it has already been included indirectly. >>>> I replied telling that this was the case also before the change. >>>> You agreed with me, and we decided that the correct thing would be fixing >>>> the >>>> check and not apply my temporary change to address the finding. >>>> >>>> Considering that the code should be amended, a SAF deviation seems to me >>>> the most appropriate way for suppressing these findings. >>> >>> Since I don't feel your reply addresses my question, asking differently: >>> With >>> your change in place, will failure to have proper guards (later) in these >>> headers still be reported by Eclair? >> >> No, if you put something between the check and the guard, >> no violation will be reported. > > From this email exchange I cannot under if Jan is OK with this patch or > not. Whether I'm okay(ish) with the patch here depends on our position towards the lost checking in Eclair mentioned above. To me it still looks relevant that checking for a guard occurs, even if that isn't first in a file for some specific reason. Jan
On Fri, 15 Mar 2024, Jan Beulich wrote: > On 14.03.2024 23:59, Stefano Stabellini wrote: > > On Mon, 11 Mar 2024, Simone Ballarin wrote: > >> On 11/03/24 14:56, Jan Beulich wrote: > >>> On 11.03.2024 13:00, Simone Ballarin wrote: > >>>> On 11/03/24 11:08, Jan Beulich wrote: > >>>>> On 11.03.2024 09:59, Simone Ballarin wrote: > >>>>>> --- a/xen/arch/arm/include/asm/hypercall.h > >>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h > >>>>>> @@ -1,3 +1,4 @@ > >>>>>> +/* SAF-5-safe direct inclusion guard before */ > >>>>>> #ifndef __XEN_HYPERCALL_H__ > >>>>>> #error "asm/hypercall.h should not be included directly - include > >>>>>> xen/hypercall.h instead" > >>>>>> #endif > >>>>>> --- 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-5-safe direct inclusion guard before */ > >>>>>> #ifndef __XEN_HYPERCALL_H__ > >>>>>> #error "asm/hypercall.h should not be included directly - include > >>>>>> xen/hypercall.h instead" > >>>>>> #endif > >>>>> > >>>>> Iirc it was said that this way checking for correct guards is suppressed > >>>>> altogether in Eclair, which is not what we want. Can you clarify this, > >>>>> please? > >>>>> > >>>> > >>>> My first change was moving this check inside the guard. > >>>> You commented my patch saying that this would be an error because someone > >>>> can > >>>> include it directly if it has already been included indirectly. > >>>> I replied telling that this was the case also before the change. > >>>> You agreed with me, and we decided that the correct thing would be fixing > >>>> the > >>>> check and not apply my temporary change to address the finding. > >>>> > >>>> Considering that the code should be amended, a SAF deviation seems to me > >>>> the most appropriate way for suppressing these findings. > >>> > >>> Since I don't feel your reply addresses my question, asking differently: > >>> With > >>> your change in place, will failure to have proper guards (later) in these > >>> headers still be reported by Eclair? > >> > >> No, if you put something between the check and the guard, > >> no violation will be reported. > > > > From this email exchange I cannot under if Jan is OK with this patch or > > not. > > Whether I'm okay(ish) with the patch here depends on our position towards > the lost checking in Eclair mentioned above. To me it still looks relevant > that checking for a guard occurs, even if that isn't first in a file for > some specific reason. More checking is better than less checking, but if we cannot find a simple and actionable way forward on this violation, deviating it is still a big improvement because it allows us to enable the ECLAIR Dir 4.10 checks in xen.git overall (which again goes back to more checking is better than less checking). Do you have a simple alternative suggestion? If not, this is still an improvement.
On 16.03.2024 01:43, Stefano Stabellini wrote: > On Fri, 15 Mar 2024, Jan Beulich wrote: >> On 14.03.2024 23:59, Stefano Stabellini wrote: >>> On Mon, 11 Mar 2024, Simone Ballarin wrote: >>>> On 11/03/24 14:56, Jan Beulich wrote: >>>>> On 11.03.2024 13:00, Simone Ballarin wrote: >>>>>> On 11/03/24 11:08, Jan Beulich wrote: >>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote: >>>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h >>>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h >>>>>>>> @@ -1,3 +1,4 @@ >>>>>>>> +/* SAF-5-safe direct inclusion guard before */ >>>>>>>> #ifndef __XEN_HYPERCALL_H__ >>>>>>>> #error "asm/hypercall.h should not be included directly - include >>>>>>>> xen/hypercall.h instead" >>>>>>>> #endif >>>>>>>> --- 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-5-safe direct inclusion guard before */ >>>>>>>> #ifndef __XEN_HYPERCALL_H__ >>>>>>>> #error "asm/hypercall.h should not be included directly - include >>>>>>>> xen/hypercall.h instead" >>>>>>>> #endif >>>>>>> >>>>>>> Iirc it was said that this way checking for correct guards is suppressed >>>>>>> altogether in Eclair, which is not what we want. Can you clarify this, >>>>>>> please? >>>>>>> >>>>>> >>>>>> My first change was moving this check inside the guard. >>>>>> You commented my patch saying that this would be an error because someone >>>>>> can >>>>>> include it directly if it has already been included indirectly. >>>>>> I replied telling that this was the case also before the change. >>>>>> You agreed with me, and we decided that the correct thing would be fixing >>>>>> the >>>>>> check and not apply my temporary change to address the finding. >>>>>> >>>>>> Considering that the code should be amended, a SAF deviation seems to me >>>>>> the most appropriate way for suppressing these findings. >>>>> >>>>> Since I don't feel your reply addresses my question, asking differently: >>>>> With >>>>> your change in place, will failure to have proper guards (later) in these >>>>> headers still be reported by Eclair? >>>> >>>> No, if you put something between the check and the guard, >>>> no violation will be reported. >>> >>> From this email exchange I cannot under if Jan is OK with this patch or >>> not. >> >> Whether I'm okay(ish) with the patch here depends on our position towards >> the lost checking in Eclair mentioned above. To me it still looks relevant >> that checking for a guard occurs, even if that isn't first in a file for >> some specific reason. > > More checking is better than less checking, but if we cannot find a > simple and actionable way forward on this violation, deviating it is > still a big improvement because it allows us to enable the ECLAIR Dir > 4.10 checks in xen.git overall (which again goes back to more checking > is better than less checking). You have a point here. I think though that at the very least the lost checking opportunity wants calling out quite explicitly. > Do you have a simple alternative suggestion? If not, this is still an > improvement. I don't know the inner workings of Eclair. Without that I'm afraid I'm not in a position to make alternative suggestions. Jan
On Mon, 18 Mar 2024, Jan Beulich wrote: > On 16.03.2024 01:43, Stefano Stabellini wrote: > > On Fri, 15 Mar 2024, Jan Beulich wrote: > >> On 14.03.2024 23:59, Stefano Stabellini wrote: > >>> On Mon, 11 Mar 2024, Simone Ballarin wrote: > >>>> On 11/03/24 14:56, Jan Beulich wrote: > >>>>> On 11.03.2024 13:00, Simone Ballarin wrote: > >>>>>> On 11/03/24 11:08, Jan Beulich wrote: > >>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote: > >>>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h > >>>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h > >>>>>>>> @@ -1,3 +1,4 @@ > >>>>>>>> +/* SAF-5-safe direct inclusion guard before */ > >>>>>>>> #ifndef __XEN_HYPERCALL_H__ > >>>>>>>> #error "asm/hypercall.h should not be included directly - include > >>>>>>>> xen/hypercall.h instead" > >>>>>>>> #endif > >>>>>>>> --- 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-5-safe direct inclusion guard before */ > >>>>>>>> #ifndef __XEN_HYPERCALL_H__ > >>>>>>>> #error "asm/hypercall.h should not be included directly - include > >>>>>>>> xen/hypercall.h instead" > >>>>>>>> #endif > >>>>>>> > >>>>>>> Iirc it was said that this way checking for correct guards is suppressed > >>>>>>> altogether in Eclair, which is not what we want. Can you clarify this, > >>>>>>> please? > >>>>>>> > >>>>>> > >>>>>> My first change was moving this check inside the guard. > >>>>>> You commented my patch saying that this would be an error because someone > >>>>>> can > >>>>>> include it directly if it has already been included indirectly. > >>>>>> I replied telling that this was the case also before the change. > >>>>>> You agreed with me, and we decided that the correct thing would be fixing > >>>>>> the > >>>>>> check and not apply my temporary change to address the finding. > >>>>>> > >>>>>> Considering that the code should be amended, a SAF deviation seems to me > >>>>>> the most appropriate way for suppressing these findings. > >>>>> > >>>>> Since I don't feel your reply addresses my question, asking differently: > >>>>> With > >>>>> your change in place, will failure to have proper guards (later) in these > >>>>> headers still be reported by Eclair? > >>>> > >>>> No, if you put something between the check and the guard, > >>>> no violation will be reported. > >>> > >>> From this email exchange I cannot under if Jan is OK with this patch or > >>> not. > >> > >> Whether I'm okay(ish) with the patch here depends on our position towards > >> the lost checking in Eclair mentioned above. To me it still looks relevant > >> that checking for a guard occurs, even if that isn't first in a file for > >> some specific reason. > > > > More checking is better than less checking, but if we cannot find a > > simple and actionable way forward on this violation, deviating it is > > still a big improvement because it allows us to enable the ECLAIR Dir > > 4.10 checks in xen.git overall (which again goes back to more checking > > is better than less checking). > > You have a point here. I think though that at the very least the lost > checking opportunity wants calling out quite explicitly. All right, then maybe this patch can go in with a clarification in the commit message? Something like: Note that with SAF-5-safe in place, failures to have proper guards later in the header files will not be reported > > Do you have a simple alternative suggestion? If not, this is still an > > improvement. > > I don't know the inner workings of Eclair. Without that I'm afraid I'm not > in a position to make alternative suggestions.
On 19.03.2024 04:34, Stefano Stabellini wrote: > On Mon, 18 Mar 2024, Jan Beulich wrote: >> On 16.03.2024 01:43, Stefano Stabellini wrote: >>> On Fri, 15 Mar 2024, Jan Beulich wrote: >>>> On 14.03.2024 23:59, Stefano Stabellini wrote: >>>>> On Mon, 11 Mar 2024, Simone Ballarin wrote: >>>>>> On 11/03/24 14:56, Jan Beulich wrote: >>>>>>> On 11.03.2024 13:00, Simone Ballarin wrote: >>>>>>>> On 11/03/24 11:08, Jan Beulich wrote: >>>>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote: >>>>>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h >>>>>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h >>>>>>>>>> @@ -1,3 +1,4 @@ >>>>>>>>>> +/* SAF-5-safe direct inclusion guard before */ >>>>>>>>>> #ifndef __XEN_HYPERCALL_H__ >>>>>>>>>> #error "asm/hypercall.h should not be included directly - include >>>>>>>>>> xen/hypercall.h instead" >>>>>>>>>> #endif >>>>>>>>>> --- 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-5-safe direct inclusion guard before */ >>>>>>>>>> #ifndef __XEN_HYPERCALL_H__ >>>>>>>>>> #error "asm/hypercall.h should not be included directly - include >>>>>>>>>> xen/hypercall.h instead" >>>>>>>>>> #endif >>>>>>>>> >>>>>>>>> Iirc it was said that this way checking for correct guards is suppressed >>>>>>>>> altogether in Eclair, which is not what we want. Can you clarify this, >>>>>>>>> please? >>>>>>>>> >>>>>>>> >>>>>>>> My first change was moving this check inside the guard. >>>>>>>> You commented my patch saying that this would be an error because someone >>>>>>>> can >>>>>>>> include it directly if it has already been included indirectly. >>>>>>>> I replied telling that this was the case also before the change. >>>>>>>> You agreed with me, and we decided that the correct thing would be fixing >>>>>>>> the >>>>>>>> check and not apply my temporary change to address the finding. >>>>>>>> >>>>>>>> Considering that the code should be amended, a SAF deviation seems to me >>>>>>>> the most appropriate way for suppressing these findings. >>>>>>> >>>>>>> Since I don't feel your reply addresses my question, asking differently: >>>>>>> With >>>>>>> your change in place, will failure to have proper guards (later) in these >>>>>>> headers still be reported by Eclair? >>>>>> >>>>>> No, if you put something between the check and the guard, >>>>>> no violation will be reported. >>>>> >>>>> From this email exchange I cannot under if Jan is OK with this patch or >>>>> not. >>>> >>>> Whether I'm okay(ish) with the patch here depends on our position towards >>>> the lost checking in Eclair mentioned above. To me it still looks relevant >>>> that checking for a guard occurs, even if that isn't first in a file for >>>> some specific reason. >>> >>> More checking is better than less checking, but if we cannot find a >>> simple and actionable way forward on this violation, deviating it is >>> still a big improvement because it allows us to enable the ECLAIR Dir >>> 4.10 checks in xen.git overall (which again goes back to more checking >>> is better than less checking). >> >> You have a point here. I think though that at the very least the lost >> checking opportunity wants calling out quite explicitly. > > All right, then maybe this patch can go in with a clarification in the > commit message? > > Something like: > > Note that with SAF-5-safe in place, failures to have proper guards later > in the header files will not be reported That would be okay with me. Jan
On 2024-03-19 08:45, Jan Beulich wrote: > On 19.03.2024 04:34, Stefano Stabellini wrote: >> On Mon, 18 Mar 2024, Jan Beulich wrote: >>> On 16.03.2024 01:43, Stefano Stabellini wrote: >>>> On Fri, 15 Mar 2024, Jan Beulich wrote: >>>>> On 14.03.2024 23:59, Stefano Stabellini wrote: >>>>>> On Mon, 11 Mar 2024, Simone Ballarin wrote: >>>>>>> On 11/03/24 14:56, Jan Beulich wrote: >>>>>>>> On 11.03.2024 13:00, Simone Ballarin wrote: >>>>>>>>> On 11/03/24 11:08, Jan Beulich wrote: >>>>>>>>>> On 11.03.2024 09:59, Simone Ballarin wrote: >>>>>>>>>>> --- a/xen/arch/arm/include/asm/hypercall.h >>>>>>>>>>> +++ b/xen/arch/arm/include/asm/hypercall.h >>>>>>>>>>> @@ -1,3 +1,4 @@ >>>>>>>>>>> +/* SAF-5-safe direct inclusion guard before */ >>>>>>>>>>> #ifndef __XEN_HYPERCALL_H__ >>>>>>>>>>> #error "asm/hypercall.h should not be included directly - >>>>>>>>>>> include >>>>>>>>>>> xen/hypercall.h instead" >>>>>>>>>>> #endif >>>>>>>>>>> --- 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-5-safe direct inclusion guard before */ >>>>>>>>>>> #ifndef __XEN_HYPERCALL_H__ >>>>>>>>>>> #error "asm/hypercall.h should not be included directly - >>>>>>>>>>> include >>>>>>>>>>> xen/hypercall.h instead" >>>>>>>>>>> #endif >>>>>>>>>> >>>>>>>>>> Iirc it was said that this way checking for correct guards is >>>>>>>>>> suppressed >>>>>>>>>> altogether in Eclair, which is not what we want. Can you >>>>>>>>>> clarify this, >>>>>>>>>> please? >>>>>>>>>> >>>>>>>>> >>>>>>>>> My first change was moving this check inside the guard. >>>>>>>>> You commented my patch saying that this would be an error >>>>>>>>> because someone >>>>>>>>> can >>>>>>>>> include it directly if it has already been included indirectly. >>>>>>>>> I replied telling that this was the case also before the >>>>>>>>> change. >>>>>>>>> You agreed with me, and we decided that the correct thing would >>>>>>>>> be fixing >>>>>>>>> the >>>>>>>>> check and not apply my temporary change to address the finding. >>>>>>>>> >>>>>>>>> Considering that the code should be amended, a SAF deviation >>>>>>>>> seems to me >>>>>>>>> the most appropriate way for suppressing these findings. >>>>>>>> >>>>>>>> Since I don't feel your reply addresses my question, asking >>>>>>>> differently: >>>>>>>> With >>>>>>>> your change in place, will failure to have proper guards (later) >>>>>>>> in these >>>>>>>> headers still be reported by Eclair? >>>>>>> >>>>>>> No, if you put something between the check and the guard, >>>>>>> no violation will be reported. >>>>>> >>>>>> From this email exchange I cannot under if Jan is OK with this >>>>>> patch or >>>>>> not. >>>>> >>>>> Whether I'm okay(ish) with the patch here depends on our position >>>>> towards >>>>> the lost checking in Eclair mentioned above. To me it still looks >>>>> relevant >>>>> that checking for a guard occurs, even if that isn't first in a >>>>> file for >>>>> some specific reason. >>>> >>>> More checking is better than less checking, but if we cannot find a >>>> simple and actionable way forward on this violation, deviating it is >>>> still a big improvement because it allows us to enable the ECLAIR >>>> Dir >>>> 4.10 checks in xen.git overall (which again goes back to more >>>> checking >>>> is better than less checking). >>> >>> You have a point here. I think though that at the very least the lost >>> checking opportunity wants calling out quite explicitly. >> >> All right, then maybe this patch can go in with a clarification in the >> commit message? >> >> Something like: >> >> Note that with SAF-5-safe in place, failures to have proper guards >> later >> in the header files will not be reported > > That would be okay with me. > Coming back to this thread. Yes, I'll update the message to reflect this change.
diff --git a/docs/misra/safe.json b/docs/misra/safe.json index d2489379a7..363c9e21b0 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: direct inclusion guard before", + "text": "Headers with just the direct inclusion guard before the inclusion guard are safe." + }, + { + "id": "SAF-6-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..b754475cd0 100644 --- a/xen/arch/arm/include/asm/hypercall.h +++ b/xen/arch/arm/include/asm/hypercall.h @@ -1,3 +1,4 @@ +/* SAF-5-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..1a51ae02bf 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-5-safe direct inclusion guard before */ #ifndef __XEN_HYPERCALL_H__ #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead" #endif
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> --- Changes in v3: - fix inconsistent deviation ID 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 + 3 files changed, 10 insertions(+)