Re: [XEN PATCH v3 3/3] xen: fix violations of MISRA C:2012 Rule 3.1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2023 10:23, Nicola Vetrini wrote:
> On 04/07/23 17:57, Jan Beulich wrote:
>> On 29.06.2023 21:20, Stefano Stabellini wrote:
>>> On Thu, 29 Jun 2023, Luca Fancellu wrote:
>>>>> On 29 Jun 2023, at 11:06, Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> 
>>>>> wrote:
>>>>> --- a/xen/common/xmalloc_tlsf.c
>>>>> +++ b/xen/common/xmalloc_tlsf.c
>>>>> @@ -140,9 +140,7 @@ static inline void MAPPING_SEARCH(unsigned long *r, 
>>>>> int *fl, int *sl)
>>>>>          *fl = flsl(*r) - 1;
>>>>>          *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI;
>>>>>          *fl -= FLI_OFFSET;
>>>>> -        /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0!
>>>>> -         *fl = *sl = 0;
>>>>> -         */
>>>>> +        ASSERT( *fl >= 0 );
>>>>
>>>> I’ve checked the codebase for usage of ASSERT, but I’ve not seen use of it 
>>>> with spaces
>>>> before and after the condition (like our if conditions) so I think they 
>>>> can be dropped.
>>>
>>> Yes, that's right. I am OK with this patch but I think we should wait
>>> for Jan's ack to be sure.
>>>
>>> An alternative that I feel more comfortable in Acking myself because it
>>> doesn't change the semantics of this code would be to change the 3 lines
>>> of code above with this:
>>>
>>> /*
>>>   * ; FL will be always >0!
>>>   * if ((*fl -= FLI_OFFSET) < 0)
>>>   *     fl = *sl = 0;
>>>   */
>>
>> While I'd be okay with this form, as Luca says it'll get us a different
>> violation, which we ought to avoid. While I was the one to suggest the
>> conversion to ASSERT(), having thought about it yet once more I'm now
>> of the opinion that _any_ transformation of this commented out piece of
>> code needs first understanding what was originally meant. Or
>> alternatively, while converting to #if form, to add a comment making
>> crystal clear that it's simply uncertain what was meant.
>>
> 
> About the violation of D4.4: the Directive was never considered for 
> compliance because it's an advisory directive, and hence considerably 
> less urgent.
> 
> Having looked a bit at the surrounding code, since *fl and *sl are used
> as array indices later in 'FIND_SUITABLE_BLOCK', I suggest using 
> something along the lines of "If *fl ever becomes < 0, reset it to a 
> safe value." (either using the form suggested by Stefano or an #if 0).

The main issue I see with any such transformation is how the
immediately preceding "*fl -= FLI_OFFSET;" is intended to interact
with the commented out code. My gut feeling (but nothing else) says
that what was meant would have been

#if 1
        *fl -= FLI_OFFSET;
#else
        if ((*fl -= FLI_OFFSET) < 0) /* FL will be always >0! */
            *fl = *sl = 0;
#endif

But of course without properly understanding the logic it might
also have been

        *fl -= FLI_OFFSET;
#if 0
        if ((*fl -= FLI_OFFSET) < 0) /* FL will be always >0! */
            *fl = *sl = 0;
#endif

> In any case this should become a standalone patch, right?

Preferably, yes.

Jan