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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-02-28 03:10, Stefano Stabellini wrote:
> 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.
void casts are an option for sure. The rationale for the rule explicitly 
lists them as a compliance mechanism. An interesting aspect is what 
would be the consensus around void casts on functions whose return value 
is always ignored vs. functions whose return value is sometimes ignored.
> 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.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)