lkml.org 
[lkml]   [2022]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] can, tty: elmcan CAN/ldisc driver for ELM327 based OBD-II adapters
On 12.03.2022 22:21:42, Max Staudt wrote:
> @Vincent - two more things have remained, and I hope it's okay once I
> explain them:
>
> 1. _memstrcmp() - memcmp() vs. str(n)cmp()
>
> The _memstrcmp() function does not compare strings, it compares raw
> buffers. I am just using C strings for the fixed buffers to compare
> against, as that allows for shorter and easier to read code. The NUL
> byte at the end of those strings goes unused.
>
> Also, I have not looked at the assembly produced, since the semantics
> are different: str(n)cmp() needs to look for NUL bytes in the buffer(s),
> which is unnecessary here. As a bonus, NUL will never even occur
> because my code filters those bytes out upon reception from the UART
> (it's a documented quirk of the ELM327).
>
> Finally, even if I were to use strcmp(), the code would still look just
> as ugly. Except the machine would also look for NUL bytes, and the next
> human to read the code would wonder why I'm comparing strings and not
> buffers.
>
> Hence memcmp(), to help the code self-document and the compiler
> optimise - I hope that's okay.

Looking at the code:

> +/* Compare a buffer to a fixed string */
> +static inline int _memstrcmp(const u8 *mem, const char *str)
> +{
> + return memcmp(mem, str, strlen(str));

The _memstrcmp is sometimes directly used. Where's the check that mem is
valid for strlen(len)? Better only use _len_memstrcmp().

> +}
> +
> +/* Compare buffer to string length, then compare buffer to fixed string.
> + * This ensures two things:
> + * - It flags cases where the fixed string is only the start of the
> + * buffer, rather than exactly all of it.
> + * - It avoids byte comparisons in case the length doesn't match.
> + */
> +static inline int _len_memstrcmp(const u8 *mem, size_t mem_len, const char *str)

make it a bool.

> +{
> + size_t str_len = strlen(str);
> +
> + return (mem_len != str_len) || memcmp(mem, str, str_len);
> +}

Can you rename _len_memstrcmp into something like elm327_match() or so?

regards,
Marc

--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-03-15 11:22    [W:0.189 / U:0.336 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site