Messages in this thread | | | Date | Tue, 15 Mar 2022 11:21:35 +0100 | From | Marc Kleine-Budde <> | Subject | Re: [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] | |