Re: [PATCH] x86: remove memcmp calls non-compliant with Rule 21.16.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.06.2025 22:49, Stefano Stabellini wrote:
> On Fri, 6 Jun 2025, Nicola Vetrini wrote:
>>>>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>
>>>>
>>>> Missing your own S-o-b.
>>>>
>>>> Also (nit) may I ask that you drop the full stop from the patch subject?
>>>
>>> I'll add the S-o-B and fix the subject
>>>
>>>
>>>>> --- a/xen/arch/x86/dmi_scan.c
>>>>> +++ b/xen/arch/x86/dmi_scan.c
>>>>> @@ -233,7 +233,7 @@ void __init dmi_efi_get_table(const void *smbios,
>>>> const void *smbios3)
>>>>>   const struct smbios_eps *eps = smbios;
>>>>>   const struct smbios3_eps *eps3 = smbios3;
>>>>>
>>>>> - if (eps3 && memcmp(eps3->anchor, "_SM3_", 5) == 0 &&
>>>>> + if (eps3 && strncmp(eps3->anchor, "_SM3_", 5) == 0 &&
>>>>
>>>> Unlike the last example given in the doc, this does not pose the risk of
>>>> false "not equal" returns. Considering there's no example there exactly
>>>> matching this situation, I'm not convinced a change is actually needed.
>>>> (Applies to all other changes here, too.)
>>>
>>> If we consider string literals "pointer types", then I think you are
>>> right that this would fall under what is permitted by 21.16. Nicola,
>>> what do you think?
>>>
>>
>> While I agree that the result of the comparison is correct either way in 
>> these
>> cases, the rule is written to be simple to apply (i.e., not limited only to
>> those cases that may differ), and in particular in the rationale it is
>> indicated that using memcmp to compare string *may* indicate a mistake. As
>> written above, deviating the string literal comparisons is an option, which
>> can be justified with efficiency concerns, but it goes a bit against the
>> rationale of the rule itself.
> 
> Also looking at Andrew's reply, it seems that the preference is to
> deviate string literals. The change to docs/misra/rules.rst is easy
> enough, but I am not sure how to make the corresponding change to
> analysis.ecl.
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index e1c26030e8..56b6e351df 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -813,7 +813,7 @@ maintainers if you want to suggest a change.
>         shall point to either a pointer type, an essentially signed type,
>         an essentially unsigned type, an essentially Boolean type or an
>         essentially enum type
> -     - void* arguments are allowed
> +     - void* and string literals arguments are allowed

Yet as per my earlier reply: This would go too far, wouldn't it?

Jan