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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 27 Feb 2024, 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.
> > 
> > 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.

I asked Roberto if void casts are an option for compliance.

In any case, I don't think we should use void casts in the specific
cases this patch is dealing with. Void casts (if anything) should be a
last resort while this patch fixes the issue in a better way.