[PATCH 0/7] tracing: Hist trigger var-ref cleanup and comments

[PATCH 0/7] tracing: Hist trigger var-ref cleanup and comments

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

Hi,

This patchset is a standalone series broken out of the v8 version of
the 'tracing: Hist trigger snapshot and onchange additions' patchset.

It's a series of changes resulting from some suggestions from Namhyung
for making the variable-reference handling code more understandable
through some refactoring and comments.

It also added a new patch changing all strlen() to sizeof() for string
constants, in trace_events_hist.c

Also, in the 'tracing: Remove open-coding of hist trigger var_ref
management' patch, in create_var_ref(), moved the saving of ref_field
and update of ref_field->var_ref_idx into the 'if' as pointed out by
Dan Carpenter/smatch 0-day robot.

It doesn't introduce any functional changes and can be applied
independently of the other patchset.

Tom


The following changes since commit 5d6ddf6acce68d1290112cb08b12fd78b201e7d5:

  arm64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack (2018-12-08 22:21:31 -0500)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/hist-var-ref-cleanup-v1

Tom Zanussi (7):
  tracing: Remove unnecessary hist trigger struct field
  tracing: Change strlen to sizeof for hist trigger static strings
  tracing: Use var_refs[] for hist trigger reference checking
  tracing: Remove open-coding of hist trigger var_ref management
  tracing: Use hist trigger's var_ref array to destroy var_refs
  tracing: Remove hist trigger synth_var_refs
  tracing: Add hist trigger comments for variable-related fields

 kernel/trace/trace_events_hist.c | 267 +++++++++++++++++++++++----------------
 1 file changed, 156 insertions(+), 111 deletions(-)

-- 
2.14.1

[PATCH 1/7] tracing: Remove unnecessary hist trigger struct field

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

hist_field.var_idx is completely unused, so remove it.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 82e72c48a5a9..d29bf8a8e1dd 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -61,7 +61,6 @@ struct hist_field {
 	char				*system;
 	char				*event_name;
 	char				*name;
-	unsigned int			var_idx;
 	unsigned int			var_ref_idx;
 	bool                            read_once;
 };
-- 
2.14.1

[PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

There's no need to use strlen() for static strings when the length is
already known, so update trace_events_hist.c with sizeof() for those
cases.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index d29bf8a8e1dd..25d06b3ae1f6 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -507,7 +507,7 @@ static int synth_field_string_size(char *type)
 	start = strstr(type, "char[");
 	if (start == NULL)
 		return -EINVAL;
-	start += strlen("char[");
+	start += sizeof("char[") - 1;
 
 	end = strchr(type, ']');
 	if (!end || end < start)
@@ -1843,8 +1843,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(", strlen("onmatch(")) == 0) ||
-	    (strncmp(str, "onmax(", strlen("onmax(")) == 0)) {
+	if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) ||
+	    (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
 		attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
 		if (!attrs->action_str[attrs->n_actions]) {
 			ret = -ENOMEM;
@@ -1861,34 +1861,34 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
 {
 	int ret = 0;
 
-	if ((strncmp(str, "key=", strlen("key=")) == 0) ||
-	    (strncmp(str, "keys=", strlen("keys=")) == 0)) {
+	if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
+	    (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
 		attrs->keys_str = kstrdup(str, GFP_KERNEL);
 		if (!attrs->keys_str) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if ((strncmp(str, "val=", strlen("val=")) == 0) ||
-		 (strncmp(str, "vals=", strlen("vals=")) == 0) ||
-		 (strncmp(str, "values=", strlen("values=")) == 0)) {
+	} else if ((strncmp(str, "val=", sizeof("val=") - 1) == 0) ||
+		 (strncmp(str, "vals=", sizeof("vals=") - 1) == 0) ||
+		 (strncmp(str, "values=", sizeof("values=") - 1) == 0)) {
 		attrs->vals_str = kstrdup(str, GFP_KERNEL);
 		if (!attrs->vals_str) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "sort=", strlen("sort=")) == 0) {
+	} else if (strncmp(str, "sort=", sizeof("sort=") - 1) == 0) {
 		attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
 		if (!attrs->sort_key_str) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "name=", strlen("name=")) == 0) {
+	} else if (strncmp(str, "name=", sizeof("name=") - 1) == 0) {
 		attrs->name = kstrdup(str, GFP_KERNEL);
 		if (!attrs->name) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "clock=", strlen("clock=")) == 0) {
+	} else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
 		strsep(&str, "=");
 		if (!str) {
 			ret = -EINVAL;
@@ -1901,7 +1901,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "size=", strlen("size=")) == 0) {
+	} else if (strncmp(str, "size=", sizeof("size=") - 1) == 0) {
 		int map_bits = parse_map_size(str);
 
 		if (map_bits < 0) {
@@ -3497,7 +3497,7 @@ static struct action_data *onmax_parse(char *str)
 	if (!onmax_fn_name || !str)
 		goto free;
 
-	if (strncmp(onmax_fn_name, "save", strlen("save")) == 0) {
+	if (strncmp(onmax_fn_name, "save", sizeof("save") - 1) == 0) {
 		char *params = strsep(&str, ")");
 
 		if (!params) {
@@ -4302,8 +4302,8 @@ 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(", strlen("onmatch(")) == 0) {
-			char *action_str = str + strlen("onmatch(");
+		if (strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) {
+			char *action_str = str + sizeof("onmatch(") - 1;
 
 			data = onmatch_parse(tr, action_str);
 			if (IS_ERR(data)) {
@@ -4311,8 +4311,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)
 				break;
 			}
 			data->fn = action_trace;
-		} else if (strncmp(str, "onmax(", strlen("onmax(")) == 0) {
-			char *action_str = str + strlen("onmax(");
+		} else if (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0) {
+			char *action_str = str + sizeof("onmax(") - 1;
 
 			data = onmax_parse(action_str);
 			if (IS_ERR(data)) {
@@ -5548,9 +5548,9 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
 			p++;
 			continue;
 		}
-		if (p >= param + strlen(param) - strlen("if") - 1)
+		if (p >= param + strlen(param) - (sizeof("if") - 1) - 1)
 			return -EINVAL;
-		if (*(p + strlen("if")) != ' ' && *(p + strlen("if")) != '\t') {
+		if (*(p + sizeof("if") - 1) != ' ' && *(p + sizeof("if") - 1) != '\t') {
 			p++;
 			continue;
 		}
-- 
2.14.1

[PATCH 3/7] tracing: Use var_refs[] for hist trigger reference checking

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

Since all the variable reference hist_fields are collected into
hist_data->var_refs[] array, there's no need to go through all the
fields looking for them, or in separate arrays like synth_var_refs[],
which will be going away soon anyway.

This also allows us to get rid of some unnecessary code and functions
currently used for the same purpose.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 57 +++++-----------------------------------
 1 file changed, 7 insertions(+), 50 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 25d06b3ae1f6..ee839c52bd3f 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1299,45 +1299,11 @@ check_field_for_var_ref(struct hist_field *hist_field,
 {
 	struct hist_field *found = NULL;
 
-	if (hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF) {
-		if (hist_field->var.idx == var_idx &&
-		    hist_field->var.hist_data == var_data) {
-			found = hist_field;
-		}
-	}
-
-	return found;
-}
-
-static struct hist_field *
-check_field_for_var_refs(struct hist_trigger_data *hist_data,
-			 struct hist_field *hist_field,
-			 struct hist_trigger_data *var_data,
-			 unsigned int var_idx,
-			 unsigned int level)
-{
-	struct hist_field *found = NULL;
-	unsigned int i;
-
-	if (level > 3)
-		return found;
-
-	if (!hist_field)
-		return found;
+	WARN_ON(!(hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF));
 
-	found = check_field_for_var_ref(hist_field, var_data, var_idx);
-	if (found)
-		return found;
-
-	for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++) {
-		struct hist_field *operand;
-
-		operand = hist_field->operands[i];
-		found = check_field_for_var_refs(hist_data, operand, var_data,
-						 var_idx, level + 1);
-		if (found)
-			return found;
-	}
+	if (hist_field && hist_field->var.idx == var_idx &&
+	    hist_field->var.hist_data == var_data)
+		found = hist_field;
 
 	return found;
 }
@@ -1349,18 +1315,9 @@ static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
 	struct hist_field *hist_field, *found = NULL;
 	unsigned int i;
 
-	for_each_hist_field(i, hist_data) {
-		hist_field = hist_data->fields[i];
-		found = check_field_for_var_refs(hist_data, hist_field,
-						 var_data, var_idx, 0);
-		if (found)
-			return found;
-	}
-
-	for (i = 0; i < hist_data->n_synth_var_refs; i++) {
-		hist_field = hist_data->synth_var_refs[i];
-		found = check_field_for_var_refs(hist_data, hist_field,
-						 var_data, var_idx, 0);
+	for (i = 0; i < hist_data->n_var_refs; i++) {
+		hist_field = hist_data->var_refs[i];
+		found = check_field_for_var_ref(hist_field, var_data, var_idx);
 		if (found)
 			return found;
 	}
-- 
2.14.1

[PATCH 4/7] tracing: Remove open-coding of hist trigger var_ref management

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

Have create_var_ref() manage the hist trigger's var_ref list, rather
than having similar code doing it in multiple places.  This cleans up
the code and makes sure var_refs are always accounted properly.

Also, document the var_ref-related functions to make what their
purpose clearer.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 93 ++++++++++++++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 18 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index ee839c52bd3f..9d4cf7a8a95b 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1292,6 +1292,17 @@ static u64 hist_field_cpu(struct hist_field *hist_field,
 	return cpu;
 }
 
+/**
+ * check_field_for_var_ref - Check if a VAR_REF field references a variable
+ * @hist_field: The VAR_REF field to check
+ * @var_data: The hist trigger that owns the variable
+ * @var_idx: The trigger variable identifier
+ *
+ * Check the given VAR_REF field to see whether or not it references
+ * the given variable associated with the given trigger.
+ *
+ * Return: The VAR_REF field if it does reference the variable, NULL if not
+ */
 static struct hist_field *
 check_field_for_var_ref(struct hist_field *hist_field,
 			struct hist_trigger_data *var_data,
@@ -1308,6 +1319,18 @@ check_field_for_var_ref(struct hist_field *hist_field,
 	return found;
 }
 
+/**
+ * find_var_ref - Check if a trigger has a reference to a trigger variable
+ * @hist_data: The hist trigger that might have a reference to the variable
+ * @var_data: The hist trigger that owns the variable
+ * @var_idx: The trigger variable identifier
+ *
+ * Check the list of var_refs[] on the first hist trigger to see
+ * whether any of them are references to the variable on the second
+ * trigger.
+ *
+ * Return: The VAR_REF field referencing the variable if so, NULL if not
+ */
 static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
 				       struct hist_trigger_data *var_data,
 				       unsigned int var_idx)
@@ -1325,6 +1348,20 @@ static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
 	return found;
 }
 
+/**
+ * find_any_var_ref - Check if there is a reference to a given trigger variable
+ * @hist_data: The hist trigger
+ * @var_idx: The trigger variable identifier
+ *
+ * Check to see whether the given variable is currently referenced by
+ * any other trigger.
+ *
+ * The trigger the variable is defined on is explicitly excluded - the
+ * assumption being that a self-reference doesn't prevent a trigger
+ * from being removed.
+ *
+ * Return: The VAR_REF field referencing the variable if so, NULL if not
+ */
 static struct hist_field *find_any_var_ref(struct hist_trigger_data *hist_data,
 					   unsigned int var_idx)
 {
@@ -1343,6 +1380,19 @@ static struct hist_field *find_any_var_ref(struct hist_trigger_data *hist_data,
 	return found;
 }
 
+/**
+ * check_var_refs - Check if there is a reference to any of trigger's variables
+ * @hist_data: The hist trigger
+ *
+ * A trigger can define one or more variables.  If any one of them is
+ * currently referenced by any other trigger, this function will
+ * determine that.
+
+ * Typically used to determine whether or not a trigger can be removed
+ * - if there are any references to a trigger's variables, it cannot.
+ *
+ * Return: True if there is a reference to any of trigger's variables
+ */
 static bool check_var_refs(struct hist_trigger_data *hist_data)
 {
 	struct hist_field *field;
@@ -2346,7 +2396,23 @@ static int init_var_ref(struct hist_field *ref_field,
 	goto out;
 }
 
-static struct hist_field *create_var_ref(struct hist_field *var_field,
+/**
+ * create_var_ref - Create a variable reference and attach it to trigger
+ * @hist_data: The trigger that will be referencing the variable
+ * @var_field: The VAR field to create a reference to
+ * @system: The optional system string
+ * @event_name: The optional event_name string
+ *
+ * Given a variable hist_field, create a VAR_REF hist_field that
+ * represents a reference to it.
+ *
+ * This function also adds the reference to the trigger that
+ * now references the variable.
+ *
+ * Return: The VAR_REF field if successful, NULL if not
+ */
+static struct hist_field *create_var_ref(struct hist_trigger_data *hist_data,
+					 struct hist_field *var_field,
 					 char *system, char *event_name)
 {
 	unsigned long flags = HIST_FIELD_FL_VAR_REF;
@@ -2358,6 +2424,9 @@ static struct hist_field *create_var_ref(struct hist_field *var_field,
 			destroy_hist_field(ref_field, 0);
 			return NULL;
 		}
+
+		hist_data->var_refs[hist_data->n_var_refs] = ref_field;
+		ref_field->var_ref_idx = hist_data->n_var_refs++;
 	}
 
 	return ref_field;
@@ -2431,7 +2500,8 @@ static struct hist_field *parse_var_ref(struct hist_trigger_data *hist_data,
 
 	var_field = find_event_var(hist_data, system, event_name, var_name);
 	if (var_field)
-		ref_field = create_var_ref(var_field, system, event_name);
+		ref_field = create_var_ref(hist_data, var_field,
+					   system, event_name);
 
 	if (!ref_field)
 		hist_err_event("Couldn't find variable: $",
@@ -2549,8 +2619,6 @@ static struct hist_field *parse_atom(struct hist_trigger_data *hist_data,
 	if (!s) {
 		hist_field = parse_var_ref(hist_data, ref_system, ref_event, ref_var);
 		if (hist_field) {
-			hist_data->var_refs[hist_data->n_var_refs] = hist_field;
-			hist_field->var_ref_idx = hist_data->n_var_refs++;
 			if (var_name) {
 				hist_field = create_alias(hist_data, hist_field, var_name);
 				if (!hist_field) {
@@ -3324,7 +3392,6 @@ static int onmax_create(struct hist_trigger_data *hist_data,
 	unsigned int var_ref_idx = hist_data->n_var_refs;
 	struct field_var *field_var;
 	char *onmax_var_str, *param;
-	unsigned long flags;
 	unsigned int i;
 	int ret = 0;
 
@@ -3341,18 +3408,10 @@ static int onmax_create(struct hist_trigger_data *hist_data,
 		return -EINVAL;
 	}
 
-	flags = HIST_FIELD_FL_VAR_REF;
-	ref_field = create_hist_field(hist_data, NULL, flags, NULL);
+	ref_field = create_var_ref(hist_data, var_field, NULL, NULL);
 	if (!ref_field)
 		return -ENOMEM;
 
-	if (init_var_ref(ref_field, var_field, NULL, NULL)) {
-		destroy_hist_field(ref_field, 0);
-		ret = -ENOMEM;
-		goto out;
-	}
-	hist_data->var_refs[hist_data->n_var_refs] = ref_field;
-	ref_field->var_ref_idx = hist_data->n_var_refs++;
 	data->onmax.var = ref_field;
 
 	data->fn = onmax_save;
@@ -3541,9 +3600,6 @@ static void save_synth_var_ref(struct hist_trigger_data *hist_data,
 			 struct hist_field *var_ref)
 {
 	hist_data->synth_var_refs[hist_data->n_synth_var_refs++] = var_ref;
-
-	hist_data->var_refs[hist_data->n_var_refs] = var_ref;
-	var_ref->var_ref_idx = hist_data->n_var_refs++;
 }
 
 static int check_synth_field(struct synth_event *event,
@@ -3697,7 +3753,8 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
 		}
 
 		if (check_synth_field(event, hist_field, field_pos) == 0) {
-			var_ref = create_var_ref(hist_field, system, event_name);
+			var_ref = create_var_ref(hist_data, hist_field,
+						 system, event_name);
 			if (!var_ref) {
 				kfree(p);
 				ret = -ENOMEM;
-- 
2.14.1

[PATCH 5/7] tracing: Use hist trigger's var_ref array to destroy var_refs

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

Since every var ref for a trigger has an entry in the var_ref[] array,
use that to destroy the var_refs, instead of piecemeal via the field
expressions.

This allows us to avoid having to keep and treat differently separate
lists for the action-related references, which future patches will
remove.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 9d4cf7a8a95b..725d22f5372f 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -2193,6 +2193,15 @@ static int contains_operator(char *str)
 	return field_op;
 }
 
+static void __destroy_hist_field(struct hist_field *hist_field)
+{
+	kfree(hist_field->var.name);
+	kfree(hist_field->name);
+	kfree(hist_field->type);
+
+	kfree(hist_field);
+}
+
 static void destroy_hist_field(struct hist_field *hist_field,
 			       unsigned int level)
 {
@@ -2204,14 +2213,13 @@ static void destroy_hist_field(struct hist_field *hist_field,
 	if (!hist_field)
 		return;
 
+	if (hist_field->flags & HIST_FIELD_FL_VAR_REF)
+		return; /* var refs will be destroyed separately */
+
 	for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++)
 		destroy_hist_field(hist_field->operands[i], level + 1);
 
-	kfree(hist_field->var.name);
-	kfree(hist_field->name);
-	kfree(hist_field->type);
-
-	kfree(hist_field);
+	__destroy_hist_field(hist_field);
 }
 
 static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data,
@@ -2338,6 +2346,12 @@ static void destroy_hist_fields(struct hist_trigger_data *hist_data)
 			hist_data->fields[i] = NULL;
 		}
 	}
+
+	for (i = 0; i < hist_data->n_var_refs; i++) {
+		WARN_ON(!(hist_data->var_refs[i]->flags & HIST_FIELD_FL_VAR_REF));
+		__destroy_hist_field(hist_data->var_refs[i]);
+		hist_data->var_refs[i] = NULL;
+	}
 }
 
 static int init_var_ref(struct hist_field *ref_field,
-- 
2.14.1

[PATCH 6/7] tracing: Remove hist trigger synth_var_refs

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

All var_refs are now handled uniformly and there's no reason to treat
the synth_refs in a special way now, so remove them and associated
functions.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 725d22f5372f..e66dd764e057 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -279,8 +279,6 @@ struct hist_trigger_data {
 	struct action_data		*actions[HIST_ACTIONS_MAX];
 	unsigned int			n_actions;
 
-	struct hist_field               *synth_var_refs[SYNTH_FIELDS_MAX];
-	unsigned int                    n_synth_var_refs;
 	struct field_var		*field_vars[SYNTH_FIELDS_MAX];
 	unsigned int			n_field_vars;
 	unsigned int			n_field_var_str;
@@ -3602,20 +3600,6 @@ static void save_field_var(struct hist_trigger_data *hist_data,
 }
 
 
-static void destroy_synth_var_refs(struct hist_trigger_data *hist_data)
-{
-	unsigned int i;
-
-	for (i = 0; i < hist_data->n_synth_var_refs; i++)
-		destroy_hist_field(hist_data->synth_var_refs[i], 0);
-}
-
-static void save_synth_var_ref(struct hist_trigger_data *hist_data,
-			 struct hist_field *var_ref)
-{
-	hist_data->synth_var_refs[hist_data->n_synth_var_refs++] = var_ref;
-}
-
 static int check_synth_field(struct synth_event *event,
 			     struct hist_field *hist_field,
 			     unsigned int field_pos)
@@ -3775,7 +3759,6 @@ static int onmatch_create(struct hist_trigger_data *hist_data,
 				goto err;
 			}
 
-			save_synth_var_ref(hist_data, var_ref);
 			field_pos++;
 			kfree(p);
 			continue;
@@ -4519,7 +4502,6 @@ static void destroy_hist_data(struct hist_trigger_data *hist_data)
 	destroy_actions(hist_data);
 	destroy_field_vars(hist_data);
 	destroy_field_var_hists(hist_data);
-	destroy_synth_var_refs(hist_data);
 
 	kfree(hist_data);
 }
-- 
2.14.1

[PATCH 7/7] tracing: Add hist trigger comments for variable-related fields

[Tom Zanussi]

From: Tom Zanussi <tom.zanussi@linux.intel.com>

Add a few comments to help clarify how variable and variable reference
fields are used in the code.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index e66dd764e057..b813da145dbd 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -40,6 +40,16 @@ enum field_op_id {
 	FIELD_OP_UNARY_MINUS,
 };
 
+/*
+ * A hist_var (histogram variable) contains variable information for
+ * hist_fields having the HIST_FIELD_FL_VAR or HIST_FIELD_FL_VAR_REF
+ * flag set.  A hist_var has a variable name e.g. ts0, and is
+ * associated with a given histogram trigger, as specified by
+ * hist_data.  The hist_var idx is the unique index assigned to the
+ * variable by the hist trigger's tracing_map.  The idx is what is
+ * used to set a variable's value and, by a variable reference, to
+ * retrieve it.
+ */
 struct hist_var {
 	char				*name;
 	struct hist_trigger_data	*hist_data;
@@ -56,11 +66,29 @@ struct hist_field {
 	const char			*type;
 	struct hist_field		*operands[HIST_FIELD_OPERANDS_MAX];
 	struct hist_trigger_data	*hist_data;
+
+	/*
+	 * Variable fields contain variable-specific info in var.
+	 */
 	struct hist_var			var;
 	enum field_op_id		operator;
 	char				*system;
 	char				*event_name;
+
+	/*
+	 * The name field is used for EXPR and VAR_REF fields.  VAR
+	 * fields contain the variable name in var.name.
+	 */
 	char				*name;
+
+	/*
+	 * When a histogram trigger is hit, if it has any references
+	 * to variables, the values of those variables are collected
+	 * into a var_ref_vals array by resolve_var_refs().  The
+	 * current value of each variable is read from the tracing_map
+	 * using the hist field's hist_var.idx and entered into the
+	 * var_ref_idx entry i.e. var_ref_vals[var_ref_idx].
+	 */
 	unsigned int			var_ref_idx;
 	bool                            read_once;
 };
@@ -365,6 +393,14 @@ struct action_data {
 
 	union {
 		struct {
+			/*
+			 * When a histogram trigger is hit, the values of any
+			 * references to variables, including variables being passed
+			 * as parameters to synthetic events, are collected into a
+			 * var_ref_vals array.  This var_ref_idx is the index of the
+			 * first param in the array to be passed to the synthetic
+			 * event invocation.
+			 */
 			unsigned int		var_ref_idx;
 			char			*match_event;
 			char			*match_event_system;
-- 
2.14.1

Re: [PATCH 0/7] tracing: Hist trigger var-ref cleanup and comments

[Namhyung Kim]

Hi Tom,

On Tue, Dec 18, 2018 at 02:33:19PM -0600, Tom Zanussi wrote:
> From: Tom Zanussi <tom.zanussi@linux.intel.com>
> 
> Hi,
> 
> This patchset is a standalone series broken out of the v8 version of
> the 'tracing: Hist trigger snapshot and onchange additions' patchset.
> 
> It's a series of changes resulting from some suggestions from Namhyung
> for making the variable-reference handling code more understandable
> through some refactoring and comments.
> 
> It also added a new patch changing all strlen() to sizeof() for string
> constants, in trace_events_hist.c
> 
> Also, in the 'tracing: Remove open-coding of hist trigger var_ref
> management' patch, in create_var_ref(), moved the saving of ref_field
> and update of ref_field->var_ref_idx into the 'if' as pointed out by
> Dan Carpenter/smatch 0-day robot.
> 
> It doesn't introduce any functional changes and can be applied
> independently of the other patchset.

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> 
> The following changes since commit 5d6ddf6acce68d1290112cb08b12fd78b201e7d5:
> 
>   arm64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack (2018-12-08 22:21:31 -0500)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/hist-var-ref-cleanup-v1
> 
> Tom Zanussi (7):
>   tracing: Remove unnecessary hist trigger struct field
>   tracing: Change strlen to sizeof for hist trigger static strings
>   tracing: Use var_refs[] for hist trigger reference checking
>   tracing: Remove open-coding of hist trigger var_ref management
>   tracing: Use hist trigger's var_ref array to destroy var_refs
>   tracing: Remove hist trigger synth_var_refs
>   tracing: Add hist trigger comments for variable-related fields
> 
>  kernel/trace/trace_events_hist.c | 267 +++++++++++++++++++++++----------------
>  1 file changed, 156 insertions(+), 111 deletions(-)
> 
> -- 
> 2.14.1
> 

Re: [PATCH 3/7] tracing: Use var_refs[] for hist trigger reference checking

[Masami Hiramatsu]

Hi Tom,

On Tue, 18 Dec 2018 14:33:22 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> From: Tom Zanussi <tom.zanussi@linux.intel.com>
> 
> Since all the variable reference hist_fields are collected into
> hist_data->var_refs[] array, there's no need to go through all the
> fields looking for them, or in separate arrays like synth_var_refs[],
> which will be going away soon anyway.
> 
> This also allows us to get rid of some unnecessary code and functions
> currently used for the same purpose.

I just have a nitpick. 

> 
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> ---
>  kernel/trace/trace_events_hist.c | 57 +++++-----------------------------------
>  1 file changed, 7 insertions(+), 50 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 25d06b3ae1f6..ee839c52bd3f 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1299,45 +1299,11 @@ check_field_for_var_ref(struct hist_field *hist_field,
>  {
>  	struct hist_field *found = NULL;
>  
> -	if (hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF) {
> -		if (hist_field->var.idx == var_idx &&
> -		    hist_field->var.hist_data == var_data) {
> -			found = hist_field;
> -		}
> -	}
> -
> -	return found;
> -}
> -
> -static struct hist_field *
> -check_field_for_var_refs(struct hist_trigger_data *hist_data,
> -			 struct hist_field *hist_field,
> -			 struct hist_trigger_data *var_data,
> -			 unsigned int var_idx,
> -			 unsigned int level)
> -{
> -	struct hist_field *found = NULL;
> -	unsigned int i;
> -
> -	if (level > 3)
> -		return found;
> -
> -	if (!hist_field)
> -		return found;
> +	WARN_ON(!(hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF));
>  
> -	found = check_field_for_var_ref(hist_field, var_data, var_idx);
> -	if (found)
> -		return found;
> -
> -	for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++) {
> -		struct hist_field *operand;
> -
> -		operand = hist_field->operands[i];
> -		found = check_field_for_var_refs(hist_data, operand, var_data,
> -						 var_idx, level + 1);
> -		if (found)
> -			return found;
> -	}
> +	if (hist_field && hist_field->var.idx == var_idx &&
> +	    hist_field->var.hist_data == var_data)
> +		found = hist_field;
>  
>  	return found;

It seems we don't need "found" var here. Just return hist_field or NULL.

>  }
> @@ -1349,18 +1315,9 @@ static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
>  	struct hist_field *hist_field, *found = NULL;
>  	unsigned int i;
>  
> -	for_each_hist_field(i, hist_data) {
> -		hist_field = hist_data->fields[i];
> -		found = check_field_for_var_refs(hist_data, hist_field,
> -						 var_data, var_idx, 0);
> -		if (found)
> -			return found;
> -	}
> -
> -	for (i = 0; i < hist_data->n_synth_var_refs; i++) {
> -		hist_field = hist_data->synth_var_refs[i];
> -		found = check_field_for_var_refs(hist_data, hist_field,
> -						 var_data, var_idx, 0);
> +	for (i = 0; i < hist_data->n_var_refs; i++) {
> +		hist_field = hist_data->var_refs[i];
> +		found = check_field_for_var_ref(hist_field, var_data, var_idx);
>  		if (found)
>  			return found;
>  	}

Here too. If you return soon after found it, you don't need to assign it.

Except for that, it looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> -- 
> 2.14.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/7] tracing: Hist trigger var-ref cleanup and comments

[Masami Hiramatsu]

Hi Tom,

On Tue, 18 Dec 2018 14:33:19 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> From: Tom Zanussi <tom.zanussi@linux.intel.com>
> 
> Hi,
> 
> This patchset is a standalone series broken out of the v8 version of
> the 'tracing: Hist trigger snapshot and onchange additions' patchset.
> 
> It's a series of changes resulting from some suggestions from Namhyung
> for making the variable-reference handling code more understandable
> through some refactoring and comments.
> 
> It also added a new patch changing all strlen() to sizeof() for string
> constants, in trace_events_hist.c
> 
> Also, in the 'tracing: Remove open-coding of hist trigger var_ref
> management' patch, in create_var_ref(), moved the saving of ref_field
> and update of ref_field->var_ref_idx into the 'if' as pointed out by
> Dan Carpenter/smatch 0-day robot.
> 
> It doesn't introduce any functional changes and can be applied
> independently of the other patchset.

OK, now it is very clear to me :-)

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

for the series.

Thank you,



> 
> Tom
> 
> 
> The following changes since commit 5d6ddf6acce68d1290112cb08b12fd78b201e7d5:
> 
>   arm64: Use ftrace_graph_get_ret_stack() instead of curr_ret_stack (2018-12-08 22:21:31 -0500)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/hist-var-ref-cleanup-v1
> 
> Tom Zanussi (7):
>   tracing: Remove unnecessary hist trigger struct field
>   tracing: Change strlen to sizeof for hist trigger static strings
>   tracing: Use var_refs[] for hist trigger reference checking
>   tracing: Remove open-coding of hist trigger var_ref management
>   tracing: Use hist trigger's var_ref array to destroy var_refs
>   tracing: Remove hist trigger synth_var_refs
>   tracing: Add hist trigger comments for variable-related fields
> 
>  kernel/trace/trace_events_hist.c | 267 +++++++++++++++++++++++----------------
>  1 file changed, 156 insertions(+), 111 deletions(-)
> 
> -- 
> 2.14.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/7] tracing: Hist trigger var-ref cleanup and comments

[Tom Zanussi]

Hi Namhyung,

On Wed, 2018-12-19 at 20:45 +0900, Namhyung Kim wrote:
> Hi Tom,
> 
> On Tue, Dec 18, 2018 at 02:33:19PM -0600, Tom Zanussi wrote:
> > From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > 
> > Hi,
> > 
> > This patchset is a standalone series broken out of the v8 version
> > of
> > the 'tracing: Hist trigger snapshot and onchange additions'
> > patchset.
> > 
> > It's a series of changes resulting from some suggestions from
> > Namhyung
> > for making the variable-reference handling code more understandable
> > through some refactoring and comments.
> > 
> > It also added a new patch changing all strlen() to sizeof() for
> > string
> > constants, in trace_events_hist.c
> > 
> > Also, in the 'tracing: Remove open-coding of hist trigger var_ref
> > management' patch, in create_var_ref(), moved the saving of
> > ref_field
> > and update of ref_field->var_ref_idx into the 'if' as pointed out
> > by
> > Dan Carpenter/smatch 0-day robot.
> > 
> > It doesn't introduce any functional changes and can be applied
> > independently of the other patchset.
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> 

Thanks!

Tom

Re: [PATCH 3/7] tracing: Use var_refs[] for hist trigger reference checking

[Tom Zanussi]

Hi Masami,

On Wed, 2018-12-19 at 21:22 +0900, Masami Hiramatsu wrote:
> Hi Tom,
> 
> On Tue, 18 Dec 2018 14:33:22 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > 
> > Since all the variable reference hist_fields are collected into
> > hist_data->var_refs[] array, there's no need to go through all the
> > fields looking for them, or in separate arrays like
> > synth_var_refs[],
> > which will be going away soon anyway.
> > 
> > This also allows us to get rid of some unnecessary code and
> > functions
> > currently used for the same purpose.
> 
> I just have a nitpick. 
> 
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> > ---
> >  kernel/trace/trace_events_hist.c | 57 +++++-----------------------
> > ------------
> >  1 file changed, 7 insertions(+), 50 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index 25d06b3ae1f6..ee839c52bd3f 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -1299,45 +1299,11 @@ check_field_for_var_ref(struct hist_field
> > *hist_field,
> >  {
> >  	struct hist_field *found = NULL;
> >  
> > -	if (hist_field && hist_field->flags &
> > HIST_FIELD_FL_VAR_REF) {
> > -		if (hist_field->var.idx == var_idx &&
> > -		    hist_field->var.hist_data == var_data) {
> > -			found = hist_field;
> > -		}
> > -	}
> > -
> > -	return found;
> > -}
> > -
> > -static struct hist_field *
> > -check_field_for_var_refs(struct hist_trigger_data *hist_data,
> > -			 struct hist_field *hist_field,
> > -			 struct hist_trigger_data *var_data,
> > -			 unsigned int var_idx,
> > -			 unsigned int level)
> > -{
> > -	struct hist_field *found = NULL;
> > -	unsigned int i;
> > -
> > -	if (level > 3)
> > -		return found;
> > -
> > -	if (!hist_field)
> > -		return found;
> > +	WARN_ON(!(hist_field && hist_field->flags &
> > HIST_FIELD_FL_VAR_REF));
> >  
> > -	found = check_field_for_var_ref(hist_field, var_data,
> > var_idx);
> > -	if (found)
> > -		return found;
> > -
> > -	for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++) {
> > -		struct hist_field *operand;
> > -
> > -		operand = hist_field->operands[i];
> > -		found = check_field_for_var_refs(hist_data,
> > operand, var_data,
> > -						 var_idx, level +
> > 1);
> > -		if (found)
> > -			return found;
> > -	}
> > +	if (hist_field && hist_field->var.idx == var_idx &&
> > +	    hist_field->var.hist_data == var_data)
> > +		found = hist_field;
> >  
> >  	return found;
> 
> It seems we don't need "found" var here. Just return hist_field or
> NULL.
> 

OK, will change these and resubmit shortly.

Thanks,

Tom

Re: [PATCH 0/7] tracing: Hist trigger var-ref cleanup and comments

[Tom Zanussi]

On Wed, 2018-12-19 at 21:27 +0900, Masami Hiramatsu wrote:
> Hi Tom,
> 
> On Tue, 18 Dec 2018 14:33:19 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > 
> > Hi,
> > 
> > This patchset is a standalone series broken out of the v8 version
> > of
> > the 'tracing: Hist trigger snapshot and onchange additions'
> > patchset.
> > 
> > It's a series of changes resulting from some suggestions from
> > Namhyung
> > for making the variable-reference handling code more understandable
> > through some refactoring and comments.
> > 
> > It also added a new patch changing all strlen() to sizeof() for
> > string
> > constants, in trace_events_hist.c
> > 
> > Also, in the 'tracing: Remove open-coding of hist trigger var_ref
> > management' patch, in create_var_ref(), moved the saving of
> > ref_field
> > and update of ref_field->var_ref_idx into the 'if' as pointed out
> > by
> > Dan Carpenter/smatch 0-day robot.
> > 
> > It doesn't introduce any functional changes and can be applied
> > independently of the other patchset.
> 
> OK, now it is very clear to me :-)
> 
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> 
> for the series.
> 

Thanks, Masami!

Tom

Re: [PATCH 3/7] tracing: Use var_refs[] for hist trigger reference checking

[Steven Rostedt]

On Wed, 19 Dec 2018 09:01:43 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> > > +	if (hist_field && hist_field->var.idx == var_idx &&
> > > +	    hist_field->var.hist_data == var_data)
> > > +		found = hist_field;
> > >  
> > >  	return found;  
> > 
> > It seems we don't need "found" var here. Just return hist_field or
> > NULL.
> >   
> 
> OK, will change these and resubmit shortly.

Tom,

If this is the only patch in the series that needs updating, can you
just reply to this patch with the v2 of 3/7? That is have a subject of:

[PATCH v2 3/7] tracing: Use var_refs[] for hist trigger reference checking

and reply to the [PATCH 3/7]


This isn't the normal way of updates, but I want to start taking this
patches in ASAP, and I figured this may be the easiest for both of us.

It's fine to send a v2 of the entire patch series (which other
maintainers require), so I'll leave it up to you.

If there's any other patch that needs changing, then a full v2 series
is required. But if this is the only change and you want to send just
this patch, then I'll take that as well.

-- Steve

Re: [PATCH 3/7] tracing: Use var_refs[] for hist trigger reference checking

[Tom Zanussi]

Hi Steve,

On Wed, 2018-12-19 at 10:36 -0500, Steven Rostedt wrote:
> On Wed, 19 Dec 2018 09:01:43 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > > > +	if (hist_field && hist_field->var.idx == var_idx &&
> > > > +	    hist_field->var.hist_data == var_data)
> > > > +		found = hist_field;
> > > >  
> > > >  	return found;  
> > > 
> > > It seems we don't need "found" var here. Just return hist_field
> > > or
> > > NULL.
> > >   
> > 
> > OK, will change these and resubmit shortly.
> 
> Tom,
> 
> If this is the only patch in the series that needs updating, can you
> just reply to this patch with the v2 of 3/7? That is have a subject
> of:
> 
> [PATCH v2 3/7] tracing: Use var_refs[] for hist trigger reference
> checking
> 
> and reply to the [PATCH 3/7]
> 
> 
> This isn't the normal way of updates, but I want to start taking this
> patches in ASAP, and I figured this may be the easiest for both of
> us.
> 
> It's fine to send a v2 of the entire patch series (which other
> maintainers require), so I'll leave it up to you.
> 
> If there's any other patch that needs changing, then a full v2 series
> is required. But if this is the only change and you want to send just
> this patch, then I'll take that as well.
> 

Sorry for the delay - had an emergency appointment for the dog at the
vet this morning..

Anyway, yeah, I'll reply with the patch - it's the only one that needs
updating in this series.

Thanks,

Tom

> -- Steve

[PATCH v2 3/7] tracing: Use var_refs[] for hist trigger reference checking

[Tom Zanussi]

Since all the variable reference hist_fields are collected into
hist_data->var_refs[] array, there's no need to go through all the
fields looking for them, or in separate arrays like synth_var_refs[],
which will be going away soon anyway.

This also allows us to get rid of some unnecessary code and functions
currently used for the same purpose.

Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 68 +++++++---------------------------------
 1 file changed, 11 insertions(+), 57 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 25d06b3ae1f6..f3caf6e484f4 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1297,75 +1297,29 @@ check_field_for_var_ref(struct hist_field *hist_field,
 			struct hist_trigger_data *var_data,
 			unsigned int var_idx)
 {
-	struct hist_field *found = NULL;
-
-	if (hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF) {
-		if (hist_field->var.idx == var_idx &&
-		    hist_field->var.hist_data == var_data) {
-			found = hist_field;
-		}
-	}
-
-	return found;
-}
-
-static struct hist_field *
-check_field_for_var_refs(struct hist_trigger_data *hist_data,
-			 struct hist_field *hist_field,
-			 struct hist_trigger_data *var_data,
-			 unsigned int var_idx,
-			 unsigned int level)
-{
-	struct hist_field *found = NULL;
-	unsigned int i;
-
-	if (level > 3)
-		return found;
-
-	if (!hist_field)
-		return found;
-
-	found = check_field_for_var_ref(hist_field, var_data, var_idx);
-	if (found)
-		return found;
-
-	for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++) {
-		struct hist_field *operand;
+	WARN_ON(!(hist_field && hist_field->flags & HIST_FIELD_FL_VAR_REF));
 
-		operand = hist_field->operands[i];
-		found = check_field_for_var_refs(hist_data, operand, var_data,
-						 var_idx, level + 1);
-		if (found)
-			return found;
-	}
+	if (hist_field && hist_field->var.idx == var_idx &&
+	    hist_field->var.hist_data == var_data)
+		return hist_field;
 
-	return found;
+	return NULL;
 }
 
 static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data,
 				       struct hist_trigger_data *var_data,
 				       unsigned int var_idx)
 {
-	struct hist_field *hist_field, *found = NULL;
+	struct hist_field *hist_field;
 	unsigned int i;
 
-	for_each_hist_field(i, hist_data) {
-		hist_field = hist_data->fields[i];
-		found = check_field_for_var_refs(hist_data, hist_field,
-						 var_data, var_idx, 0);
-		if (found)
-			return found;
-	}
-
-	for (i = 0; i < hist_data->n_synth_var_refs; i++) {
-		hist_field = hist_data->synth_var_refs[i];
-		found = check_field_for_var_refs(hist_data, hist_field,
-						 var_data, var_idx, 0);
-		if (found)
-			return found;
+	for (i = 0; i < hist_data->n_var_refs; i++) {
+		hist_field = hist_data->var_refs[i];
+		if (check_field_for_var_ref(hist_field, var_data, var_idx))
+			return hist_field;
 	}
 
-	return found;
+	return NULL;
 }
 
 static struct hist_field *find_any_var_ref(struct hist_trigger_data *hist_data,
-- 
2.14.1

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Steven Rostedt]

On Tue, 18 Dec 2018 14:33:21 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> From: Tom Zanussi <tom.zanussi@linux.intel.com>
> 
> There's no need to use strlen() for static strings when the length is
> already known, so update trace_events_hist.c with sizeof() for those
> cases.
> 
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> ---
>  kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index d29bf8a8e1dd..25d06b3ae1f6 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -507,7 +507,7 @@ static int synth_field_string_size(char *type)
>  	start = strstr(type, "char[");
>  	if (start == NULL)
>  		return -EINVAL;
> -	start += strlen("char[");
> +	start += sizeof("char[") - 1;
>  
>  	end = strchr(type, ']');
>  	if (!end || end < start)
> @@ -1843,8 +1843,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(", strlen("onmatch(")) == 0) ||
> -	    (strncmp(str, "onmax(", strlen("onmax(")) == 0)) {
> +	if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) ||
> +	    (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
>  		attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
>  		if (!attrs->action_str[attrs->n_actions]) {
>  			ret = -ENOMEM;
> @@ -1861,34 +1861,34 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
>  {
>  	int ret = 0;
>  
> -	if ((strncmp(str, "key=", strlen("key=")) == 0) ||
> -	    (strncmp(str, "keys=", strlen("keys=")) == 0)) {
> +	if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
> +	    (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
>  		attrs->keys_str = kstrdup(str, GFP_KERNEL);

I'll apply this as is, but since there's a lot of these, I wonder if we
should make a marcro for this:

#define strcmp_const(str, str_const) strncmp(str, str_const, sizeof(str_const) - 1)

?

This would help prevent bugs due to typos and bad cut and paste.

-- Steve



>  		if (!attrs->keys_str) {
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> -	} else if ((strncmp(str, "val=", strlen("val=")) == 0) ||
> -		 (strncmp(str, "vals=", strlen("vals=")) == 0) ||
> -		 (strncmp(str, "values=", strlen("values=")) == 0)) {
> +	} else if ((strncmp(str, "val=", sizeof("val=") - 1) == 0) ||
> +		 (strncmp(str, "vals=", sizeof("vals=") - 1) == 0) ||
> +		 (strncmp(str, "values=", sizeof("values=") - 1) == 0)) {
>  		attrs->vals_str = kstrdup(str, GFP_KERNEL);
>  		if (!attrs->vals_str) {
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> -	} else if (strncmp(str, "sort=", strlen("sort=")) == 0) {
> +	} else if (strncmp(str, "sort=", sizeof("sort=") - 1) == 0) {
>  		attrs->sort_key_str = kstrdup(str, GFP_KERNEL);
>  		if (!attrs->sort_key_str) {
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> -	} else if (strncmp(str, "name=", strlen("name=")) == 0) {
> +	} else if (strncmp(str, "name=", sizeof("name=") - 1) == 0) {
>  		attrs->name = kstrdup(str, GFP_KERNEL);
>  		if (!attrs->name) {
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> -	} else if (strncmp(str, "clock=", strlen("clock=")) == 0) {
> +	} else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
>  		strsep(&str, "=");
>  		if (!str) {
>  			ret = -EINVAL;
> @@ -1901,7 +1901,7 @@ static int parse_assignment(char *str, struct hist_trigger_attrs *attrs)
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> -	} else if (strncmp(str, "size=", strlen("size=")) == 0) {
> +	} else if (strncmp(str, "size=", sizeof("size=") - 1) == 0) {
>  		int map_bits = parse_map_size(str);
>  
>  		if (map_bits < 0) {
> @@ -3497,7 +3497,7 @@ static struct action_data *onmax_parse(char *str)
>  	if (!onmax_fn_name || !str)
>  		goto free;
>  
> -	if (strncmp(onmax_fn_name, "save", strlen("save")) == 0) {
> +	if (strncmp(onmax_fn_name, "save", sizeof("save") - 1) == 0) {
>  		char *params = strsep(&str, ")");
>  
>  		if (!params) {
> @@ -4302,8 +4302,8 @@ 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(", strlen("onmatch(")) == 0) {
> -			char *action_str = str + strlen("onmatch(");
> +		if (strncmp(str, "onmatch(", sizeof("onmatch(") - 1) == 0) {
> +			char *action_str = str + sizeof("onmatch(") - 1;
>  
>  			data = onmatch_parse(tr, action_str);
>  			if (IS_ERR(data)) {
> @@ -4311,8 +4311,8 @@ static int parse_actions(struct hist_trigger_data *hist_data)
>  				break;
>  			}
>  			data->fn = action_trace;
> -		} else if (strncmp(str, "onmax(", strlen("onmax(")) == 0) {
> -			char *action_str = str + strlen("onmax(");
> +		} else if (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0) {
> +			char *action_str = str + sizeof("onmax(") - 1;
>  
>  			data = onmax_parse(action_str);
>  			if (IS_ERR(data)) {
> @@ -5548,9 +5548,9 @@ static int event_hist_trigger_func(struct event_command *cmd_ops,
>  			p++;
>  			continue;
>  		}
> -		if (p >= param + strlen(param) - strlen("if") - 1)
> +		if (p >= param + strlen(param) - (sizeof("if") - 1) - 1)
>  			return -EINVAL;
> -		if (*(p + strlen("if")) != ' ' && *(p + strlen("if")) != '\t') {
> +		if (*(p + sizeof("if") - 1) != ' ' && *(p + sizeof("if") - 1) != '\t') {
>  			p++;
>  			continue;
>  		}

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Tom Zanussi]

Hi Steve,

On Wed, 2018-12-19 at 14:40 -0500, Steven Rostedt wrote:
> On Tue, 18 Dec 2018 14:33:21 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > 
> > There's no need to use strlen() for static strings when the length
> > is
> > already known, so update trace_events_hist.c with sizeof() for
> > those
> > cases.
> > 
> > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> > ---
> >  kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++---------
> > ----------
> >  1 file changed, 19 insertions(+), 19 deletions(-)
> > 
> > diff --git a/kernel/trace/trace_events_hist.c
> > b/kernel/trace/trace_events_hist.c
> > index d29bf8a8e1dd..25d06b3ae1f6 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -507,7 +507,7 @@ static int synth_field_string_size(char *type)
> >  	start = strstr(type, "char[");
> >  	if (start == NULL)
> >  		return -EINVAL;
> > -	start += strlen("char[");
> > +	start += sizeof("char[") - 1;
> >  
> >  	end = strchr(type, ']');
> >  	if (!end || end < start)
> > @@ -1843,8 +1843,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(", strlen("onmatch(")) == 0) ||
> > -	    (strncmp(str, "onmax(", strlen("onmax(")) == 0)) {
> > +	if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) ==
> > 0) ||
> > +	    (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
> >  		attrs->action_str[attrs->n_actions] = kstrdup(str,
> > GFP_KERNEL);
> >  		if (!attrs->action_str[attrs->n_actions]) {
> >  			ret = -ENOMEM;
> > @@ -1861,34 +1861,34 @@ static int parse_assignment(char *str,
> > struct hist_trigger_attrs *attrs)
> >  {
> >  	int ret = 0;
> >  
> > -	if ((strncmp(str, "key=", strlen("key=")) == 0) ||
> > -	    (strncmp(str, "keys=", strlen("keys=")) == 0)) {
> > +	if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
> > +	    (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
> >  		attrs->keys_str = kstrdup(str, GFP_KERNEL);
> 
> I'll apply this as is, but since there's a lot of these, I wonder if
> we
> should make a marcro for this:
> 
> #define strcmp_const(str, str_const) strncmp(str, str_const,
> sizeof(str_const) - 1)
> 
> ?
> 
> This would help prevent bugs due to typos and bad cut and paste.
> 

Yeah, I had considered it but wasn't sure it was worth it.  Since
you're suggesting it is, I can send another patch on top of these, or
feel free if you want to too.  ;-)

Tom

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Tom Zanussi]

On Wed, 2018-12-19 at 13:46 -0600, Tom Zanussi wrote:
> Hi Steve,
> 
> On Wed, 2018-12-19 at 14:40 -0500, Steven Rostedt wrote:
> > On Tue, 18 Dec 2018 14:33:21 -0600
> > Tom Zanussi <zanussi@kernel.org> wrote:
> > 
> > > From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > > 
> > > There's no need to use strlen() for static strings when the
> > > length
> > > is
> > > already known, so update trace_events_hist.c with sizeof() for
> > > those
> > > cases.
> > > 
> > > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> > > ---
> > >  kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++-------
> > > --
> > > ----------
> > >  1 file changed, 19 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_events_hist.c
> > > b/kernel/trace/trace_events_hist.c
> > > index d29bf8a8e1dd..25d06b3ae1f6 100644
> > > --- a/kernel/trace/trace_events_hist.c
> > > +++ b/kernel/trace/trace_events_hist.c
> > > @@ -507,7 +507,7 @@ static int synth_field_string_size(char
> > > *type)
> > >  	start = strstr(type, "char[");
> > >  	if (start == NULL)
> > >  		return -EINVAL;
> > > -	start += strlen("char[");
> > > +	start += sizeof("char[") - 1;
> > >  
> > >  	end = strchr(type, ']');
> > >  	if (!end || end < start)
> > > @@ -1843,8 +1843,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(", strlen("onmatch(")) == 0)
> > > ||
> > > -	    (strncmp(str, "onmax(", strlen("onmax(")) == 0)) {
> > > +	if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) ==
> > > 0) ||
> > > +	    (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0))
> > > {
> > >  		attrs->action_str[attrs->n_actions] =
> > > kstrdup(str,
> > > GFP_KERNEL);
> > >  		if (!attrs->action_str[attrs->n_actions]) {
> > >  			ret = -ENOMEM;
> > > @@ -1861,34 +1861,34 @@ static int parse_assignment(char *str,
> > > struct hist_trigger_attrs *attrs)
> > >  {
> > >  	int ret = 0;
> > >  
> > > -	if ((strncmp(str, "key=", strlen("key=")) == 0) ||
> > > -	    (strncmp(str, "keys=", strlen("keys=")) == 0)) {
> > > +	if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
> > > +	    (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
> > >  		attrs->keys_str = kstrdup(str, GFP_KERNEL);
> > 
> > I'll apply this as is, but since there's a lot of these, I wonder
> > if
> > we
> > should make a marcro for this:
> > 
> > #define strcmp_const(str, str_const) strncmp(str, str_const,
> > sizeof(str_const) - 1)
> > 
> > ?
> > 
> > This would help prevent bugs due to typos and bad cut and paste.
> > 
> 
> Yeah, I had considered it but wasn't sure it was worth it.  Since
> you're suggesting it is, I can send another patch on top of these, or
> feel free if you want to too.  ;-)
> 

How's this?

[PATCH] tracing: Introduce and use strcmp_const() for hist triggers

Provide a new strcmp_const() macro and make use of it instead of the
longer and more error-prone strncmp(str, "str", sizeof("str") - 1).

Idea-and-macro-by: Steve Rostedt <rostedt@goodmis.org>
Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
---
 kernel/trace/trace_events_hist.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c5448c103770..aaf0d2ac4aab 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -22,6 +22,9 @@
 
 #define STR_VAR_LEN_MAX		32 /* must be multiple of sizeof(u64) */
 
+#define strcmp_const(str, str_const) \
+	strncmp(str, str_const, sizeof(str_const) - 1)
+
 struct hist_field;
 
 typedef u64 (*hist_field_fn_t) (struct hist_field *field,
@@ -1881,8 +1884,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 ((strcmp_const(str, "onmatch(") == 0) ||
+	    (strcmp_const(str, "onmax(") == 0)) {
 		attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
 		if (!attrs->action_str[attrs->n_actions]) {
 			ret = -ENOMEM;
@@ -1899,34 +1902,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 ((strcmp_const(str, "key=") == 0) ||
+	    (strcmp_const(str, "keys=") == 0)) {
 		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 ((strcmp_const(str, "val=") == 0) ||
+		 (strcmp_const(str, "vals=") == 0) ||
+		 (strcmp_const(str, "values=") == 0)) {
 		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 (strcmp_const(str, "sort=") == 0) {
 		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 (strcmp_const(str, "name=") == 0) {
 		attrs->name = kstrdup(str, GFP_KERNEL);
 		if (!attrs->name) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
+	} else if (strcmp_const(str, "clock=") == 0) {
 		strsep(&str, "=");
 		if (!str) {
 			ret = -EINVAL;
@@ -1939,7 +1942,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 (strcmp_const(str, "size=") == 0) {
 		int map_bits = parse_map_size(str);
 
 		if (map_bits < 0) {
@@ -3558,7 +3561,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 (strcmp_const(onmax_fn_name, "save") == 0) {
 		char *params = strsep(&str, ")");
 
 		if (!params) {
@@ -4346,7 +4349,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 (strcmp_const(str, "onmatch(") == 0) {
 			char *action_str = str + sizeof("onmatch(") - 1;
 
 			data = onmatch_parse(tr, action_str);
@@ -4355,7 +4358,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 (strcmp_const(str, "onmax(") == 0) {
 			char *action_str = str + sizeof("onmax(") - 1;
 
 			data = onmax_parse(action_str);
-- 
2.14.1



> Tom

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Joe Perches]

On Wed, 2018-12-19 at 13:46 -0600, Tom Zanussi wrote:
> Hi Steve,
> 
> On Wed, 2018-12-19 at 14:40 -0500, Steven Rostedt wrote:
> > On Tue, 18 Dec 2018 14:33:21 -0600
> > Tom Zanussi <zanussi@kernel.org> wrote:
> > 
> > > From: Tom Zanussi <tom.zanussi@linux.intel.com>
> > > 
> > > There's no need to use strlen() for static strings when the length
> > > is
> > > already known, so update trace_events_hist.c with sizeof() for
> > > those
> > > cases.
> > > 
> > > Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> > > ---
> > >  kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++---------
> > > ----------
> > >  1 file changed, 19 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/kernel/trace/trace_events_hist.c
> > > b/kernel/trace/trace_events_hist.c
> > > index d29bf8a8e1dd..25d06b3ae1f6 100644
> > > --- a/kernel/trace/trace_events_hist.c
> > > +++ b/kernel/trace/trace_events_hist.c
> > > @@ -507,7 +507,7 @@ static int synth_field_string_size(char *type)
> > >  	start = strstr(type, "char[");
> > >  	if (start == NULL)
> > >  		return -EINVAL;
> > > -	start += strlen("char[");
> > > +	start += sizeof("char[") - 1;
> > >  
> > >  	end = strchr(type, ']');
> > >  	if (!end || end < start)
> > > @@ -1843,8 +1843,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(", strlen("onmatch(")) == 0) ||
> > > -	    (strncmp(str, "onmax(", strlen("onmax(")) == 0)) {
> > > +	if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) ==
> > > 0) ||
> > > +	    (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) {
> > >  		attrs->action_str[attrs->n_actions] = kstrdup(str,
> > > GFP_KERNEL);
> > >  		if (!attrs->action_str[attrs->n_actions]) {
> > >  			ret = -ENOMEM;
> > > @@ -1861,34 +1861,34 @@ static int parse_assignment(char *str,
> > > struct hist_trigger_attrs *attrs)
> > >  {
> > >  	int ret = 0;
> > >  
> > > -	if ((strncmp(str, "key=", strlen("key=")) == 0) ||
> > > -	    (strncmp(str, "keys=", strlen("keys=")) == 0)) {
> > > +	if ((strncmp(str, "key=", sizeof("key=") - 1) == 0) ||
> > > +	    (strncmp(str, "keys=", sizeof("keys=") - 1) == 0)) {
> > >  		attrs->keys_str = kstrdup(str, GFP_KERNEL);
> > 
> > I'll apply this as is, but since there's a lot of these, I wonder if
> > we
> > should make a marcro for this:
> > 
> > #define strcmp_const(str, str_const) strncmp(str, str_const,
> > sizeof(str_const) - 1)
> > 
> > ?
> > 
> > This would help prevent bugs due to typos and bad cut and paste.
> > 
> 
> Yeah, I had considered it but wasn't sure it was worth it.  Since
> you're suggesting it is, I can send another patch on top of these, or
> feel free if you want to too.  ;-)

I believe the 'strlen("foo") -> sizeof("foo") - 1'
conversions do not change objects at all.

strlen("constant") is already optimized by gcc to a
constant value when fed a constant string.

the strcmp_const macro does seem to make sense as
the copy/paste/typo possibility is real.

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Joe Perches]

On Wed, 2018-12-19 at 14:16 -0600, Tom Zanussi wrote:
> 
> How's this?
> 
> [PATCH] tracing: Introduce and use strcmp_const() for hist triggers
> 
> Provide a new strcmp_const() macro and make use of it instead of the
> longer and more error-prone strncmp(str, "str", sizeof("str") - 1).
[]
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
[]
> @@ -22,6 +22,9 @@
>  
>  #define STR_VAR_LEN_MAX		32 /* must be multiple of sizeof(u64) */
>  
> +#define strcmp_const(str, str_const) \
> +	strncmp(str, str_const, sizeof(str_const) - 1)

Not good as it's too easy to pass a pointer as str_const
and sizeof(pointer) - 1 isn't likely the string length.

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Steven Rostedt]

On Wed, 19 Dec 2018 13:46:49 -0600
Tom Zanussi <zanussi@kernel.org> wrote:


> Yeah, I had considered it but wasn't sure it was worth it.  Since
> you're suggesting it is, I can send another patch on top of these, or
> feel free if you want to too.  ;-)
> 

Here, want to ack/review it?

-- Steve


From 664457f5cc776aa624502bc628285ea6d70ac8c9 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Wed, 19 Dec 2018 15:13:33 -0500
Subject: [PATCH] tracing: Add strncmp_const() macro to prevent typos and cut
 paste errors

The trace_events_hist.c file has a lot of strncmp()s of the form:

   if (strncmp(str, "constant", sizeof("constant") - 1) == 0)

To prevent typos and errors with the constant string being different from
the string use in sizeof(), and also to condense the code, add a macro that
does this:

 #define strncmp_const(str, str_const) \
   strncmp(str, str_const, sizeof(str_const) - 1)

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c5448c103770..40e76053d3d7 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -22,6 +22,10 @@
 
 #define STR_VAR_LEN_MAX		32 /* must be multiple of sizeof(u64) */
 
+/* Safe way to compare string constants (from typos and cut and paste errors) */
+#define strncmp_const(str, str_const)			\
+	strncmp(str, str_const, sizeof(str_const) - 1)
+
 struct hist_field;
 
 typedef u64 (*hist_field_fn_t) (struct hist_field *field,
@@ -1881,8 +1885,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 ((strncmp_const(str, "onmatch(") == 0) ||
+	    (strncmp_const(str, "onmax(") == 0)) {
 		attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
 		if (!attrs->action_str[attrs->n_actions]) {
 			ret = -ENOMEM;
@@ -1899,34 +1903,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 ((strncmp_const(str, "key=") == 0) ||
+	    (strncmp_const(str, "keys=") == 0)) {
 		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 ((strncmp_const(str, "val=") == 0) ||
+		   (strncmp_const(str, "vals=") == 0) ||
+		   (strncmp_const(str, "values=") == 0)) {
 		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 (strncmp_const(str, "sort=") == 0) {
 		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 (strncmp_const(str, "name=") == 0) {
 		attrs->name = kstrdup(str, GFP_KERNEL);
 		if (!attrs->name) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
+	} else if (strncmp_const(str, "clock=") == 0) {
 		strsep(&str, "=");
 		if (!str) {
 			ret = -EINVAL;
@@ -1939,7 +1943,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 (strncmp_const(str, "size=") == 0) {
 		int map_bits = parse_map_size(str);
 
 		if (map_bits < 0) {
@@ -3558,7 +3562,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 (strncmp_const(onmax_fn_name, "save") == 0) {
 		char *params = strsep(&str, ")");
 
 		if (!params) {
@@ -4346,7 +4350,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 (strncmp_const(str, "onmatch(") == 0) {
 			char *action_str = str + sizeof("onmatch(") - 1;
 
 			data = onmatch_parse(tr, action_str);
@@ -4355,7 +4359,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 (strncmp_const(str, "onmax(") == 0) {
 			char *action_str = str + sizeof("onmax(") - 1;
 
 			data = onmax_parse(action_str);
-- 
2.19.1

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Steven Rostedt]

On Wed, 19 Dec 2018 13:46:49 -0600
Tom Zanussi <zanussi@kernel.org> wrote:


> Yeah, I had considered it but wasn't sure it was worth it.  Since
> you're suggesting it is, I can send another patch on top of these, or
> feel free if you want to too.  ;-)
> 

Here, want to ack/review it?

-- Steve


>From 664457f5cc776aa624502bc628285ea6d70ac8c9 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Wed, 19 Dec 2018 15:13:33 -0500
Subject: [PATCH] tracing: Add strncmp_const() macro to prevent typos and cut
 paste errors

The trace_events_hist.c file has a lot of strncmp()s of the form:

   if (strncmp(str, "constant", sizeof("constant") - 1) == 0)

To prevent typos and errors with the constant string being different from
the string use in sizeof(), and also to condense the code, add a macro that
does this:

 #define strncmp_const(str, str_const) \
   strncmp(str, str_const, sizeof(str_const) - 1)

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_events_hist.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index c5448c103770..40e76053d3d7 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -22,6 +22,10 @@
 
 #define STR_VAR_LEN_MAX		32 /* must be multiple of sizeof(u64) */
 
+/* Safe way to compare string constants (from typos and cut and paste errors) */
+#define strncmp_const(str, str_const)			\
+	strncmp(str, str_const, sizeof(str_const) - 1)
+
 struct hist_field;
 
 typedef u64 (*hist_field_fn_t) (struct hist_field *field,
@@ -1881,8 +1885,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 ((strncmp_const(str, "onmatch(") == 0) ||
+	    (strncmp_const(str, "onmax(") == 0)) {
 		attrs->action_str[attrs->n_actions] = kstrdup(str, GFP_KERNEL);
 		if (!attrs->action_str[attrs->n_actions]) {
 			ret = -ENOMEM;
@@ -1899,34 +1903,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 ((strncmp_const(str, "key=") == 0) ||
+	    (strncmp_const(str, "keys=") == 0)) {
 		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 ((strncmp_const(str, "val=") == 0) ||
+		   (strncmp_const(str, "vals=") == 0) ||
+		   (strncmp_const(str, "values=") == 0)) {
 		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 (strncmp_const(str, "sort=") == 0) {
 		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 (strncmp_const(str, "name=") == 0) {
 		attrs->name = kstrdup(str, GFP_KERNEL);
 		if (!attrs->name) {
 			ret = -ENOMEM;
 			goto out;
 		}
-	} else if (strncmp(str, "clock=", sizeof("clock=") - 1) == 0) {
+	} else if (strncmp_const(str, "clock=") == 0) {
 		strsep(&str, "=");
 		if (!str) {
 			ret = -EINVAL;
@@ -1939,7 +1943,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 (strncmp_const(str, "size=") == 0) {
 		int map_bits = parse_map_size(str);
 
 		if (map_bits < 0) {
@@ -3558,7 +3562,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 (strncmp_const(onmax_fn_name, "save") == 0) {
 		char *params = strsep(&str, ")");
 
 		if (!params) {
@@ -4346,7 +4350,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 (strncmp_const(str, "onmatch(") == 0) {
 			char *action_str = str + sizeof("onmatch(") - 1;
 
 			data = onmatch_parse(tr, action_str);
@@ -4355,7 +4359,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 (strncmp_const(str, "onmax(") == 0) {
 			char *action_str = str + sizeof("onmax(") - 1;
 
 			data = onmax_parse(action_str);
-- 
2.19.1

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Steven Rostedt]

On Wed, 19 Dec 2018 14:16:31 -0600
Tom Zanussi <zanussi@kernel.org> wrote:


> > Yeah, I had considered it but wasn't sure it was worth it.  Since
> > you're suggesting it is, I can send another patch on top of these, or
> > feel free if you want to too.  ;-)
> >   
> 
> How's this?
> 
> [PATCH] tracing: Introduce and use strcmp_const() for hist triggers
> 
> Provide a new strcmp_const() macro and make use of it instead of the
> longer and more error-prone strncmp(str, "str", sizeof("str") - 1).
> 
> Idea-and-macro-by: Steve Rostedt <rostedt@goodmis.org>
> Signed-off-by: Tom Zanussi <tom.zanussi@linux.intel.com>
> ---

And I just sent you my version :-) (cross emails).

Note, I changed it to strncmp_const() because it should note that it's
a strncmp() and not an strcmp().

Pretty much identical patches too ;-)

-- Steve

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Steven Rostedt]

On Wed, 19 Dec 2018 12:20:19 -0800
Joe Perches <joe@perches.com> wrote:

> > Yeah, I had considered it but wasn't sure it was worth it.  Since
> > you're suggesting it is, I can send another patch on top of these, or
> > feel free if you want to too.  ;-)  
> 
> I believe the 'strlen("foo") -> sizeof("foo") - 1'
> conversions do not change objects at all.
> 
> strlen("constant") is already optimized by gcc to a
> constant value when fed a constant string.

If that's the case (and it probably is), then yeah, strlen is probably
better. As it can handle the "not a constant" that you stated in
another email.

-- Steve


> 
> the strcmp_const macro does seem to make sense as
> the copy/paste/typo possibility is real.

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Steven Rostedt]

On Wed, 19 Dec 2018 12:22:38 -0800
Joe Perches <joe@perches.com> wrote:

> On Wed, 2018-12-19 at 14:16 -0600, Tom Zanussi wrote:
> > 
> > How's this?
> > 
> > [PATCH] tracing: Introduce and use strcmp_const() for hist triggers
> > 
> > Provide a new strcmp_const() macro and make use of it instead of the
> > longer and more error-prone strncmp(str, "str", sizeof("str") - 1).  
> []
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c  
> []
> > @@ -22,6 +22,9 @@
> >  
> >  #define STR_VAR_LEN_MAX		32 /* must be multiple of sizeof(u64) */
> >  
> > +#define strcmp_const(str, str_const) \
> > +	strncmp(str, str_const, sizeof(str_const) - 1)  
> 
> Not good as it's too easy to pass a pointer as str_const
> and sizeof(pointer) - 1 isn't likely the string length.

Agreed. And I noticed that this is used all over the kernel, so I'm not
going to add this patch. I'm going to add a:

#define strncmp_prefix(str, prefix) \
	strncmp(str, prefix, strlen(prefix))

in include/linux/string.h

And go around and use that throughout the kernel. By doing a quick
grep, I already spotted a few bugs.

-- Steve

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Tom Zanussi]

On Wed, 2018-12-19 at 15:30 -0500, Steven Rostedt wrote:
> On Wed, 19 Dec 2018 12:20:19 -0800
> Joe Perches <joe@perches.com> wrote:
> 
> > > Yeah, I had considered it but wasn't sure it was worth it.  Since
> > > you're suggesting it is, I can send another patch on top of
> > > these, or
> > > feel free if you want to too.  ;-)  
> > 
> > I believe the 'strlen("foo") -> sizeof("foo") - 1'
> > conversions do not change objects at all.
> > 
> > strlen("constant") is already optimized by gcc to a
> > constant value when fed a constant string.
> 
> If that's the case (and it probably is), then yeah, strlen is
> probably
> better. As it can handle the "not a constant" that you stated in
> another email.
> 

OK, so I guess that means we should just drop this patch ('[PATCH 2/7]
tracing: Change strlen to sizeof for hist trigger static strings').

Tom

> -- Steve
> 
> 
> > 
> > the strcmp_const macro does seem to make sense as
> > the copy/paste/typo possibility is real.
> 
> 

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Joe Perches]

On Wed, 2018-12-19 at 15:34 -0500, Steven Rostedt wrote:
> On Wed, 19 Dec 2018 12:22:38 -0800
> Joe Perches <joe@perches.com> wrote:
> 
> > On Wed, 2018-12-19 at 14:16 -0600, Tom Zanussi wrote:
> > > How's this?
> > > 
> > > [PATCH] tracing: Introduce and use strcmp_const() for hist triggers
> > > 
> > > Provide a new strcmp_const() macro and make use of it instead of the
> > > longer and more error-prone strncmp(str, "str", sizeof("str") - 1).  
> > []
> > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c  
> > []
> > > @@ -22,6 +22,9 @@
> > >  
> > >  #define STR_VAR_LEN_MAX		32 /* must be multiple of sizeof(u64) */
> > >  
> > > +#define strcmp_const(str, str_const) \
> > > +	strncmp(str, str_const, sizeof(str_const) - 1)  
> > 
> > Not good as it's too easy to pass a pointer as str_const
> > and sizeof(pointer) - 1 isn't likely the string length.
> 
> Agreed. And I noticed that this is used all over the kernel, so I'm not
> going to add this patch. I'm going to add a:
> 
> #define strncmp_prefix(str, prefix) \
> 	strncmp(str, prefix, strlen(prefix))
> 
> in include/linux/string.h
> 
> And go around and use that throughout the kernel. By doing a quick
> grep, I already spotted a few bugs.

I hope you also convert the existing uses like

	strncmp(str1, "str2", 4)

where the length value is precalculated to the strlen
of the const string

But there seem to be _a lot_ of those...

$ git grep -P "\bstrncmp\s*\([^,]+,[^,]+,\s*\d+\s*\)" | wc -l
1681

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Steven Rostedt]

On Wed, 19 Dec 2018 14:38:39 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> On Wed, 2018-12-19 at 15:30 -0500, Steven Rostedt wrote:
> > On Wed, 19 Dec 2018 12:20:19 -0800
> > Joe Perches <joe@perches.com> wrote:
> >   
> > > > Yeah, I had considered it but wasn't sure it was worth it.  Since
> > > > you're suggesting it is, I can send another patch on top of
> > > > these, or
> > > > feel free if you want to too.  ;-)    
> > > 
> > > I believe the 'strlen("foo") -> sizeof("foo") - 1'
> > > conversions do not change objects at all.
> > > 
> > > strlen("constant") is already optimized by gcc to a
> > > constant value when fed a constant string.  
> > 
> > If that's the case (and it probably is), then yeah, strlen is
> > probably
> > better. As it can handle the "not a constant" that you stated in
> > another email.
> >   
> 
> OK, so I guess that means we should just drop this patch ('[PATCH 2/7]
> tracing: Change strlen to sizeof for hist trigger static strings').
> 

No, that patch is fine, the macro was not. I've already applied your
patch set. Just need to run it through my tests.

-- Steve

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Steven Rostedt]

On Wed, 19 Dec 2018 12:51:59 -0800
Joe Perches <joe@perches.com> wrote:


> > #define strncmp_prefix(str, prefix) \
> > 	strncmp(str, prefix, strlen(prefix))
> > 
> > in include/linux/string.h
> > 
> > And go around and use that throughout the kernel. By doing a quick
> > grep, I already spotted a few bugs.  
> 
> I hope you also convert the existing uses like
> 
> 	strncmp(str1, "str2", 4)
> 
> where the length value is precalculated to the strlen
> of the const string

Yeah sure.

> 
> But there seem to be _a lot_ of those...
> 
> $ git grep -P "\bstrncmp\s*\([^,]+,[^,]+,\s*\d+\s*\)" | wc -l
> 1681
> 

Right.

I'm going to first create the macro and probably just apply it to the
tracing directory. Then when the macro is added to the next merge
window, I'll post a bunch of patches that clean up the rest of the code.

-- Steve

Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings

[Joe Perches]

On Wed, 2018-12-19 at 16:01 -0500, Steven Rostedt wrote:
> On Wed, 19 Dec 2018 14:38:39 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > On Wed, 2018-12-19 at 15:30 -0500, Steven Rostedt wrote:
> > > On Wed, 19 Dec 2018 12:20:19 -0800
> > > Joe Perches <joe@perches.com> wrote:
> > >   
> > > > > Yeah, I had considered it but wasn't sure it was worth it.  Since
> > > > > you're suggesting it is, I can send another patch on top of
> > > > > these, or
> > > > > feel free if you want to too.  ;-)    
> > > > 
> > > > I believe the 'strlen("foo") -> sizeof("foo") - 1'
> > > > conversions do not change objects at all.
> > > > 
> > > > strlen("constant") is already optimized by gcc to a
> > > > constant value when fed a constant string.  
> > > 
> > > If that's the case (and it probably is), then yeah, strlen is
> > > probably
> > > better. As it can handle the "not a constant" that you stated in
> > > another email.
> > >   
> > 
> > OK, so I guess that means we should just drop this patch ('[PATCH 2/7]
> > tracing: Change strlen to sizeof for hist trigger static strings').
> > 
> 
> No, that patch is fine, the macro was not. I've already applied your
> patch set. Just need to run it through my tests.

The patch included:

-	start += strlen("char[");
+	start += sizeof("char[") - 1;

So, that 2/7 patch is unnecessary as there is no object code
change and using 'sizeof("const") - 1' is less intelligible
than strlen("const")

If you convert the strncmp(ptr, "const", strlen("const"))
uses to strncmp_prefix(ptr, "const") eventually, the patch
just makes more variations to change.