[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
  • Date: Fri, 06 Jun 2025 09:12:52 +0200
  • Arc-authentication-results: i=1; bugseng.com; arc=none smtp.remote-ip=162.55.131.47
  • Arc-message-signature: i=1; d=bugseng.com; s=openarc; a=rsa-sha256; c=relaxed/relaxed; t=1749193972; h=DKIM-Signature:MIME-Version:Date:From:To:Cc:Subject:In-Reply-To: References:Message-ID:X-Sender:Organization:Content-Type: Content-Transfer-Encoding; bh=8XlTGpk6rzDC9YecCj+wib31GFHYBlMi6psmQaX3a3A=; b=IAwwlNB7U6/yb08y8pVNMcP5IfonrVuB9NoD4rMyn525y/Mvh4LQExN92gFNaQgnapiA 12+Sfr8RBVGdXGBRKx+kq76SYyZlICZ2NGlZDaqE+VYdvzCKXmYAtF8rCp0jvQiZCfTym AyFv6tsSbdLQZxZfRfBbgS5IRoc3xRnnFs4BTxfVYABqVYOl5BoQuMszF85ISFwj/P55h sx/D8lEdKhdI4hrlRB+lnNujBuvHx+DVXCYoZ/dhhZ/Uf/LBNrK7YkUDYY8LuSur8Q+a4 f+1t5vXiYqZWvhuJERmTNLZ4kkIbba+kPUxSI8fZ2G8Mt0sDdInsJQ9kIR2xHwMS6PNbS PRpl0c1vPE8BuylcgBHC660Dh1hBDyb/9cLo2mf8k3a3OdaVKXdrXLREx10glzYaG5rgg TYXEFCC2/teOG3eRyRpAzLOsxUc7TDPxWisZup+pGnbX5o1bzkS0ZZNAWujm9L1JRpjVF 03dxvfkfQ4Q4TepvSltFY0pHmSUOzKxy7CbaGaHtnIuTQjlAtxFtZskesou/QrJrwy2QO zgy3WMqUMpuGc+zTA4mCbmhWY8ljy5PJIa/rXALxV9/urCTDyPbKnn7xBhIQN+H1zOHTh 0LNOrYT59fQ20k6HguP5l8KjfNu5VKz+pHRQYzdGbMeFGLbiOhEB2rUzI3JdsrM=
  • Arc-seal: i=1; d=bugseng.com; s=openarc; a=rsa-sha256; cv=none; t=1749193972; b=KEuBOrWrt8hxb4KviAmQMkmbOAYfrWVBihPQKiE4Z6vYZLFCniY3e6gYoBFEEkssDtsz qanDT/xJDub4NiiClVDsHe5v5JrgMZGPK8GjoOr39aGvR4SFKiMhXTr64TfcbJHXvsKRm 7omy4MH8p/Sc/K0LuUQscORpxMKGEjo/gP49IFZN+1Jn44eWeGggmKJmmQbI800kSrLv2 9/Ah8ZQozM35VdHD4dpB1WQ46xBjHVjDN5q6bGpD+qG3RjIA4H+3ADydZBGgjqYzkbtoa +d4FZ3x6FnFR3a21w20lbbqdax7bge3t4e0kCpdFYhQboiFHh6arLs2n7i032DTrgvxsH Bp4zm5upq3ANcgVlrpLQ4+dVe+g5S9GKpQdmu2Z7yLRSChuSGQAQ+YXcmGJBakBEs+Unw AX7Onzf1O0UVIg0lbPyvSyhXimKCT61oQZYLmZMhmcW0HHl4/HS4bmm1Z9pgvR4KPYsvA InIJ8y4fpBnBte7D4IuyGc/nk3cvHdGoF7yZDNQYSD5Sx39RJTi1v6DoFsAR4crl2eut4 L59WiCRIjZA7k86xmrGeQhf+dRvO8GM4pqMURf+h94EWztdeIDDbs9IZFaO7rJEufhNkG Rr8F/3J/BBVt2lkNQbNKplXF1V0MEq37fpyVBsrC4e6FxVkZjWUNWG4bqBL3nJE=
  • Authentication-results: bugseng.com; arc=none smtp.remote-ip=162.55.131.47
  • Cc: Jan Beulich <jbeulich@xxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx, federico.serafini@xxxxxxxxxxx
  • Delivery-date: Fri, 06 Jun 2025 07:12:56 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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).

> 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

--
Nicola Vetrini, B.Sc.
Software Engineer
BUGSENG (https://bugseng.com)
LinkedIn: https://www.linkedin.com/in/nicola-vetrini-a42471253



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.