Re: [XEN PATCH 2/2] xen/cpu: address MISRA C Rule 17.7

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 27/02/2024 07:28, Jan Beulich wrote:
> On 27.02.2024 01:26, Stefano Stabellini wrote:
> > On Mon, 26 Feb 2024, Jan Beulich wrote:
> > > On 23.02.2024 10:35, Nicola Vetrini wrote:
> > > > Refactor cpu_notifier_call_chain into two functions:
> > > > - the variant that is allowed to fail loses the nofail flag
> > > > - the variant that shouldn't fail is encapsulated in a call
> > > >    to the failing variant, with an additional check.
> > > >
> > > > This prevents uses of the function that are not supposed to
> > > > fail from ignoring the return value, thus violating Rule 17.7:
> > > > "The value returned by a function having non-void return type shall
> > > > be used".
> > > >
> > > > No functional change.
> > > >
> > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> > > I'm afraid I disagree with this kind of bifurcation. No matter what
> > > Misra thinks or says, it is normal for return values of functions to
> > > not always be relevant to check.
> > Hi Jan, I disagree.
> >
> > Regardless of MISRA, I really think return values need to be checked.
> > Moreover, we decided as a group to honor MISRA Rule 17.7, which requires
> > return values to be checked. This patch is a good step forward.
> Yet splitting functions isn't the only way to deal with Misra's
> requirements, I suppose. After all there are functions where the
> return value is purely courtesy for perhaps just one of its callers.
You are right that we have some places where one caller care about the 
return value. But the problem is how do you tell whether the return was 
ignored on purpose or not?
We had at least one XSA because the return value of a function was not 
checked (see XSA-222). We also had plenty of smaller patches to check 
returns.
So far, we added __must_check when we believed return values should be 
checked. But usually at the point we notice, this is far too late.
To me the goal should be that we enforce __must_check everywhere. We are 
probably going to detect places where we forgot to check the return. For 
thoses that are on purpose, we can document them.
> Splitting simply doesn't scale very well, imo.
Do you have another proposal? As Stefano said, we adopted the rule 17.7. 
So we know need a solution to address it.
> > > To deal with the Misra rule imo requires to first have an abstract
> > > plan of how to handle such globally in the code base. Imo such a plan
> > > can't be to introduce perhaps dozens of new wrapper functions like is
> > > done here.
> > This patch is following the right pattern, one we already follow with
> > the _locked suffix.
> Right, and - just to mention it - one which I similarly dislike, albeit
> to a lesser degree.
AFAIU, we are debating between having a boolean indicating if a function 
doesn't fail or adding a wrapper.
While I understand this requires to add more code, the advantage of the 
suffix is this is more obvious for the reader (including the reviewer) 
that the call is not supposed to fail. I agree this is a matter of taste 
here...
In this case, what would be your suggestion to address the problem?

Cheers,

--
Julien Grall