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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 5 Jun 2025, 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?

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?


> > @@ -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