From: Namhyung Kim <namhyung@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Ingo Molnar <mingo@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Joe Perches <joe@perches.com>,
Masami Hiramatsu <mhiramat@kernel.org>,
Tom Zanussi <zanussi@kernel.org>,
Andreas Schwab <schwab@linux-m68k.org>,
kernel-team@lge.com
Subject: Re: [PATCH v4] string.h: Add str_has_prefix() helper function
Date: Sat, 22 Dec 2018 18:33:46 +0900 [thread overview]
Message-ID: <20181222093346.GB7610@danjae.aot.lge.com> (raw)
In-Reply-To: <20181221231924.4583e90b@vmware.local.home>
On Fri, Dec 21, 2018 at 11:19:24PM -0500, Steven Rostedt wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> A discussion came up in the trace triggers thread about converting a
> bunch of:
>
> strncmp(str, "const", sizeof("const") - 1)
>
> use cases into a helper macro. It started with:
>
> strncmp(str, const, sizeof(const) - 1)
>
> But then Joe Perches mentioned that if a const is not used, the
> sizeof() will be the size of a pointer, which can be bad. And that
> gcc will optimize strlen("const") into "sizeof("const") - 1".
>
> Thinking about this more, a quick grep in the kernel tree found several
> (thousands!) of cases that use this construct. A quick grep also
> revealed that there's probably several bugs in that use case. Some are
> that people forgot the "- 1" (which I found) and others could be that
> the constant for the sizeof is different than the constant (although, I
> haven't found any of those, but I also didn't look hard).
>
> 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.
>
> Note, gcc appears to optimize this when we make it into an always_inline
> static function, which removes a lot of issues that a macro produces.
>
> Link: http://lkml.kernel.org/r/e3e754f2bd18e56eaa8baf79bee619316ebf4cfc.1545161087.git.tom.zanussi@linux.intel.com
> Link: http://lkml.kernel.org/r/20181219211615.2298e781@gandalf.local.home
> Link: http://lkml.kernel.org/r/CAHk-=wg_sR-UEC1ggmkZpypOUYanL5CMX4R7ceuaV4QMf5jBtg@mail.gmail.com
>
> Cc: Tom Zanussi <zanussi@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Suggestions-by: Joe Perches <joe@perches.com>
> Suggestions-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>
> Since I haven't heard anything since Linus said he preferred the len
> to be the return value, I'm posting this last version before running
> it through my tests.
>
> Changes since v3:
>
> - Use size_t instead of int (Joe Perches)
>
> include/linux/string.h | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 27d0482e5e05..7927b875f80c 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -456,4 +456,24 @@ 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 size_t str_has_prefix(const char *str, const char *prefix)
> +{
> + size_t len = strlen(prefix);
> + return strncmp(str, prefix, len) == 0 ? len : 0;
As it already knows the length (and it needs to use it for return
value), isn't it (slightly) better using memcmp() instead?
Thanks,
Namhyung
> +}
> +
> #endif /* _LINUX_STRING_H_ */
> --
> 2.19.1
>
next prev parent reply other threads:[~2018-12-22 16:51 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-22 4:19 [PATCH v4] string.h: Add str_has_prefix() helper function Steven Rostedt
2018-12-22 4:42 ` Steven Rostedt
2018-12-22 9:33 ` Namhyung Kim [this message]
2018-12-22 12:24 ` Steven Rostedt
2018-12-22 14:24 ` Namhyung Kim
2018-12-22 15:12 ` Steven Rostedt
2018-12-22 16:16 ` Steven Rostedt
2018-12-22 16:46 ` Namhyung Kim
2018-12-22 17:19 ` Steven Rostedt
2018-12-22 17:23 ` Steven Rostedt
2018-12-22 17:24 ` Steven Rostedt
2018-12-23 3:13 ` Namhyung Kim
2018-12-23 3:23 ` Steven Rostedt
2018-12-23 3:05 ` Namhyung Kim
2019-01-11 8:10 ` [utility perl script] strncmp() -> str_has_prefix() conversions Joe Perches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181222093346.GB7610@danjae.aot.lge.com \
--to=namhyung@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=gregkh@linuxfoundation.org \
--cc=joe@perches.com \
--cc=kernel-team@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=mingo@kernel.org \
--cc=rostedt@goodmis.org \
--cc=schwab@linux-m68k.org \
--cc=torvalds@linux-foundation.org \
--cc=zanussi@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).