* [for-next][PATCH 1/5] string.h: Add str_has_prefix() helper function
2018-12-22 16:20 [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages Steven Rostedt
@ 2018-12-22 16:20 ` Steven Rostedt
2018-12-22 16:20 ` [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code Steven Rostedt
` (5 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Steven Rostedt @ 2018-12-22 16:20 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Joe Perches, Namhyung Kim,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh,
Tom Zanussi, Greg Kroah-Hartman
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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Suggestions-by: Linus Torvalds <torvalds@linux-foundation.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>
---
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;
+}
+
#endif /* _LINUX_STRING_H_ */
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code
2018-12-22 16:20 [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages Steven Rostedt
2018-12-22 16:20 ` [for-next][PATCH 1/5] string.h: Add str_has_prefix() helper function Steven Rostedt
@ 2018-12-22 16:20 ` Steven Rostedt
2018-12-22 18:01 ` Steven Rostedt
2018-12-23 3:32 ` Namhyung Kim
2018-12-22 16:20 ` [for-next][PATCH 3/5] tracing: Use str_has_prefix() instead of using fixed sizes Steven Rostedt
` (4 subsequent siblings)
6 siblings, 2 replies; 19+ messages in thread
From: Steven Rostedt @ 2018-12-22 16:20 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Joe Perches, Namhyung Kim,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh,
Tom Zanussi
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
The tracing histogram code contains a lot of instances of the construct:
strncmp(str, "const", sizeof("const") - 1)
This can be prone to bugs due to typos or bad cut and paste. Use the
str_has_prefix() helper macro instead that removes the need for having two
copies of the constant string.
Cc: Tom Zanussi <zanussi@linux.intel.com>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_events_hist.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c5448c103770..9d590138f870 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1881,8 +1881,8 @@ static int parse_action(char *str, struct hist_trigger_attrs *attrs)
if (attrs->n_actions >= HIST_ACTIONS_MAX)
return ret;
- if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) ||
- (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
+ if ((str_has_prefix(str, "onmatch(")) ||
+ (str_has_prefix(str, "onmax("))) {
attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
if (!attrs->action_str[attrs->n_actions]) {
ret = -ENOMEM;
@@ -1899,34 +1899,34 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
{
int ret = 0;
- if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
- (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
+ if ((str_has_prefix(str, "key=")) ||
+ (str_has_prefix(str, "keys="))) {
attrs->keys_str = kstrdup(str, GFP_KERNEL);
if (!attrs->keys_str) {
ret = -ENOMEM;
goto out;
}
- } else if ((strncmp(str, "val=", sizeof("val=") - 1) == 0) ||
- (strncmp(str, "vals=", sizeof("vals=") - 1) == 0) ||
- (strncmp(str, "values=", sizeof("values=") - 1) == 0)) {
+ } else if ((str_has_prefix(str, "val=")) ||
+ (str_has_prefix(str, "vals=")) ||
+ (str_has_prefix(str, "values="))) {
attrs->vals_str = kstrdup(str, GFP_KERNEL);
if (!attrs->vals_str) {
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "sort=", sizeof("sort=") - 1) == 0) {
+ } else if (str_has_prefix(str, "sort=")) {
attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
if (!attrs->sort_key_str) {
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "name=", sizeof("name=") - 1) == 0) {
+ } else if (str_has_prefix(str, "name=")) {
attrs->name = kstrdup(str, GFP_KERNEL);
if (!attrs->name) {
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
+ } else if (str_has_prefix(str, "clock=")) {
strsep(&str, "=");
if (!str) {
ret = -EINVAL;
@@ -1939,7 +1939,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
ret = -ENOMEM;
goto out;
}
- } else if (strncmp(str, "size=", sizeof("size=") - 1) == 0) {
+ } else if (str_has_prefix(str, "size=")) {
int map_bits = parse_map_size(str);
if (map_bits < 0) {
@@ -3558,7 +3558,7 @@ static struct action_data *onmax_parse(char *str)
if (!onmax_fn_name || !str)
goto free;
- if (strncmp(onmax_fn_name, "save", sizeof("save") - 1) == 0) {
+ if (str_has_prefix(onmax_fn_name, "save")) {
char *params = strsep(&str, ")");
if (!params) {
@@ -4346,7 +4346,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)
for (i = 0; i < hist_data->attrs->n_actions; i++) {
str = hist_data->attrs->action_str[i];
- if (strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) {
+ if (str_has_prefix(str, "onmatch(")) {
char *action_str = str + sizeof("onmatch(") - 1;
data = onmatch_parse(tr, action_str);
@@ -4355,7 +4355,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)
break;
}
data->fn = action_trace;
- } else if (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0) {
+ } else if (str_has_prefix(str, "onmax(")) {
char *action_str = str + sizeof("onmax(") - 1;
data = onmax_parse(action_str);
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code
2018-12-22 16:20 ` [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code Steven Rostedt
@ 2018-12-22 18:01 ` Steven Rostedt
2018-12-22 18:02 ` Steven Rostedt
2018-12-22 18:03 ` Tom Zanussi
2018-12-23 3:32 ` Namhyung Kim
1 sibling, 2 replies; 19+ messages in thread
From: Steven Rostedt @ 2018-12-22 18:01 UTC (permalink / raw)
To: Tom Zanussi
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Namhyung Kim, Linus Torvalds, Yoshinori Sato, Rich Felker,
linux-sh
On Sat, 22 Dec 2018 11:20:09 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> The tracing histogram code contains a lot of instances of the construct:
>
> strncmp(str, "const", sizeof("const") - 1)
>
> This can be prone to bugs due to typos or bad cut and paste. Use the
> str_has_prefix() helper macro instead that removes the need for having two
> copies of the constant string.
>
> Cc: Tom Zanussi <zanussi@linux.intel.com>
I have no idea why I copied your intel email. The linux.intel.com
appears to be no longer active. I'm going to rebase to fix this email
address.
-- Steve
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> kernel/trace/trace_events_hist.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index c5448c103770..9d590138f870 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1881,8 +1881,8 @@ static int parse_action(char *str, struct hist_trigger_attrs *attrs)
> if (attrs->n_actions >= HIST_ACTIONS_MAX)
> return ret;
>
> - if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) ||
> - (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
> + if ((str_has_prefix(str, "onmatch(")) ||
> + (str_has_prefix(str, "onmax("))) {
> attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
> if (!attrs->action_str[attrs->n_actions]) {
> ret = -ENOMEM;
> @@ -1899,34 +1899,34 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
> {
> int ret = 0;
>
> - if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
> - (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
> + if ((str_has_prefix(str, "key=")) ||
> + (str_has_prefix(str, "keys="))) {
> attrs->keys_str = kstrdup(str, GFP_KERNEL);
> if (!attrs->keys_str) {
> ret = -ENOMEM;
> goto out;
> }
> - } else if ((strncmp(str, "val=", sizeof("val=") - 1) == 0) ||
> - (strncmp(str, "vals=", sizeof("vals=") - 1) == 0) ||
> - (strncmp(str, "values=", sizeof("values=") - 1) == 0)) {
> + } else if ((str_has_prefix(str, "val=")) ||
> + (str_has_prefix(str, "vals=")) ||
> + (str_has_prefix(str, "values="))) {
> attrs->vals_str = kstrdup(str, GFP_KERNEL);
> if (!attrs->vals_str) {
> ret = -ENOMEM;
> goto out;
> }
> - } else if (strncmp(str, "sort=", sizeof("sort=") - 1) == 0) {
> + } else if (str_has_prefix(str, "sort=")) {
> attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
> if (!attrs->sort_key_str) {
> ret = -ENOMEM;
> goto out;
> }
> - } else if (strncmp(str, "name=", sizeof("name=") - 1) == 0) {
> + } else if (str_has_prefix(str, "name=")) {
> attrs->name = kstrdup(str, GFP_KERNEL);
> if (!attrs->name) {
> ret = -ENOMEM;
> goto out;
> }
> - } else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
> + } else if (str_has_prefix(str, "clock=")) {
> strsep(&str, "=");
> if (!str) {
> ret = -EINVAL;
> @@ -1939,7 +1939,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
> ret = -ENOMEM;
> goto out;
> }
> - } else if (strncmp(str, "size=", sizeof("size=") - 1) == 0) {
> + } else if (str_has_prefix(str, "size=")) {
> int map_bits = parse_map_size(str);
>
> if (map_bits < 0) {
> @@ -3558,7 +3558,7 @@ static struct action_data *onmax_parse(char *str)
> if (!onmax_fn_name || !str)
> goto free;
>
> - if (strncmp(onmax_fn_name, "save", sizeof("save") - 1) == 0) {
> + if (str_has_prefix(onmax_fn_name, "save")) {
> char *params = strsep(&str, ")");
>
> if (!params) {
> @@ -4346,7 +4346,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)
> for (i = 0; i < hist_data->attrs->n_actions; i++) {
> str = hist_data->attrs->action_str[i];
>
> - if (strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) {
> + if (str_has_prefix(str, "onmatch(")) {
> char *action_str = str + sizeof("onmatch(") - 1;
>
> data = onmatch_parse(tr, action_str);
> @@ -4355,7 +4355,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)
> break;
> }
> data->fn = action_trace;
> - } else if (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0) {
> + } else if (str_has_prefix(str, "onmax(")) {
> char *action_str = str + sizeof("onmax(") - 1;
>
> data = onmax_parse(action_str);
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code
2018-12-22 18:01 ` Steven Rostedt
@ 2018-12-22 18:02 ` Steven Rostedt
2018-12-22 18:03 ` Tom Zanussi
1 sibling, 0 replies; 19+ messages in thread
From: Steven Rostedt @ 2018-12-22 18:02 UTC (permalink / raw)
To: Tom Zanussi
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Namhyung Kim, Linus Torvalds, Yoshinori Sato, Rich Felker,
linux-sh
On Sat, 22 Dec 2018 13:01:07 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> > Cc: Tom Zanussi <zanussi@linux.intel.com>
>
> I have no idea why I copied your intel email. The linux.intel.com
> appears to be no longer active. I'm going to rebase to fix this email
> address.
Looking at the other patches, I guess I cut and pasted wrong.
I see: Tom Zanussi <tom.zanussi@linux.intel.com>
(with the "tom." part missing in this patch).
Still I'll fix it.
-- Steve
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code
2018-12-22 18:01 ` Steven Rostedt
2018-12-22 18:02 ` Steven Rostedt
@ 2018-12-22 18:03 ` Tom Zanussi
2018-12-23 3:53 ` Steven Rostedt
1 sibling, 1 reply; 19+ messages in thread
From: Tom Zanussi @ 2018-12-22 18:03 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Namhyung Kim, Linus Torvalds, Yoshinori Sato, Rich Felker,
linux-sh
Hi Steve,
On Sat, 2018-12-22 at 13:01 -0500, Steven Rostedt wrote:
> On Sat, 22 Dec 2018 11:20:09 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> >
> > The tracing histogram code contains a lot of instances of the
> > construct:
> >
> > strncmp(str, "const", sizeof("const") - 1)
> >
> > This can be prone to bugs due to typos or bad cut and paste. Use
> > the
> > str_has_prefix() helper macro instead that removes the need for
> > having two
> > copies of the constant string.
> >
> > Cc: Tom Zanussi <zanussi@linux.intel.com>
>
> I have no idea why I copied your intel email. The linux.intel.com
> appears to be no longer active. I'm going to rebase to fix this email
> address.
linux.intel.com is active, but there's no zanussi there, just
tom.zanussi ;-)
So tom.zanussi@linux.intel.com should work fine.
Tom
>
> -- Steve
>
>
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> > kernel/trace/trace_events_hist.c | 28 ++++++++++++++--------------
> > 1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index c5448c103770..9d590138f870 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -1881,8 +1881,8 @@ static int parse_action(char *str, struct
> > hist_trigger_attrs *attrs)
> > if (attrs->n_actions >= HIST_ACTIONS_MAX)
> > return ret;
> >
> > - if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) ==
> > 0) ||
> > - (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
> > + if ((str_has_prefix(str, "onmatch(")) ||
> > + (str_has_prefix(str, "onmax("))) {
> > attrs->action_str[attrs->n_actions] = kstrdup(str,
> > GFP_KERNEL);
> > if (!attrs->action_str[attrs->n_actions]) {
> > ret = -ENOMEM;
> > @@ -1899,34 +1899,34 @@ static int parse_assignment(char *str,
> > struct hist_trigger_attrs *attrs)
> > {
> > int ret = 0;
> >
> > - if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
> > - (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
> > + if ((str_has_prefix(str, "key=")) ||
> > + (str_has_prefix(str, "keys="))) {
> > attrs->keys_str = kstrdup(str, GFP_KERNEL);
> > if (!attrs->keys_str) {
> > ret = -ENOMEM;
> > goto out;
> > }
> > - } else if ((strncmp(str, "val=", sizeof("val=") - 1) == 0)
> > ||
> > - (strncmp(str, "vals=", sizeof("vals=") - 1) == 0)
> > ||
> > - (strncmp(str, "values=", sizeof("values=") - 1)
> > == 0)) {
> > + } else if ((str_has_prefix(str, "val=")) ||
> > + (str_has_prefix(str, "vals=")) ||
> > + (str_has_prefix(str, "values="))) {
> > attrs->vals_str = kstrdup(str, GFP_KERNEL);
> > if (!attrs->vals_str) {
> > ret = -ENOMEM;
> > goto out;
> > }
> > - } else if (strncmp(str, "sort=", sizeof("sort=") - 1) ==
> > 0) {
> > + } else if (str_has_prefix(str, "sort=")) {
> > attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
> > if (!attrs->sort_key_str) {
> > ret = -ENOMEM;
> > goto out;
> > }
> > - } else if (strncmp(str, "name=", sizeof("name=") - 1) ==
> > 0) {
> > + } else if (str_has_prefix(str, "name=")) {
> > attrs->name = kstrdup(str, GFP_KERNEL);
> > if (!attrs->name) {
> > ret = -ENOMEM;
> > goto out;
> > }
> > - } else if (strncmp(str, "clock=", sizeof("clock=") - 1) ==
> > 0) {
> > + } else if (str_has_prefix(str, "clock=")) {
> > strsep(&str, "=");
> > if (!str) {
> > ret = -EINVAL;
> > @@ -1939,7 +1939,7 @@ static int parse_assignment(char *str, struct
> > hist_trigger_attrs *attrs)
> > ret = -ENOMEM;
> > goto out;
> > }
> > - } else if (strncmp(str, "size=", sizeof("size=") - 1) ==
> > 0) {
> > + } else if (str_has_prefix(str, "size=")) {
> > int map_bits = parse_map_size(str);
> >
> > if (map_bits < 0) {
> > @@ -3558,7 +3558,7 @@ static struct action_data *onmax_parse(char
> > *str)
> > if (!onmax_fn_name || !str)
> > goto free;
> >
> > - if (strncmp(onmax_fn_name, "save", sizeof("save") - 1) ==
> > 0) {
> > + if (str_has_prefix(onmax_fn_name, "save")) {
> > char *params = strsep(&str, ")");
> >
> > if (!params) {
> > @@ -4346,7 +4346,7 @@ static int parse_actions(struct
> > hist_trigger_data *hist_data)
> > for (i = 0; i < hist_data->attrs->n_actions; i++) {
> > str = hist_data->attrs->action_str[i];
> >
> > - if (strncmp(str, "onmatch(", sizeof("onmatch(") -
> > 1) == 0) {
> > + if (str_has_prefix(str, "onmatch(")) {
> > char *action_str = str +
> > sizeof("onmatch(") - 1;
> >
> > data = onmatch_parse(tr, action_str);
> > @@ -4355,7 +4355,7 @@ static int parse_actions(struct
> > hist_trigger_data *hist_data)
> > break;
> > }
> > data->fn = action_trace;
> > - } else if (strncmp(str, "onmax(", sizeof("onmax(")
> > - 1) == 0) {
> > + } else if (str_has_prefix(str, "onmax(")) {
> > char *action_str = str + sizeof("onmax(")
> > - 1;
> >
> > data = onmax_parse(action_str);
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code
2018-12-22 18:03 ` Tom Zanussi
@ 2018-12-23 3:53 ` Steven Rostedt
2018-12-23 16:38 ` Tom Zanussi
0 siblings, 1 reply; 19+ messages in thread
From: Steven Rostedt @ 2018-12-23 3:53 UTC (permalink / raw)
To: Tom Zanussi
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Namhyung Kim, Linus Torvalds, Yoshinori Sato, Rich Felker,
linux-sh
On Sat, 22 Dec 2018 12:03:41 -0600
Tom Zanussi <zanussi@kernel.org> wrote:
> Hi Steve,
>
> On Sat, 2018-12-22 at 13:01 -0500, Steven Rostedt wrote:
> > On Sat, 22 Dec 2018 11:20:09 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > >
> > > The tracing histogram code contains a lot of instances of the
> > > construct:
> > >
> > > strncmp(str, "const", sizeof("const") - 1)
> > >
> > > This can be prone to bugs due to typos or bad cut and paste. Use
> > > the
> > > str_has_prefix() helper macro instead that removes the need for
> > > having two
> > > copies of the constant string.
> > >
> > > Cc: Tom Zanussi <zanussi@linux.intel.com>
> >
> > I have no idea why I copied your intel email. The linux.intel.com
> > appears to be no longer active. I'm going to rebase to fix this email
> > address.
>
> linux.intel.com is active, but there's no zanussi there, just
> tom.zanussi ;-)
>
> So tom.zanussi@linux.intel.com should work fine.
BTW, would you give me your ack-by?
-- Steve
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code
2018-12-23 3:53 ` Steven Rostedt
@ 2018-12-23 16:38 ` Tom Zanussi
0 siblings, 0 replies; 19+ messages in thread
From: Tom Zanussi @ 2018-12-23 16:38 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Namhyung Kim, Linus Torvalds, Yoshinori Sato, Rich Felker,
linux-sh
On Sat, 2018-12-22 at 22:53 -0500, Steven Rostedt wrote:
> On Sat, 22 Dec 2018 12:03:41 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
>
> > Hi Steve,
> >
> > On Sat, 2018-12-22 at 13:01 -0500, Steven Rostedt wrote:
> > > On Sat, 22 Dec 2018 11:20:09 -0500
> > > Steven Rostedt <rostedt@goodmis.org> wrote:
> > >
> > > > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > > >
> > > > The tracing histogram code contains a lot of instances of the
> > > > construct:
> > > >
> > > > strncmp(str, "const", sizeof("const") - 1)
> > > >
> > > > This can be prone to bugs due to typos or bad cut and paste.
> > > > Use
> > > > the
> > > > str_has_prefix() helper macro instead that removes the need for
> > > > having two
> > > > copies of the constant string.
> > > >
> > > > Cc: Tom Zanussi <zanussi@linux.intel.com>
> > >
> > > I have no idea why I copied your intel email. The linux.intel.com
> > > appears to be no longer active. I'm going to rebase to fix this
> > > email
> > > address.
> >
> > linux.intel.com is active, but there's no zanussi there, just
> > tom.zanussi ;-)
> >
> > So tom.zanussi@linux.intel.com should work fine.
>
> BTW, would you give me your ack-by?
>
Sure,
Acked-by: Tom Zanussi <tom.zanussi@linux.intel.com>
Tom
> -- Steve
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code
2018-12-22 16:20 ` [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code Steven Rostedt
2018-12-22 18:01 ` Steven Rostedt
@ 2018-12-23 3:32 ` Namhyung Kim
1 sibling, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2018-12-23 3:32 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh,
Tom Zanussi, kernel-team
On Sat, Dec 22, 2018 at 11:20:09AM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> The tracing histogram code contains a lot of instances of the construct:
>
> strncmp(str, "const", sizeof("const") - 1)
>
> This can be prone to bugs due to typos or bad cut and paste. Use the
> str_has_prefix() helper macro instead that removes the need for having two
> copies of the constant string.
>
> Cc: Tom Zanussi <zanussi@linux.intel.com>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> kernel/trace/trace_events_hist.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index c5448c103770..9d590138f870 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1881,8 +1881,8 @@ static int parse_action(char *str, struct hist_trigger_attrs *attrs)
> if (attrs->n_actions >= HIST_ACTIONS_MAX)
> return ret;
>
> - if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) ||
> - (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
> + if ((str_has_prefix(str, "onmatch(")) ||
> + (str_has_prefix(str, "onmax("))) {
> attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
> if (!attrs->action_str[attrs->n_actions]) {
> ret = -ENOMEM;
> @@ -1899,34 +1899,34 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
> {
> int ret = 0;
>
> - if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
> - (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
> + if ((str_has_prefix(str, "key=")) ||
> + (str_has_prefix(str, "keys="))) {
> attrs->keys_str = kstrdup(str, GFP_KERNEL);
> if (!attrs->keys_str) {
> ret = -ENOMEM;
> goto out;
> }
> - } else if ((strncmp(str, "val=", sizeof("val=") - 1) == 0) ||
> - (strncmp(str, "vals=", sizeof("vals=") - 1) == 0) ||
> - (strncmp(str, "values=", sizeof("values=") - 1) == 0)) {
> + } else if ((str_has_prefix(str, "val=")) ||
> + (str_has_prefix(str, "vals=")) ||
> + (str_has_prefix(str, "values="))) {
> attrs->vals_str = kstrdup(str, GFP_KERNEL);
> if (!attrs->vals_str) {
> ret = -ENOMEM;
> goto out;
> }
> - } else if (strncmp(str, "sort=", sizeof("sort=") - 1) == 0) {
> + } else if (str_has_prefix(str, "sort=")) {
> attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
> if (!attrs->sort_key_str) {
> ret = -ENOMEM;
> goto out;
> }
> - } else if (strncmp(str, "name=", sizeof("name=") - 1) == 0) {
> + } else if (str_has_prefix(str, "name=")) {
> attrs->name = kstrdup(str, GFP_KERNEL);
> if (!attrs->name) {
> ret = -ENOMEM;
> goto out;
> }
> - } else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
> + } else if (str_has_prefix(str, "clock=")) {
> strsep(&str, "=");
> if (!str) {
> ret = -EINVAL;
> @@ -1939,7 +1939,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
> ret = -ENOMEM;
> goto out;
> }
> - } else if (strncmp(str, "size=", sizeof("size=") - 1) == 0) {
> + } else if (str_has_prefix(str, "size=")) {
> int map_bits = parse_map_size(str);
>
> if (map_bits < 0) {
> @@ -3558,7 +3558,7 @@ static struct action_data *onmax_parse(char *str)
> if (!onmax_fn_name || !str)
> goto free;
>
> - if (strncmp(onmax_fn_name, "save", sizeof("save") - 1) == 0) {
> + if (str_has_prefix(onmax_fn_name, "save")) {
> char *params = strsep(&str, ")");
>
> if (!params) {
> @@ -4346,7 +4346,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)
> for (i = 0; i < hist_data->attrs->n_actions; i++) {
> str = hist_data->attrs->action_str[i];
>
> - if (strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) {
> + if (str_has_prefix(str, "onmatch(")) {
> char *action_str = str + sizeof("onmatch(") - 1;
You'd better using the return value of str_has_prefix().
>
> data = onmatch_parse(tr, action_str);
> @@ -4355,7 +4355,7 @@ static int parse_actions(struct hist_trigger_data *hist_data)
> break;
> }
> data->fn = action_trace;
> - } else if (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0) {
> + } else if (str_has_prefix(str, "onmax(")) {
> char *action_str = str + sizeof("onmax(") - 1;
Ditto.
Thanks,
Namhyung
>
> data = onmax_parse(action_str);
> --
> 2.19.2
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [for-next][PATCH 3/5] tracing: Use str_has_prefix() instead of using fixed sizes
2018-12-22 16:20 [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages Steven Rostedt
2018-12-22 16:20 ` [for-next][PATCH 1/5] string.h: Add str_has_prefix() helper function Steven Rostedt
2018-12-22 16:20 ` [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code Steven Rostedt
@ 2018-12-22 16:20 ` Steven Rostedt
2018-12-22 16:20 ` [for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix Steven Rostedt
` (3 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Steven Rostedt @ 2018-12-22 16:20 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Joe Perches, Namhyung Kim,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
There are several instances of strncmp(str, "const", 123), where 123 is the
strlen of the const string to check if "const" is the prefix of str. But
this can be error prone. Use str_has_prefix() instead.
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace.c | 2 +-
kernel/trace/trace_events.c | 2 +-
kernel/trace/trace_events_hist.c | 2 +-
kernel/trace/trace_probe.c | 4 ++--
kernel/trace/trace_stack.c | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 5afcfecb4bc2..eac2824a18ab 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4411,7 +4411,7 @@ static int trace_set_options(struct trace_array *tr, char *option)
cmp = strstrip(option);
- if (strncmp(cmp, "no", 2) == 0) {
+ if (str_has_prefix(cmp, "no")) {
neg = 1;
cmp += 2;
}
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index bd0162c0467c..5b3b0c3c8a47 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -1251,7 +1251,7 @@ static int f_show(struct seq_file *m, void *v)
*/
array_descriptor = strchr(field->type, '[');
- if (!strncmp(field->type, "__data_loc", 10))
+ if (str_has_prefix(field->type, "__data_loc"))
array_descriptor = NULL;
if (!array_descriptor)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 9d590138f870..0d878dcd1e4b 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -518,7 +518,7 @@ static int synth_event_define_fields(struct trace_event_call *call)
static bool synth_field_signed(char *type)
{
- if (strncmp(type, "u", 1) == 0)
+ if (str_has_prefix(type, "u"))
return false;
return true;
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index ff86417c0149..541375737403 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -194,7 +194,7 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
code->op = FETCH_OP_RETVAL;
else
ret = -EINVAL;
- } else if (strncmp(arg, "stack", 5) == 0) {
+ } else if (str_has_prefix(arg, "stack")) {
if (arg[5] == '\0') {
code->op = FETCH_OP_STACKP;
} else if (isdigit(arg[5])) {
@@ -213,7 +213,7 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
} else if (((flags & TPARG_FL_MASK) ==
(TPARG_FL_KERNEL | TPARG_FL_FENTRY)) &&
- strncmp(arg, "arg", 3) == 0) {
+ str_has_prefix(arg, "arg")) {
if (!isdigit(arg[3]))
return -EINVAL;
ret = kstrtoul(arg + 3, 10, ¶m);
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index e2a153fc1afc..3641f28c343f 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -448,7 +448,7 @@ static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata;
static __init int enable_stacktrace(char *str)
{
- if (strncmp(str, "_filter=", 8) == 0)
+ if (str_has_prefix(str, "_filter="))
strncpy(stack_trace_filter_buf, str+8, COMMAND_LINE_SIZE);
stack_tracer_enabled = 1;
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* [for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix
2018-12-22 16:20 [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages Steven Rostedt
` (2 preceding siblings ...)
2018-12-22 16:20 ` [for-next][PATCH 3/5] tracing: Use str_has_prefix() instead of using fixed sizes Steven Rostedt
@ 2018-12-22 16:20 ` Steven Rostedt
2018-12-23 3:35 ` Namhyung Kim
2018-12-23 16:40 ` Tom Zanussi
2018-12-22 16:20 ` [for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers Steven Rostedt
` (2 subsequent siblings)
6 siblings, 2 replies; 19+ messages in thread
From: Steven Rostedt @ 2018-12-22 16:20 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Joe Perches, Namhyung Kim,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh,
Tom Zanussi
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
As str_has_prefix() returns the length on match, we can use that for the
updating of the string pointer instead of recalculating the prefix size.
Cc: Tom Zanussi <zanussi@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace_events_hist.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 0d878dcd1e4b..449d90cfa151 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -4342,12 +4342,13 @@ static int parse_actions(struct hist_trigger_data *hist_data)
unsigned int i;
int ret = 0;
char *str;
+ int len;
for (i = 0; i < hist_data->attrs->n_actions; i++) {
str = hist_data->attrs->action_str[i];
- if (str_has_prefix(str, "onmatch(")) {
- char *action_str = str + sizeof("onmatch(") - 1;
+ if ((len = str_has_prefix(str, "onmatch("))) {
+ char *action_str = str + len;
data = onmatch_parse(tr, action_str);
if (IS_ERR(data)) {
@@ -4355,8 +4356,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)
break;
}
data->fn = action_trace;
- } else if (str_has_prefix(str, "onmax(")) {
- char *action_str = str + sizeof("onmax(") - 1;
+ } else if ((len = str_has_prefix(str, "onmax("))) {
+ char *action_str = str + len;
data = onmax_parse(action_str);
if (IS_ERR(data)) {
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix
2018-12-22 16:20 ` [for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix Steven Rostedt
@ 2018-12-23 3:35 ` Namhyung Kim
2018-12-23 16:40 ` Tom Zanussi
1 sibling, 0 replies; 19+ messages in thread
From: Namhyung Kim @ 2018-12-23 3:35 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh,
Tom Zanussi, kernel-team
On Sat, Dec 22, 2018 at 11:20:11AM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> As str_has_prefix() returns the length on match, we can use that for the
> updating of the string pointer instead of recalculating the prefix size.
>
> Cc: Tom Zanussi <zanussi@kernel.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> kernel/trace/trace_events_hist.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 0d878dcd1e4b..449d90cfa151 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4342,12 +4342,13 @@ static int parse_actions(struct hist_trigger_data *hist_data)
> unsigned int i;
> int ret = 0;
> char *str;
> + int len;
>
> for (i = 0; i < hist_data->attrs->n_actions; i++) {
> str = hist_data->attrs->action_str[i];
>
> - if (str_has_prefix(str, "onmatch(")) {
> - char *action_str = str + sizeof("onmatch(") - 1;
> + if ((len = str_has_prefix(str, "onmatch("))) {
> + char *action_str = str + len;
Ah you did it here.
Thanks,
Namhyung
>
> data = onmatch_parse(tr, action_str);
> if (IS_ERR(data)) {
> @@ -4355,8 +4356,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)
> break;
> }
> data->fn = action_trace;
> - } else if (str_has_prefix(str, "onmax(")) {
> - char *action_str = str + sizeof("onmax(") - 1;
> + } else if ((len = str_has_prefix(str, "onmax("))) {
> + char *action_str = str + len;
>
> data = onmax_parse(action_str);
> if (IS_ERR(data)) {
> --
> 2.19.2
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix
2018-12-22 16:20 ` [for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix Steven Rostedt
2018-12-23 3:35 ` Namhyung Kim
@ 2018-12-23 16:40 ` Tom Zanussi
1 sibling, 0 replies; 19+ messages in thread
From: Tom Zanussi @ 2018-12-23 16:40 UTC (permalink / raw)
To: Steven Rostedt, linux-kernel
Cc: Ingo Molnar, Andrew Morton, Joe Perches, Namhyung Kim,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh
Hi Steve,
In the subject line, s/historgram/histogram...
On Sat, 2018-12-22 at 11:20 -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> As str_has_prefix() returns the length on match, we can use that for
> the
> updating of the string pointer instead of recalculating the prefix
> size.
>
> Cc: Tom Zanussi <zanussi@kernel.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> kernel/trace/trace_events_hist.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c
> b/kernel/trace/trace_events_hist.c
> index 0d878dcd1e4b..449d90cfa151 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -4342,12 +4342,13 @@ static int parse_actions(struct
> hist_trigger_data *hist_data)
> unsigned int i;
> int ret = 0;
> char *str;
> + int len;
>
> for (i = 0; i < hist_data->attrs->n_actions; i++) {
> str = hist_data->attrs->action_str[i];
>
> - if (str_has_prefix(str, "onmatch(")) {
> - char *action_str = str + sizeof("onmatch(")
> - 1;
> + if ((len = str_has_prefix(str, "onmatch("))) {
> + char *action_str = str + len;
>
> data = onmatch_parse(tr, action_str);
> if (IS_ERR(data)) {
> @@ -4355,8 +4356,8 @@ static int parse_actions(struct
> hist_trigger_data *hist_data)
> break;
> }
> data->fn = action_trace;
> - } else if (str_has_prefix(str, "onmax(")) {
> - char *action_str = str + sizeof("onmax(") -
> 1;
> + } else if ((len = str_has_prefix(str, "onmax("))) {
> + char *action_str = str + len;
>
> data = onmax_parse(action_str);
> if (IS_ERR(data)) {
Acked-by: Tom Zanussi <tom.zanussi@linux.intel.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers
2018-12-22 16:20 [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages Steven Rostedt
` (3 preceding siblings ...)
2018-12-22 16:20 ` [for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix Steven Rostedt
@ 2018-12-22 16:20 ` Steven Rostedt
2018-12-23 3:23 ` Masami Hiramatsu
2018-12-23 0:55 ` [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages Steven Rostedt
2018-12-23 3:41 ` Namhyung Kim
6 siblings, 1 reply; 19+ messages in thread
From: Steven Rostedt @ 2018-12-22 16:20 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Joe Perches, Namhyung Kim,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh,
Masami Hiramatsu
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
There are several locations that compare constants to the beginning of
string variables to determine what commands should be done, then the
constant length is used to index into the string. This is error prone as the
hard coded numbers have to match the size of the constants. Instead, use the
len returned from str_has_prefix() and remove the open coded string length
sizes.
Cc: Joe Perches <joe@perches.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
kernel/trace/trace.c | 8 +++++---
kernel/trace/trace_probe.c | 17 +++++++++--------
kernel/trace/trace_stack.c | 6 ++++--
3 files changed, 18 insertions(+), 13 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index eac2824a18ab..18b86c3974e1 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -4408,13 +4408,15 @@ static int trace_set_options(struct trace_array *tr, char *option)
int neg = 0;
int ret;
size_t orig_len = strlen(option);
+ int len;
cmp = strstrip(option);
- if (str_has_prefix(cmp, "no")) {
+ len = str_has_prefix(cmp, "no");
+ if (len)
neg = 1;
- cmp += 2;
- }
+
+ cmp += len;
mutex_lock(&trace_types_lock);
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 541375737403..9962cb5da8ac 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -186,19 +186,20 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
static int parse_probe_vars(char *arg, const struct fetch_type *t,
struct fetch_insn *code, unsigned int flags)
{
- int ret = 0;
unsigned long param;
+ int ret = 0;
+ int len;
if (strcmp(arg, "retval") == 0) {
if (flags & TPARG_FL_RETURN)
code->op = FETCH_OP_RETVAL;
else
ret = -EINVAL;
- } else if (str_has_prefix(arg, "stack")) {
- if (arg[5] == '\0') {
+ } else if ((len = str_has_prefix(arg, "stack"))) {
+ if (arg[len] == '\0') {
code->op = FETCH_OP_STACKP;
- } else if (isdigit(arg[5])) {
- ret = kstrtoul(arg + 5, 10, ¶m);
+ } else if (isdigit(arg[len])) {
+ ret = kstrtoul(arg + len, 10, ¶m);
if (ret || ((flags & TPARG_FL_KERNEL) &&
param > PARAM_MAX_STACK))
ret = -EINVAL;
@@ -213,10 +214,10 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
} else if (((flags & TPARG_FL_MASK) ==
(TPARG_FL_KERNEL | TPARG_FL_FENTRY)) &&
- str_has_prefix(arg, "arg")) {
- if (!isdigit(arg[3]))
+ (len = str_has_prefix(arg, "arg"))) {
+ if (!isdigit(arg[len]))
return -EINVAL;
- ret = kstrtoul(arg + 3, 10, ¶m);
+ ret = kstrtoul(arg + len, 10, ¶m);
if (ret || !param || param > PARAM_MAX_STACK)
return -EINVAL;
code->op = FETCH_OP_ARG;
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 3641f28c343f..eec648a0d673 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -448,8 +448,10 @@ static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata;
static __init int enable_stacktrace(char *str)
{
- if (str_has_prefix(str, "_filter="))
- strncpy(stack_trace_filter_buf, str+8, COMMAND_LINE_SIZE);
+ int len;
+
+ if ((len = str_has_prefix(str, "_filter=")))
+ strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE);
stack_tracer_enabled = 1;
last_stack_tracer_enabled = 1;
--
2.19.2
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers
2018-12-22 16:20 ` [for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers Steven Rostedt
@ 2018-12-23 3:23 ` Masami Hiramatsu
2018-12-23 3:46 ` Steven Rostedt
0 siblings, 1 reply; 19+ messages in thread
From: Masami Hiramatsu @ 2018-12-23 3:23 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Namhyung Kim, Linus Torvalds, Yoshinori Sato, Rich Felker,
linux-sh, Masami Hiramatsu
On Sat, 22 Dec 2018 11:20:12 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> There are several locations that compare constants to the beginning of
> string variables to determine what commands should be done, then the
> constant length is used to index into the string. This is error prone as the
> hard coded numbers have to match the size of the constants. Instead, use the
> len returned from str_has_prefix() and remove the open coded string length
> sizes.
Looks good to me.
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
for trace_probe part.
Thanks!
>
> Cc: Joe Perches <joe@perches.com>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
> kernel/trace/trace.c | 8 +++++---
> kernel/trace/trace_probe.c | 17 +++++++++--------
> kernel/trace/trace_stack.c | 6 ++++--
> 3 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index eac2824a18ab..18b86c3974e1 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -4408,13 +4408,15 @@ static int trace_set_options(struct trace_array *tr, char *option)
> int neg = 0;
> int ret;
> size_t orig_len = strlen(option);
> + int len;
>
> cmp = strstrip(option);
>
> - if (str_has_prefix(cmp, "no")) {
> + len = str_has_prefix(cmp, "no");
> + if (len)
> neg = 1;
> - cmp += 2;
> - }
> +
> + cmp += len;
>
> mutex_lock(&trace_types_lock);
>
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 541375737403..9962cb5da8ac 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -186,19 +186,20 @@ int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
> static int parse_probe_vars(char *arg, const struct fetch_type *t,
> struct fetch_insn *code, unsigned int flags)
> {
> - int ret = 0;
> unsigned long param;
> + int ret = 0;
> + int len;
>
> if (strcmp(arg, "retval") == 0) {
> if (flags & TPARG_FL_RETURN)
> code->op = FETCH_OP_RETVAL;
> else
> ret = -EINVAL;
> - } else if (str_has_prefix(arg, "stack")) {
> - if (arg[5] == '\0') {
> + } else if ((len = str_has_prefix(arg, "stack"))) {
> + if (arg[len] == '\0') {
> code->op = FETCH_OP_STACKP;
> - } else if (isdigit(arg[5])) {
> - ret = kstrtoul(arg + 5, 10, ¶m);
> + } else if (isdigit(arg[len])) {
> + ret = kstrtoul(arg + len, 10, ¶m);
> if (ret || ((flags & TPARG_FL_KERNEL) &&
> param > PARAM_MAX_STACK))
> ret = -EINVAL;
> @@ -213,10 +214,10 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
> #ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
> } else if (((flags & TPARG_FL_MASK) ==
> (TPARG_FL_KERNEL | TPARG_FL_FENTRY)) &&
> - str_has_prefix(arg, "arg")) {
> - if (!isdigit(arg[3]))
> + (len = str_has_prefix(arg, "arg"))) {
> + if (!isdigit(arg[len]))
> return -EINVAL;
> - ret = kstrtoul(arg + 3, 10, ¶m);
> + ret = kstrtoul(arg + len, 10, ¶m);
> if (ret || !param || param > PARAM_MAX_STACK)
> return -EINVAL;
> code->op = FETCH_OP_ARG;
> diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
> index 3641f28c343f..eec648a0d673 100644
> --- a/kernel/trace/trace_stack.c
> +++ b/kernel/trace/trace_stack.c
> @@ -448,8 +448,10 @@ static char stack_trace_filter_buf[COMMAND_LINE_SIZE+1] __initdata;
>
> static __init int enable_stacktrace(char *str)
> {
> - if (str_has_prefix(str, "_filter="))
> - strncpy(stack_trace_filter_buf, str+8, COMMAND_LINE_SIZE);
> + int len;
> +
> + if ((len = str_has_prefix(str, "_filter=")))
> + strncpy(stack_trace_filter_buf, str + len, COMMAND_LINE_SIZE);
>
> stack_tracer_enabled = 1;
> last_stack_tracer_enabled = 1;
> --
> 2.19.2
>
>
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers
2018-12-23 3:23 ` Masami Hiramatsu
@ 2018-12-23 3:46 ` Steven Rostedt
0 siblings, 0 replies; 19+ messages in thread
From: Steven Rostedt @ 2018-12-23 3:46 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Namhyung Kim, Linus Torvalds, Yoshinori Sato, Rich Felker,
linux-sh
On Sun, 23 Dec 2018 12:23:52 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> On Sat, 22 Dec 2018 11:20:12 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> >
> > There are several locations that compare constants to the beginning of
> > string variables to determine what commands should be done, then the
> > constant length is used to index into the string. This is error prone as the
> > hard coded numbers have to match the size of the constants. Instead, use the
> > len returned from str_has_prefix() and remove the open coded string length
> > sizes.
>
> Looks good to me.
>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
>
> for trace_probe part.
Thanks Masami!
-- Steve
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages
2018-12-22 16:20 [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages Steven Rostedt
` (4 preceding siblings ...)
2018-12-22 16:20 ` [for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers Steven Rostedt
@ 2018-12-23 0:55 ` Steven Rostedt
2018-12-23 3:41 ` Namhyung Kim
6 siblings, 0 replies; 19+ messages in thread
From: Steven Rostedt @ 2018-12-23 0:55 UTC (permalink / raw)
To: linux-kernel
Cc: Ingo Molnar, Andrew Morton, Joe Perches, Namhyung Kim,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh
On Sat, 22 Dec 2018 11:20:07 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> They ran through all my tests, althought zero-day-bot had a weird build
> regression in sh, that looks totally unrelated:
>
> Regressions in current branch:
>
> arch/sh/kernel/dwarf.c:107:26: error: 'dwarf_frame_reg' defined but not used [-Werror=unused-function]
> arch/sh/kernel/dwarf.c:1209:0: error: unterminated argument list invoking macro "WARN_ON"
> arch/sh/kernel/dwarf.c:226:12: error: 'dwarf_read_encoded_value' defined but not used [-Werror=unused-function]
> arch/sh/kernel/dwarf.c:306:26: error: 'dwarf_lookup_cie' defined but not used [-Werror=unused-function]
> arch/sh/kernel/dwarf.c:38:27: error: 'dwarf_frame_cachep' defined but not used [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:399:12: error: 'dwarf_cfa_execute_insns' defined but not used [-Werror=unused-function]
> arch/sh/kernel/dwarf.c:41:27: error: 'dwarf_reg_cachep' defined but not used [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:580:22: error: unused variable 'frame' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:581:20: error: unused variable 'cie' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:582:20: error: unused variable 'fde' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:583:20: error: unused variable 'reg' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:584:16: error: unused variable 'addr' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:622:3: error: expected ';' at end of input
> arch/sh/kernel/dwarf.c:622:3: error: expected declaration or statement at end of input
> arch/sh/kernel/dwarf.c:622:3: error: 'WARN_ON' undeclared (first use in this function); did you mean 'WMARK_LOW'?
>
> Pushing to my for-next branch should kick off another run. Let's see
> if it reports that again. Unless someone knows why that happened?
FYI,
Zeroday reported back a successful run of my for-next branch, and did
it again, after I pushed a rebase that added an Acked-by. Thus, I'm
guessing that the above is a simple fluke.
-- Steve
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages
2018-12-22 16:20 [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages Steven Rostedt
` (5 preceding siblings ...)
2018-12-23 0:55 ` [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages Steven Rostedt
@ 2018-12-23 3:41 ` Namhyung Kim
2018-12-23 3:53 ` Steven Rostedt
6 siblings, 1 reply; 19+ messages in thread
From: Namhyung Kim @ 2018-12-23 3:41 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh,
kernel-team
On Sat, Dec 22, 2018 at 11:20:07AM -0500, Steven Rostedt wrote:
> I hope everyone is OK with these changes. I pushed to linux-next but I'm
> willing to change them if there are still issues.
>
> They ran through all my tests, althought zero-day-bot had a weird build
> regression in sh, that looks totally unrelated:
>
> Regressions in current branch:
>
> arch/sh/kernel/dwarf.c:107:26: error: 'dwarf_frame_reg' defined but not used [-Werror=unused-function]
> arch/sh/kernel/dwarf.c:1209:0: error: unterminated argument list invoking macro "WARN_ON"
> arch/sh/kernel/dwarf.c:226:12: error: 'dwarf_read_encoded_value' defined but not used [-Werror=unused-function]
> arch/sh/kernel/dwarf.c:306:26: error: 'dwarf_lookup_cie' defined but not used [-Werror=unused-function]
> arch/sh/kernel/dwarf.c:38:27: error: 'dwarf_frame_cachep' defined but not used [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:399:12: error: 'dwarf_cfa_execute_insns' defined but not used [-Werror=unused-function]
> arch/sh/kernel/dwarf.c:41:27: error: 'dwarf_reg_cachep' defined but not used [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:580:22: error: unused variable 'frame' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:581:20: error: unused variable 'cie' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:582:20: error: unused variable 'fde' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:583:20: error: unused variable 'reg' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:584:16: error: unused variable 'addr' [-Werror=unused-variable]
> arch/sh/kernel/dwarf.c:622:3: error: expected ';' at end of input
> arch/sh/kernel/dwarf.c:622:3: error: expected declaration or statement at end of input
> arch/sh/kernel/dwarf.c:622:3: error: 'WARN_ON' undeclared (first use in this function); did you mean 'WMARK_LOW'?
>
> Pushing to my for-next branch should kick off another run. Let's see
> if it reports that again. Unless someone knows why that happened?
>
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
> for-next
>
> Head SHA1: 92b9de3e574bd874188a4e27a8830bb901a08ef8
>
>
> Steven Rostedt (VMware) (5):
> string.h: Add str_has_prefix() helper function
> tracing: Use str_has_prefix() helper for histogram code
> tracing: Use str_has_prefix() instead of using fixed sizes
> tracing: Have the historgram use the result of str_has_prefix() for len of prefix
> tracing: Use the return of str_has_prefix() to remove open coded numbers
For the series:
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
>
> ----
> include/linux/string.h | 20 ++++++++++++++++++++
> kernel/trace/trace.c | 8 +++++---
> kernel/trace/trace_events.c | 2 +-
> kernel/trace/trace_events_hist.c | 35 ++++++++++++++++++-----------------
> kernel/trace/trace_probe.c | 17 +++++++++--------
> kernel/trace/trace_stack.c | 6 ++++--
> 6 files changed, 57 insertions(+), 31 deletions(-)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages
2018-12-23 3:41 ` Namhyung Kim
@ 2018-12-23 3:53 ` Steven Rostedt
0 siblings, 0 replies; 19+ messages in thread
From: Steven Rostedt @ 2018-12-23 3:53 UTC (permalink / raw)
To: Namhyung Kim
Cc: linux-kernel, Ingo Molnar, Andrew Morton, Joe Perches,
Linus Torvalds, Yoshinori Sato, Rich Felker, linux-sh,
kernel-team
On Sun, 23 Dec 2018 12:41:20 +0900
Namhyung Kim <namhyung@kernel.org> wrote:
> > Steven Rostedt (VMware) (5):
> > string.h: Add str_has_prefix() helper function
> > tracing: Use str_has_prefix() helper for histogram code
> > tracing: Use str_has_prefix() instead of using fixed sizes
> > tracing: Have the historgram use the result of str_has_prefix() for len of prefix
> > tracing: Use the return of str_has_prefix() to remove open coded numbers
>
> For the series:
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks Namhyung!
-- Steve
^ permalink raw reply [flat|nested] 19+ messages in thread