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

[Demi Marie Obenour <demiobenour@xxxxxxxxx>]

On 6/5/25 02:31, Jan Beulich wrote:
> On 05.06.2025 01:35, Stefano Stabellini wrote:
>> From: Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>
>>
>> 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.
> 
>> 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?
> 
>> --- 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.)
> 
>> @@ -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).
> 
>> @@ -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.

What about using an array of const uint8_t[] instead of a string literal?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

Attachment: OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature