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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-02-27 13:47, Jan Beulich wrote:
> On 27.02.2024 12:52, Julien Grall wrote:
> > 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.
Yes. I certainly don't plan to do much splitting either. I saw this one 
as a low-hanging fruit.
> > Do you have another proposal? As Stefano said, we adopted the rule 
> > 17.7.
> > So we know need a solution to address it.
> One possibility that was circulated while discussing was to add (void)
> casts. I'm not a huge fan of those, but between the two options that
> might be the lesser evil. We also use funny (should I say ugly)
> workarounds in a few cases where we have __must_check but still want
> to not really handle the return value in certain cases. Given there are
> example in the code base, extending use of such constructs is certainly
> also something that may want considering.
Can you point out some of these constructs, just to get an idea of what 
that might look like?
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)