[for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages

[for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages

[Steven Rostedt]

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?

-- Steve



  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

----
 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(-)

[for-next][PATCH 1/5] string.h: Add str_has_prefix() helper function

[Steven Rostedt]

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

[for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code

[Steven Rostedt]

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

[for-next][PATCH 3/5] tracing: Use str_has_prefix() instead of using fixed sizes

[Steven Rostedt]

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, &param);
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

[for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix

[Steven Rostedt]

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

[for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers

[Steven Rostedt]

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, &param);
+		} else if (isdigit(arg[len])) {
+			ret = kstrtoul(arg + len, 10, &param);
 			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, &param);
+		ret = kstrtoul(arg + len, 10, &param);
 		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

Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code

[Steven Rostedt]

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);

Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code

[Steven Rostedt]

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

Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code

[Tom Zanussi]

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);
> 
> 

Re: [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages

[Steven Rostedt]

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

Re: [for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers

[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, &param);
> +		} else if (isdigit(arg[len])) {
> +			ret = kstrtoul(arg + len, 10, &param);
>  			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, &param);
> +		ret = kstrtoul(arg + len, 10, &param);
>  		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>

Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code

[Namhyung Kim]

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
> 
> 

Re: [for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix

[Namhyung Kim]

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
> 
> 

Re: [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages

[Namhyung Kim]

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(-)

Re: [for-next][PATCH 5/5] tracing: Use the return of str_has_prefix() to remove open coded numbers

[Steven Rostedt]

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

Re: [for-next][PATCH 0/5] tracing: Add string_has_prefix() and usages

[Steven Rostedt]

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

Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code

[Steven Rostedt]

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

Re: [for-next][PATCH 2/5] tracing: Use str_has_prefix() helper for histogram code

[Tom Zanussi]

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
> 

Re: [for-next][PATCH 4/5] tracing: Have the historgram use the result of str_has_prefix() for len of prefix

[Tom Zanussi]

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>