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

Steven Rostedt
2018-12-24T00:05:50Z

On Mon, 24 Dec 2018 00:52:13 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On 23/12/2018 23.56, Steven Rostedt wrote:
> > On Sun, 23 Dec 2018 23:01:52 +0100
> > Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >
> >> On 21/12/2018 23.20, Joe Perches wrote:
> >>>
> >>> Using
> >>>
> >>> static inline bool str_has_prefix(const char *str, const char prefix[])
> >>> {
> >>> return !strncmp(str, prefix, strlen(prefix));
> >>> }
> >>>
> >>
> >> We already have exactly that function, it's called strstarts().
> >
> > It's not exact.
>
> Huh? The str_has_prefix() I quoted is exactly strstarts().

The the implemented str_has_prefix() that you replied to is:

+/*
+ * 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 strncmp_prefix().
+ */
+#define strncmp_prefix(str, prefix) \
+ ({ \
+ int ____strcmp_prefix_ret____; \
+ if (__builtin_constant_p(&prefix)) { \
+ ____strcmp_prefix_ret____ = \
+ strncmp(str, prefix, sizeof(prefix) - 1); \
+ } else { \
+ typeof(prefix) ____strcmp_prefix____ = prefix; \
+ ____strcmp_prefix_ret____ = \
+ strncmp(str, ____strcmp_prefix____, \
+ strlen(____strcmp_prefix____)); \
+ } \
+ ____strcmp_prefix_ret____; \
+ })
+

Note, this has turned into a nice function:

http://lkml.kernel.org/r/20181222162856.518489380@goodmis.org

+/**
+ * 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 size_t str_has_prefix(const char *str, const char *prefix)
+{
+ size_t len = strlen(prefix);
+ return strncmp(str, prefix, len) == 0 ? len : 0;
+}
+

>
> >
> > Well, one thing that str_has_prefix() does that strstarts() does not,
> > is to return the prefix length which gets rid of the duplication.
>
> I hadn't seen the patches containing that version of str_has_prefix().
> Anyway, I just wanted to point out that strstarts() exists, so that we
> at least do not add a copy of that.

That's because you didn't read the patch that you quoted, just the
change log.

>
> > Would it be OK to convert strstarts() to return the length of prefix?
>
> Dunno. By far, most users of the strncmp() idiom only seem to be
> interested in the boolean result. It's true that there are some that
> then want to skip the prefix and do further parsing, and I can see how
> avoiding duplicating the prefix length is useful. But the mathematician
> in me can't help consider the corner case of 'the empty string is always
> a prefix of any other string', and having str_has_prefix(str, "") be
> false seems wrong - obviously, nobody would ever use a literal "" there,
> but nothing in str_has_prefix() _requires_ the prefix to be a constant.

Which would be a useless use case. And if you define that it returns
the length of prefix on return, then it both matches and doesn't
match ;-)

>
> Maybe 'bool str_skip_prefix(const char *s, const char *p, const char
> **out)' where *out is set to s+len on success, and set to s on failure
> (just to allow passing &s and continue parsing in elseifs)? That would
> make your 4/5 "tracing: Have the historgram use the result of
> str_has_prefix() for len of prefix" do
>
> if (str_skip_prefix(str, "onmatch(", &action_str)) {
>
> hoisting the action_str declaration to the top, replacing the len variable?
>

The use cases I've used in the final patch series uses the len for
indexing and other cases.

I think I'm keeping the str_has_prefix() and change the other users to
use it in the kernel. Most of the git grep strstarts() is tools
and scripts anyway.

-- Steve