Re: [PATCH v3] can, tty: elmcan CAN/ldisc driver for ELM327 based OBD-II adapters

[Max Staudt <max@enpas.org>]

On Tue, 15 Mar 2022 11:21:35 +0100
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

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

It's implicit in the code that calls it.

Anyway, you're the second reviewer to trip upon this (after Vincent),
so my take away is that my code is too confusing. I'll check if I can
just strncmp() it, to keep it simple.

Sorry for what's in large part a premature optimisation.