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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan