Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper macro

[Steven Rostedt]

On Fri, 21 Dec 2018 14:08:11 -0800
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Fri, Dec 21, 2018 at 12:55 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> >
> > On Fri, 21 Dec 2018 12:41:17 -0800
> > Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >  
> > > Parentheses....  
> >
> > ?  
> 
> Your patch actually had them, but in the body of your email you did
> 
> > #define have_prefix(str, prefix) ({ \
> >       const char *__pfx = (const char *)prefix; \  
> 
> which is just completely wrong.
> 
> Considering your _old_ patch had the exact same bug, I really think
> you need to start internalizing the whole "macro arguments *have* to
> be properly protected" thing.

Well, there's less with assignments that can go wrong than with other
code. That is, there's little that can happen with "int x = arg;" where
arg is the macro paramater to cause something really nasty. The chances
of that happening is up there with using only two underscores causing an
issue.

But they don't hurt to add.

> 
> And I agree with Joe that using a million underscores just makes code
> less legible. Two underscores at the beginning is the standard
> namespace protection. Underscores at the end do nothing. And using
> *more* than two is just crazy.

The two are standard for static variables in C code, but that makes me
worry about macros. I usually do at least three for macros. The
underscores at the end, to me, make it more balanced and easier to read:

	strncmp(str, ___prefix___, ___len___);

To me looks better than:

	strncmp(str, ___prefix, ___len);

But again, no reason to fight this bikeshed.

> 
> Finally, I think the cast is actually bogus and wrong. Why would the
> prefix ever be anything but "const char *"? Adding the cast only makes
> it more possible that somebody uses a truly wrong type ("unsigned long
> *" ?), and then the cast just silently hides it.

Actually after I sent the email, I was thinking the same thing, that
the original strncmp() would probably complain about that too, and we
should keep it the same. Not sure why I even suggested that. Perhaps, I
tested too many use cases :-/

> 
> If somebody uses "unsigned char *" for this, they'd get the exact same
> warning if they were using strncmp and do this by hand.
> 
> So why would that helper function act any differently? Particularly
> when it then has the huge downside that it will also accept absolute
> garbage?
> 
> Anyway, that was a long and winding NAK for your patch.


But after all that and said. I tested it as a static __always_inline,
and guess what. It also optimizes out the strlen() for constants!!!

Suggested-by: Andreas Schwab <schwab@xxxxxxxxxxxxxx>

-- Steve

diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..538469dfb458 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -456,4 +456,25 @@ static inline void memcpy_and_pad(void *dest, size_t dest_len,
 		memcpy(dest, src, dest_len);
 }
 
+/**
+ * str_has_prefix - Test if a string has a given prefix
+ * @str: The string to test
+ * @prefix: The string to see if @str starts with
+ *
+ * A common way to test a prefix of a string is to do:
+ *  strncmp(str, prefix, sizeof(prefix) - 1)
+ *
+ * But this can lead to bugs due to typos, or if prefix is a pointer
+ * and not a constant. Instead use str_has_prefix().
+ *
+ * Returns: 0 if @str does not start with @prefix
+         strlen(@prefix) if @str does start with @prefix
+ */
+static __always_inline int str_has_prefix(const char *str, const char *prefix)
+{
+	int len = strlen(prefix);
+
+	return strncmp(str, prefix, len) == 0 ? len : 0;
+}
+
 #endif /* _LINUX_STRING_H_ */