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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan