Message-ID: <20181221144054.20bdeb33@gandalf.local.home>
Date:   Fri, 21 Dec 2018 14:40:54 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Joe Perches <joe@...ches.com>,
        Tom Zanussi <zanussi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [for-next][PATCH 23/24] string.h: Add strncmp_prefix() helper
 macro

On Fri, 21 Dec 2018 10:51:29 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Fri, Dec 21, 2018 at 9:57 AM Steven Rostedt <rostedt@...dmis.org> wrote:
> >
> > I figured the best thing to do is to create a helper macro and place it
> > into include/linux/string.h. And go around and fix all the open coded
> > versions of it later.
> >
> > I plan on only applying this patch and updating the tracing hooks for
> > this merge window. And perhaps use it to fix some of the bugs that were
> > found.  
> 
> I like the helper function concept, but as they say about CS: "There
> is only one hard problem in computer science: naming and off-by-one
> errors".
> 
> And this one has that problem. The name makes absolutely no sense.

Good thing I rebased and put these patches at the end before running
my tests :-) (Specifically because I was expecting to have feedback like
this).

> 
> Calling it "strncmp_prefix()" when there is no "n" there makes no sense.
> 
> So drop the "n" from the name.

I originally did that, but then I thought strcmp() is a full compare,
and 'n' denotes it is partial. But thinking about it more, yeah one
would think it would need a parameter to represent 'n'.

> 
> And honestly, maybe it would be better to use a different naming
> pattern entirely, and avoid the crazy non-boolean "str*cmp()" model.
> That thing is useful for search comparisons (where "before/after"
> matters), but it's horrible for the actual "is this true or not",
> where the code ends up being
> 
>         if (!strcmp_prefix(a, "prefix")) {
>                 // This is the "Yes, prefix matched" case, despite the
> "if !" syntax
> 
> which is just confusing.
> 
> So I'd really prefer more of a
> 
>         #define has_prefix(str, prefix) ...

I like changing the name.

> 
> model that actually returns a boolean (true if it has a prefix, false
> if it doesn't), rather than use the "str*cmp" naming and return
> values.

Actually, instead of returning a bool, it can return the length if it
matches.

Reason being, there's several locations that does something like:

	while (...) {
		len = strlen("const");
		if (strncmp(str, "const", len) == 0) {
			str += len;
			break;
		}
	}

And I was having trouble thinking about how to deal with these. But if
we have a has_prefix() that returns the length on match then we can do:

	if ((len = has_prefix(str, "const")) {
		str += len;



> 
> (But I agree that *if* you use the "strcmp" naming, then you do need
> to hold to the traditional strcmp return value semantics).
> 
> Hmm?
> 
> Finally, I also suspect that your helper might be slightly fragile.
> Doing sizeof() seems broken. I could see somebody using some prefix
> define for arrays with constant sizes, and doing
> 
>    #define PFX1 "cpp\0"
>    #define PFX2 "c\0\0\0"
>    #define PFX3 "h\0\0\0"
> 
> or similar. Also, is there a reason you use "&prefix" for the constant

That was left over in my original tests in userspace. When I first
tried it with __builtin_constant_p() I got an error, and added the '&',
but then fixed something else. The something else was what actually
caused the error, but since it didn't complain (and past all my tests),
I left in the '&'.

> test? I don't see that pattern anywhere else. Plus it looks entirely
> wrong without the parenthesis (ie "&(prefix)" to make sure we group
> things right).
> 
> So a lot of small problems, but the naming one is big.

OK, what about if we just use strlen() and say that this macro is not
safe for parameters with side effects.

-- Steve

diff --git a/include/linux/string.h b/include/linux/string.h
index 27d0482e5e05..f9d274a81276 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -14,6 +14,29 @@ extern void *memdup_user(const void __user *, size_t);
 extern void *vmemdup_user(const void __user *, size_t);
 extern void *memdup_user_nul(const void __user *, size_t);
 
+/**
+ * have_prefix - Test if a string has a given prefix
+ * @str: The string to test
+ * @prefix: The string to see if @str starts with
+ *
+ * IMPORTANT; @prefix must not have side effects (used more than once here)
+ *
+ * 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 has_prefix().
+ *
+ * Returns: 0 if @str does not start with @prefix
+         strlen(@prefix) if @str does start with @prefix
+ */
+#define has_prefix(str, prefix)						\
+	({								\
+		int ____strcmp_prefix_len____ = strlen(prefix);		\
+		strncmp(str, prefix, ____strcmp_prefix_len____) == 0 ?	\
+			____strcmp_prefix_len____ : 0;			\
+	})
+
 /*
  * Include machine specific inline routines
  */

Powered by blists - more mailing lists