[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN PATCH v2] x86: p2m-pod: address violation of MISRA C Rule 2.1



On Tue, 10 Sep 2024, Jan Beulich wrote:
> On 10.09.2024 12:17, Nicola Vetrini wrote:
> > On 2024-09-10 12:03, Jan Beulich wrote:
> >> On 10.09.2024 11:53, Nicola Vetrini wrote:
> >>> On 2024-09-10 11:08, Jan Beulich wrote:
> >>>> On 10.09.2024 10:56, Nicola Vetrini wrote:
> >>>>> On 2024-07-01 10:36, Jan Beulich wrote:
> >>>>>> On 28.06.2024 08:30, Nicola Vetrini wrote:
> >>>>>> This being about unreachable code, why are the domain_crash() not 
> >>>>>> the
> >>>>>> crucial points of "unreachability"? And even if they weren't there,
> >>>>>> why
> >>>>>> wouldn't it be the goto or ...
> >>>>>>
> >>>>>>> --- a/xen/arch/x86/mm/p2m-pod.c
> >>>>>>> +++ b/xen/arch/x86/mm/p2m-pod.c
> >>>>>>> @@ -1040,6 +1040,7 @@ out_unmap:
> >>>>>>>       * Something went wrong, probably crashing the domain.  Unmap
> >>>>>>>       * everything and return.
> >>>>>>>       */
> >>>>>>> +    /* SAF-8-safe Rule 2.1: defensive programming */
> >>>>>>>      for ( i = 0; i < count; i++ )
> >>>>>>>          if ( map[i] )
> >>>>>>>              unmap_domain_page(map[i]);
> >>>>>>
> >>>>>> ... the label (just out of context) where the comment needs to go?
> >>>>>
> >>>>> Because of the way this rule's configuration work, deviations are
> >>>>> placed
> >>>>> on the construct that ends up being the target of the 
> >>>>> unreachability,
> >>>>
> >>>> What's "target" here? What if this loop was removed from the 
> >>>> function?
> >>>> Then both the label and the domain_crash() invocations would still be
> >>>> unreachable in debug builds. Are you telling me that this then 
> >>>> wouldn't
> >>>> be diagnosed by Eclair? Or that it would then consider the closing
> >>>> figure brace of the function "the target of the unreachability"?
> >>>
> >>> Exactly, the end brace is a target to which the "function end" 
> >>> construct
> >>> is associated.
> >>> It would be kind of strange, though: why not just doing 
> >>> "domain_crash();
> >>> return;" in that case?
> >>
> >> Sure, the question was theoretical. Now if "return" was used directly
> >> there, what would then be the "target"? IOW - the more abstract 
> >> question
> >> of my earlier reply still wasn't answered.
> >>
> > 
> > The return statement in
> > 
> > ...
> > domain_crash();
> > return;
> > <~~~~~>
> > 
> > Whichever statement is found to be unreachable in the current 
> > preprocessed code.
> 
> Yet then again: Why is it the return statement and not the function call
> one (really, it being a macro invocation: the do/while one that the macro
> expands to)? That's the first thing that won't be reached.

Hi Jan,

Are you trying to get clarity on the specific locations where the SAF
deviations could be placed for the sake of understanding how the
deviation system work?

Or are you asking for the SAF comment to be moved elsewhere because you
don't like the SAF comment after the out_unmap macro?


I think that the position Nicola has used is better than any of the
alternatives. It is clear and immediately obvious when you read it in
context (I admit that looking at the patch alone, without applying it,
it is a bit puzzling).

But I guess you probably also prefer a single SAF comment after the
out_unmap label rather than multiple SAF comment after every single
domain_crash ?  I don't think this patch can be improved.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.