| Message ID | 7e4f836606d72769a80299c5451f6f7241471d8a.1734530952.git.alessandro.zucchelli@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] misra: add deviation for MISRA C Rule R11.8. | expand |
On 18.12.2024 15:25, Alessandro Zucchelli wrote: > Rule 11.8 states as following: "A cast shall not remove any `const' or > `volatile' qualification from the type pointed to by a pointer". > > Function `__hvm_copy' in `xen/arch/x86/hvm/hvm.c' is a double-use > function, where the parameter needs to not be const because it can be > set for write or not. As it was decided a new const-only function will > lead to more developer confusion than it's worth, this violation is > addressed by deviating the function. > All cases of casting away const-ness are accompanied with a comment > explaining why it is safe given the other flags passed in; such comment is used > by the deviation in order to match the appropriate function call. > > No functional change. > > Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> > --- > As this patch introduces a deviation for service MC3A2.R11.8, it > depends on the following patch and shall not be applied prior to its > application. > https://lore.kernel.org/xen-devel/cf13be4779f15620e94b99b3b91f9cb040319989.1733826952.git.alessandro.zucchelli@bugseng.com/T/#u This wasn't applicable anymore at the time you posted the patch, I suppose? > --- a/automation/eclair_analysis/ECLAIR/deviations.ecl > +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl > @@ -393,6 +393,12 @@ Fixing this violation would require to increase code complexity and lower readab > -config=MC3R1.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"} > -doc_end > > +-doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a double-use > +function, where the parameter needs to not be const because it can be set for > +write or not" > +-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*__hvm_copy.*HVMCOPY_to_guest doesn't modify.*$)))"} This is probably good enough for now, yet still: It constrains re-formatting that we may want to do on such function calls. Personally I'd consider it entirely unexpected if I ended up (re)introducing a violation just by re- formatting one of those function calls to return __hvm_copy( (void *)buf /* HVMCOPY_to_guest doesn't modify */, addr, size, current, HVMCOPY_to_guest | HVMCOPY_linear, PFEC_page_present | PFEC_write_access | pfec, pfinfo); yet aiui the pattern above would have this effect (I don't think .* matches newlines; instead I expect such regex-es to be applied to individual lines only). Thoughts anyone? Jan
On 2024-12-19 09:49, Jan Beulich wrote: > On 18.12.2024 15:25, Alessandro Zucchelli wrote: >> Rule 11.8 states as following: "A cast shall not remove any `const' or >> `volatile' qualification from the type pointed to by a pointer". >> >> Function `__hvm_copy' in `xen/arch/x86/hvm/hvm.c' is a double-use >> function, where the parameter needs to not be const because it can be >> set for write or not. As it was decided a new const-only function will >> lead to more developer confusion than it's worth, this violation is >> addressed by deviating the function. >> All cases of casting away const-ness are accompanied with a comment >> explaining why it is safe given the other flags passed in; such >> comment is used >> by the deviation in order to match the appropriate function call. >> >> No functional change. >> >> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> >> --- >> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >> @@ -393,6 +393,12 @@ Fixing this violation would require to increase >> code complexity and lower readab >> >> -config=MC3R1.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"} >> -doc_end >> >> +-doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a >> double-use >> +function, where the parameter needs to not be const because it can be >> set for >> +write or not" >> +-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*__hvm_copy.*HVMCOPY_to_guest >> doesn't modify.*$)))"} > > This is probably good enough for now, yet still: It constrains > re-formatting > that we may want to do on such function calls. Personally I'd consider > it > entirely unexpected if I ended up (re)introducing a violation just by > re- > formatting one of those function calls to > > return __hvm_copy( > (void *)buf /* HVMCOPY_to_guest doesn't modify */, > addr, size, current, HVMCOPY_to_guest | HVMCOPY_linear, > PFEC_page_present | PFEC_write_access | pfec, pfinfo); > > yet aiui the pattern above would have this effect (I don't think .* > matches > newlines; instead I expect such regex-es to be applied to individual > lines > only). Thoughts anyone? > Hi Jan, we can simply drop the "__hvm_copy" part from the regex. The regex can be made multiline, or alternatively we can apply the search to a range of lines. By default it searches on the same location mentioned by the report, which in this case is the line containing __hvm_copy (range defaults to 0..0). However I would leave it either as is or without the __hvm_copy prefix.
On 19.12.2024 09:58, Nicola Vetrini wrote: > On 2024-12-19 09:49, Jan Beulich wrote: >> On 18.12.2024 15:25, Alessandro Zucchelli wrote: >>> Rule 11.8 states as following: "A cast shall not remove any `const' or >>> `volatile' qualification from the type pointed to by a pointer". >>> >>> Function `__hvm_copy' in `xen/arch/x86/hvm/hvm.c' is a double-use >>> function, where the parameter needs to not be const because it can be >>> set for write or not. As it was decided a new const-only function will >>> lead to more developer confusion than it's worth, this violation is >>> addressed by deviating the function. >>> All cases of casting away const-ness are accompanied with a comment >>> explaining why it is safe given the other flags passed in; such >>> comment is used >>> by the deviation in order to match the appropriate function call. >>> >>> No functional change. >>> >>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> >>> --- > >>> --- a/automation/eclair_analysis/ECLAIR/deviations.ecl >>> +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl >>> @@ -393,6 +393,12 @@ Fixing this violation would require to increase >>> code complexity and lower readab >>> >>> -config=MC3R1.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"} >>> -doc_end >>> >>> +-doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a >>> double-use >>> +function, where the parameter needs to not be const because it can be >>> set for >>> +write or not" >>> +-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*__hvm_copy.*HVMCOPY_to_guest >>> doesn't modify.*$)))"} >> >> This is probably good enough for now, yet still: It constrains >> re-formatting >> that we may want to do on such function calls. Personally I'd consider >> it >> entirely unexpected if I ended up (re)introducing a violation just by >> re- >> formatting one of those function calls to >> >> return __hvm_copy( >> (void *)buf /* HVMCOPY_to_guest doesn't modify */, >> addr, size, current, HVMCOPY_to_guest | HVMCOPY_linear, >> PFEC_page_present | PFEC_write_access | pfec, pfinfo); >> >> yet aiui the pattern above would have this effect (I don't think .* >> matches >> newlines; instead I expect such regex-es to be applied to individual >> lines >> only). Thoughts anyone? > > we can simply drop the "__hvm_copy" part from the regex. The regex can > be made multiline, or alternatively we can apply the search to a range > of lines. By default it searches on the same location mentioned by the > report, which in this case is the line containing __hvm_copy (range > defaults to 0..0). However I would leave it either as is or without the > __hvm_copy prefix. Omitting the __hvm_copy part would again widen it too much for my taste. Jan
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl index 2f58f29203..c9d06b44f4 100644 --- a/automation/eclair_analysis/ECLAIR/deviations.ecl +++ b/automation/eclair_analysis/ECLAIR/deviations.ecl @@ -393,6 +393,12 @@ Fixing this violation would require to increase code complexity and lower readab -config=MC3R1.R11.8,reports+={safe,"any_area(any_loc(any_exp(macro(^container_of$))))"} -doc_end +-doc_begin="Function __hvm_copy in xen/arch/x86/hvm/hvm.c is a double-use +function, where the parameter needs to not be const because it can be set for +write or not" +-config=MC3A2.R11.8,reports+={safe,"any_area(any_loc(text(^.*__hvm_copy.*HVMCOPY_to_guest doesn't modify.*$)))"} +-doc_end + -doc_begin="This construct is used to check if the type is scalar, and for this purpose the use of 0 as a null pointer constant is deliberate." -config=MC3R1.R11.9,reports+={deliberate, "any_area(any_loc(any_exp(macro(^__ACCESS_ONCE$))))" }
Rule 11.8 states as following: "A cast shall not remove any `const' or `volatile' qualification from the type pointed to by a pointer". Function `__hvm_copy' in `xen/arch/x86/hvm/hvm.c' is a double-use function, where the parameter needs to not be const because it can be set for write or not. As it was decided a new const-only function will lead to more developer confusion than it's worth, this violation is addressed by deviating the function. All cases of casting away const-ness are accompanied with a comment explaining why it is safe given the other flags passed in; such comment is used by the deviation in order to match the appropriate function call. No functional change. Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> --- As this patch introduces a deviation for service MC3A2.R11.8, it depends on the following patch and shall not be applied prior to its application. https://lore.kernel.org/xen-devel/cf13be4779f15620e94b99b3b91f9cb040319989.1733826952.git.alessandro.zucchelli@bugseng.com/T/#u Changes from V1: The deviation has been refined to specify that every instance of casting away const-ness is accompanied by a comment explaining why it is safe. This comment is a requirement that has been incorporated into the text defining the deviation. --- automation/eclair_analysis/ECLAIR/deviations.ecl | 6 ++++++ 1 file changed, 6 insertions(+)