On 06.06.2025 09:12, Nicola Vetrini wrote:
> On 2025-06-06 01:39, Stefano Stabellini wrote:
>> On Thu, 5 Jun 2025, Jan Beulich wrote:
>>> On 05.06.2025 01:35, Stefano Stabellini wrote:
>>>> From: Alessandro Zucchelli <alessandro.zucche...@bugseng.com>
>>>>
>>>> MISRA C Rule 21.16 states the following: "The pointer arguments to
>>>> the Standard Library function `memcmp' shall point to either a pointer
>>>> type, an essentially signed type, an essentially unsigned type, an
>>>> essentially Boolean type or an essentially enum type".
>>>>
>>>> Comparing string literals with char arrays is more appropriately
>>>> done via strncmp.
>>>
>>> More appropriately - maybe. Yet less efficiently. IOW I view ...
>>>
>>>> No functional change.
>>>
>>> ... this as at the edge of not being true.
>>>
>
> Then our views of what constitutes a functional change clearly differ.
> If you are concerned about performance the patch may be dropped, but
> then does it make sense to apply the rule at all? An alternative
> suggestion might be that of deviating the rule for memcmp applied to
> string literals in either the first or second argument, or both).
FTAOD (since Stefano also said it like this) - it's not just "string
literal". The additional requirement is that the last argument passed
must equal sizeof(<string literal>) for the comparison to work
correctly.
Jan
>>>> Signed-off-by: Alessandro Zucchelli <alessandro.zucche...@bugseng.com>
>>>
>>> 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.
>
>>
>>>> @@ -302,7 +302,7 @@ const char *__init dmi_get_table(paddr_t *base, u32
>>>> *len)
>>>> continue;
>>>> memcpy_fromio(&eps.dmi + 1, q + sizeof(eps.dmi),
>>>> sizeof(eps.smbios3) - sizeof(eps.dmi));
>>>> - if (!memcmp(eps.smbios3.anchor, "_SM3_", 5) &&
>>>> + if (strncmp(eps.smbios3.anchor, "_SM3_", 5) == 0 &&
>>>
>>> Here and below there's a further (style) change, moving from ! to "==
>>> 0"
>>> (or from implicit boolean to "!= 0"). As we use the original style in
>>> many
>>> other places, some justification for this extra change would be needed
>>> in
>>> the description (or these extra adjustments be dropped).
>>
>> The adjustments can be dropped
>>
>>
>>>> @@ -720,10 +720,10 @@ static void __init efi_check_config(void)
>>>> __set_fixmap(FIX_EFI_MPF, PFN_DOWN(efi.mps), __PAGE_HYPERVISOR);
>>>> mpf = fix_to_virt(FIX_EFI_MPF) + ((long)efi.mps & (PAGE_SIZE-1));
>>>>
>>>> - if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
>>>> - mpf->mpf_length == 1 &&
>>>> - mpf_checksum((void *)mpf, 16) &&
>>>> - (mpf->mpf_specification == 1 || mpf->mpf_specification == 4)) {
>>>> + if (strncmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
>>>> + mpf->mpf_length == 1 &&
>>>> + mpf_checksum((void *)mpf, 16) &&
>>>> + (mpf->mpf_specification == 1 || mpf->mpf_specification == 4))
>>>> {
>>>> smp_found_config = true;
>>>> printk(KERN_INFO "SMP MP-table at %08lx\n", efi.mps);
>>>> mpf_found = mpf;
>>>
>>> There are extra (indentation) changes here which ought to be dropped.
>>
>> Yes
>
