[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86: remove memcmp calls non-compliant with Rule 21.16.
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.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. >>> > > 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.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. > >> >>>> @@ -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 >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |