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

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

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@bugseng.com>
>>>>> 
>>>>> 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)