tracing: Hist trigger var-ref cleanup and comments
cover tracing: Hist trigger var-ref cleanup and comments
From: Tom Zanussi <[email protected]>
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(-)
Signature · 1 line
2.14.1
PATCH 1/7 tracing: Remove unnecessary hist trigger struct field
From: Tom Zanussi <[email protected]>
hist_field.var_idx is completely unused, so remove it.
--- kernel/trace/trace_events_hist.c | 1 - 1 file changed, 1 deletion(-)
kernel/trace/trace_events_hist.c +0 -1
@@ -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; };
Signature · 1 line
2.14.1
PATCH 2/7 tracing: Change strlen to sizeof for hist trigger static strings
In reply to Tom ZanussiFrom: Tom Zanussi <[email protected]>
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.
--- kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)
kernel/trace/trace_events_hist.c +19 -19
@@ -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 ; }
Signature · 1 line
2.14.1
PATCH 3/7 tracing: Use var_refs[] for hist trigger reference checking
In reply to Tom ZanussiFrom: Tom Zanussi <[email protected]>
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.
--- kernel/trace/trace_events_hist.c | 57 +++++----------------------------------- 1 file changed, 7 insertions(+), 50 deletions(-)
kernel/trace/trace_events_hist.c +7 -50
@@ -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; }
Signature · 1 line
2.14.1
PATCH 4/7 tracing: Remove open-coding of hist trigger var_ref management
In reply to Tom ZanussiFrom: Tom Zanussi <[email protected]>
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.
--- kernel/trace/trace_events_hist.c | 93 ++++++++++++++++++++++++++++++++-------- 1 file changed, 75 insertions(+), 18 deletions(-)
kernel/trace/trace_events_hist.c +75 -18
@@ -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 ;
Signature · 1 line
2.14.1
PATCH 5/7 tracing: Use hist trigger's var_ref array to destroy var_refs
In reply to Tom ZanussiFrom: Tom Zanussi <[email protected]>
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.
--- kernel/trace/trace_events_hist.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)
kernel/trace/trace_events_hist.c +19 -5
@@ -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,
Signature · 1 line
2.14.1
PATCH 6/7 tracing: Remove hist trigger synth_var_refs
In reply to Tom ZanussiFrom: Tom Zanussi <[email protected]>
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.
--- kernel/trace/trace_events_hist.c | 18 ------------------ 1 file changed, 18 deletions(-)
kernel/trace/trace_events_hist.c +0 -18
@@ -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); }
Signature · 1 line
2.14.1
PATCH 7/7 tracing: Add hist trigger comments for variable-related fields
In reply to Tom ZanussiFrom: Tom Zanussi <[email protected]>
Add a few comments to help clarify how variable and variable reference fields are used in the code.
--- kernel/trace/trace_events_hist.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)
kernel/trace/trace_events_hist.c +36 -0
@@ -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;
Signature · 1 line
2.14.1
Hi Tom,
Quoted 14 lines · Tom Zanussi
From: Tom Zanussi <[email protected]>
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()tosizeof()for string constants, intrace_events_hist.c
Quoted 7 lines
Also, in the 'tracing: Remove open-coding of hist trigger
var_refmanagement' patch, increate_var_ref(), moved the saving ofref_fieldand update ofref_field->var_ref_idxinto 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.
Thanks,
Namhyung
Quoted 24 lines
The following changes since commit
5d6ddf6acce68d1290112cb08b12fd78b201e7d5:arm64: Use
ftrace_graph_get_ret_stack()instead ofcurr_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.gitftrace/hist-var-ref-cleanup-v1Tom 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 triggervar_refmanagement
tracing: Use hist trigger'svar_refarray to destroyvar_refs
tracing: Remove hist triggersynth_var_refs
tracing: Add hist trigger comments for variable-related fieldskernel/trace/trace_events_hist.c | 267 +++++++++++++++++++++++---------------- 1 file changed, 156 insertions(+), 111 deletions(-)Signature · 1 line
2.14.1
Re: [PATCH 3/7] tracing: Use var_refs[] for hist trigger reference checking
In reply to Tom ZanussiHi Tom,
Quoted 9 lines · On Tue, 18 Dec 2018 14:33:22 -0600 Tom Zanussi <[email protected]> wrote:
From: Tom Zanussi <[email protected]>
Since all the variable reference
hist_fieldsare 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.
Quoted 49 lines
Signed-off-by Tom Zanussi <[email protected]>--- kernel/trace/trace_events_hist.c | 57 +++++----------------------------------- 1 file changed, 7 insertions(+), 50 deletions(-)kernel/trace/trace_events_hist.c +1
-33@@ -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];
kernel/trace/trace_events_hist.c check_field_for_var_ref(struct hist_field *hist_field, · line 1308
Quoted 10 lines
kernel/trace/trace_events_hist.c +3
-5- 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.
Quoted 13 lines
kernel/trace/trace_events_hist.c +0
-8}@@ -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; - } -
kernel/trace/trace_events_hist.c static struct hist_field *find_var_ref(struct hist_trigger_data *hist_data, · line 1323
Quoted 10 lines
kernel/trace/trace_events_hist.c +3
-4-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.
Thank you,
Quoted 3 lines
Signature · 1 line
2.14.1
Signature · 1 line
Masami Hiramatsu <[email protected]>
Hi Tom,
Quoted 14 lines · On Tue, 18 Dec 2018 14:33:19 -0600 Tom Zanussi <[email protected]> wrote:
From: Tom Zanussi <[email protected]>
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()tosizeof()for string constants, intrace_events_hist.c
Quoted 7 lines
Also, in the 'tracing: Remove open-coding of hist trigger
var_refmanagement' patch, increate_var_ref(), moved the saving ofref_fieldand update ofref_field->var_ref_idxinto 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 :-)
for the series.
Thank you,
Quoted 27 lines
Tom
The following changes since commit
5d6ddf6acce68d1290112cb08b12fd78b201e7d5:arm64: Use
ftrace_graph_get_ret_stack()instead ofcurr_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.gitftrace/hist-var-ref-cleanup-v1Tom 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 triggervar_refmanagement
tracing: Use hist trigger'svar_refarray to destroyvar_refs
tracing: Remove hist triggersynth_var_refs
tracing: Add hist trigger comments for variable-related fieldskernel/trace/trace_events_hist.c | 267 +++++++++++++++++++++++---------------- 1 file changed, 156 insertions(+), 111 deletions(-)Signature · 1 line
2.14.1
Signature · 1 line
Masami Hiramatsu <[email protected]>
Hi Namhyung,
Quoted 28 lines · Namhyung Kim
Hi Tom,
Quoted 25 lines · Tom Zanussi
From: Tom Zanussi <[email protected]>
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()tosizeof()for string
constants, intrace_events_hist.cAlso, in the 'tracing: Remove open-coding of hist trigger
var_refmanagement' patch, increate_var_ref(), moved the saving ofref_field
and update ofref_field->var_ref_idxinto the 'if' as pointed out by
Dan Carpenter/smatch 0-day robot.
Quoted 5 lines
Quoted 2 lines
It doesn't introduce any functional changes and can be applied independently of the other patchset.
Acked-by Namhyung Kim <[email protected]>
Thanks!
Tom
Re: [PATCH 3/7] tracing: Use var_refs[] for hist trigger reference checking
In reply to Masami HiramatsuHi Masami,
Quoted 85 lines · Masami Hiramatsu
Hi Tom,
Quoted 11 lines · On Tue, 18 Dec 2018 14:33:22 -0600 Tom Zanussi <[email protected]> wrote:
From: Tom Zanussi <[email protected]>
Since all the variable reference
hist_fieldsare 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.
Quoted 66 lines
Signed-off-by Tom Zanussi <[email protected]>--- kernel/trace/trace_events_hist.c | 57 +++++----------------------- ------------ 1 file changed, 7 insertions(+), 50 deletions(-)kernel/trace/trace_events_hist.c +4
-38@@ -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;
Quoted 5 lines
Quoted 1 line
return found;
It seems we don't need "found" var here. Just return
hist_fieldor NULL.
OK, will change these and resubmit shortly.
Thanks,
Tom
Quoted 30 lines · Masami Hiramatsu
Hi Tom,
Quoted 25 lines · On Tue, 18 Dec 2018 14:33:19 -0600 Tom Zanussi <[email protected]> wrote:
From: Tom Zanussi <[email protected]>
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()tosizeof()for string
constants, intrace_events_hist.cAlso, in the 'tracing: Remove open-coding of hist trigger
var_refmanagement' patch, increate_var_ref(), moved the saving ofref_field
and update ofref_field->var_ref_idxinto the 'if' as pointed out by
Dan Carpenter/smatch 0-day robot.
Quoted 9 lines
Quoted 2 lines
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 <[email protected]>for the series.
Thanks, Masami!
Tom
Steven Rostedt
maintainerRe: [PATCH 3/7] tracing: Use var_refs[] for hist trigger reference checking
In reply to Tom ZanussiQuoted 11 lines · On Wed, 19 Dec 2018 09:01:43 -0600 Tom Zanussi <[email protected]> wrote:
Quoted 9 lines
kernel/trace/trace_events_hist.c check_field_for_var_ref(struct hist_field *hist_field, · line 1308
Quoted 5 lines
kernel/trace/trace_events_hist.c +3
-0+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_fieldor 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
Hi Steve,
Quoted 32 lines · Steven Rostedt
Quoted 4 lines · On Wed, 19 Dec 2018 09:01:43 -0600 Tom Zanussi <[email protected]> wrote:
Quoted 4 lines
Quoted 4 lines
kernel/trace/trace_events_hist.c +3
-0+if (hist_field && hist_field->var.idx == var_idx && + hist_field->var.hist_data == var_data) + found = hist_field;Quoted 8 lines
Quoted 6 lines
Quoted 1 line
return found;
It seems we don't need "found" var here. Just return
hist_fieldor
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.
Quoted 7 lines
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
Quoted 1 line
-- Steve
PATCH v2 3/7 tracing: Use var_refs[] for hist trigger reference checking
In reply to Tom ZanussiSince 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.
--- kernel/trace/trace_events_hist.c | 68 +++++++--------------------------------- 1 file changed, 11 insertions(+), 57 deletions(-)
kernel/trace/trace_events_hist.c +11 -57
@@ -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,
Signature · 1 line
2.14.1
Steven Rostedt
maintainerRe: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings
In reply to Tom ZanussiQuoted 36 lines · On Tue, 18 Dec 2018 14:33:21 -0600 Tom Zanussi <[email protected]> wrote:
From: Tom Zanussi <[email protected]>
There's no need to use
strlen()for static strings when the length is already known, so updatetrace_events_hist.cwithsizeof()for those cases.Signed-off-by Tom Zanussi <[email protected]>--- kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)kernel/trace/trace_events_hist.c +3
-3@@ -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 ;
kernel/trace/trace_events_hist.c static int parse_assignment(char *str, struct hist_trigger_attrs *attrs) · line 1866
Quoted 8 lines
kernel/trace/trace_events_hist.c +2
-2@@ -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
Quoted 86 lines
kernel/trace/trace_events_hist.c +14
-14if (!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
Hi Steve,
Quoted 60 lines · Steven Rostedt
Quoted 43 lines · On Tue, 18 Dec 2018 14:33:21 -0600 Tom Zanussi <[email protected]> wrote:
From: Tom Zanussi <[email protected]>
There's no need to use
strlen()for static strings when the length is
already known, so updatetrace_events_hist.cwithsizeof()for those
cases.Signed-off-by Tom Zanussi <[email protected]>--- kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++--------- ---------- 1 file changed, 19 insertions(+), 19 deletions(-)kernel/trace/trace_events_hist.c +2
-3@@ -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) ||
kernel/trace/trace_events_hist.c +1
-0+ (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) { attrs->action_str[attrs->n_actions] = kstrdup(str,
GFP_KERNEL);kernel/trace/trace_events_hist.c +0
-0if (!attrs->action_str[attrs->n_actions]) { ret = -ENOMEM ;kernel/trace/trace_events_hist.c static int parse_assignment(char *str, · line 1867
Quoted 9 lines
kernel/trace/trace_events_hist.c +2
-2@@ -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:
Quoted 7 lines
#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
Quoted 73 lines · Tom Zanussi
Hi Steve,
Quoted 70 lines · Steven Rostedt
Quoted 49 lines · On Tue, 18 Dec 2018 14:33:21 -0600 Tom Zanussi <[email protected]> wrote:
From: Tom Zanussi <[email protected]>
There's no need to use
strlen()for static strings when the length
is
already known, so updatetrace_events_hist.cwithsizeof()for those
cases.Signed-off-by Tom Zanussi <[email protected]>--- kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++------- -- ---------- 1 file changed, 19 insertions(+), 19 deletions(-)kernel/trace/trace_events_hist.c +1
-1@@ -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,']' );kernel/trace/trace_events_hist.c +0
-1if (!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)||
kernel/trace/trace_events_hist.c +1
-1- (strncmp(str, "onmax(", strlen("onmax(")) == 0)) { + if ((strncmp(str, "onmatch(", sizeof("onmatch(") - 1) ==0) ||
kernel/trace/trace_events_hist.c +1
-0+ (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)){
attrs->action_str[attrs->n_actions] =
kstrdup(str,GFP_KERNEL);kernel/trace/trace_events_hist.c +0
-0if (!attrs->action_str[attrs->n_actions]) { ret = -ENOMEM ;kernel/trace/trace_events_hist.c static int parse_assignment(char *str, · line 1867
Quoted 9 lines
kernel/trace/trace_events_hist.c +2
-2@@ -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)
Quoted 9 lines
Quoted 4 lines
?
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).
--- kernel/trace/trace_events_hist.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
kernel/trace/trace_events_hist.c +17 -14
@@ -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);
Signature · 1 line
2.14.1
Quoted 1 line
Tom
Re: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings
In reply to Tom ZanussiQuoted 66 lines · Tom Zanussi
Hi Steve,
Quoted 63 lines · Steven Rostedt
Quoted 43 lines · On Tue, 18 Dec 2018 14:33:21 -0600 Tom Zanussi <[email protected]> wrote:
From: Tom Zanussi <[email protected]>
There's no need to use
strlen()for static strings when the length is
already known, so updatetrace_events_hist.cwithsizeof()for those
cases.Signed-off-by Tom Zanussi <[email protected]>--- kernel/trace/trace_events_hist.c | 38 +++++++++++++++++++--------- ---------- 1 file changed, 19 insertions(+), 19 deletions(-)kernel/trace/trace_events_hist.c +2
-3@@ -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) ||
kernel/trace/trace_events_hist.c +1
-0+ (strncmp(str, "onmax(", sizeof("onmax(") - 1) == 0)) { attrs->action_str[attrs->n_actions] = kstrdup(str,
GFP_KERNEL);kernel/trace/trace_events_hist.c +0
-0if (!attrs->action_str[attrs->n_actions]) { ret = -ENOMEM ;kernel/trace/trace_events_hist.c static int parse_assignment(char *str, · line 1867
Quoted 9 lines
kernel/trace/trace_events_hist.c +2
-2@@ -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)
Quoted 8 lines
Quoted 4 lines
?
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
In reply to Tom ZanussiQuoted 7 lines · Tom Zanussi
How's this?
[PATCH] tracing: Introduce and use
strcmp_const()for hist triggersProvide a new
strcmp_const()macro and make use of it instead of the longer and more error-pronestrncmp(str, "str", sizeof("str") - 1).
[]
kernel/trace/trace_events_hist.c
Quoted 1 line
kernel/trace/trace_events_hist.c +0
-0
[]
kernel/trace/trace_events_hist.c line 26
Quoted 6 lines
kernel/trace/trace_events_hist.c +2
-0@@ -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.
Steven Rostedt
maintainerRe: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings
In reply to Tom ZanussiQuoted 4 lines · On Wed, 19 Dec 2018 13:46:49 -0600 Tom Zanussi <[email protected]> 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
Quoted 1 line
From
664457f5cc776aa624502bc628285ea6d70ac8c9Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <[email protected]>
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)
--- kernel/trace/trace_events_hist.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
kernel/trace/trace_events_hist.c +18 -14
@@ -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);
Signature · 1 line
2.19.1
Steven Rostedt
maintainerRe: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings
In reply to Tom ZanussiQuoted 7 lines · On Wed, 19 Dec 2018 14:16:31 -0600 Tom Zanussi <[email protected]> wrote:
Quoted 4 lines
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?
Quoted 8 lines
[PATCH] tracing: Introduce and use
strcmp_const()for hist triggersProvide a new
strcmp_const()macro and make use of it instead of the longer and more error-pronestrncmp(str, "str", sizeof("str") - 1).Idea-and-macro-by Steve Rostedt <[email protected]>Signed-off-by Tom Zanussi <[email protected]>---
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
Steven Rostedt
maintainerRe: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings
In reply to Joe PerchesQuoted 9 lines · On Wed, 19 Dec 2018 12:20:19 -0800 Joe Perches <[email protected]> wrote:
Quoted 3 lines
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
Quoted 3 lines
the
strcmp_constmacro does seem to make sense as the copy/paste/typo possibility is real.
Steven Rostedt
maintainerRe: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings
In reply to Joe PerchesQuoted 13 lines · On Wed, 19 Dec 2018 12:22:38 -0800 Joe Perches <[email protected]> wrote:
Quoted 7 lines · Tom Zanussi
How's this?
[PATCH] tracing: Introduce and use
strcmp_const()for hist triggersProvide a new
strcmp_const()macro and make use of it instead of the longer and more error-pronestrncmp(str, "str", sizeof("str") - 1).[]
kernel/trace/trace_events_hist.c
Quoted 1 line
kernel/trace/trace_events_hist.c +0
-0[]
Quoted 2 lines
kernel/trace/trace_events_hist.c +0
-0@@ -22,6 +22,9 @@
Quoted 7 lines
kernel/trace/trace_events_hist.c line 26
Quoted 4 lines
kernel/trace/trace_events_hist.c +2
-0#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_constand 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
In reply to Steven RostedtQuoted 11 lines · Steven Rostedt
Quoted 8 lines · On Wed, 19 Dec 2018 12:20:19 -0800 Joe Perches <[email protected]> wrote:
Quoted 4 lines
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.
Quoted 8 lines
Quoted 2 lines
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
Quoted 8 lines
-- Steve
Quoted 3 lines
the
strcmp_constmacro 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
In reply to Steven RostedtQuoted 26 lines · Steven Rostedt
Quoted 12 lines · On Wed, 19 Dec 2018 12:22:38 -0800 Joe Perches <[email protected]> wrote:
Quoted 6 lines · Tom Zanussi
How's this?
[PATCH] tracing: Introduce and use
strcmp_const()for hist triggersProvide a new
strcmp_const()macro and make use of it instead of the longer and more error-pronestrncmp(str, "str", sizeof("str") - 1).[]
kernel/trace/trace_events_hist.c
Quoted 1 line
kernel/trace/trace_events_hist.c +0
-0[]
Quoted 2 lines
kernel/trace/trace_events_hist.c +0
-0@@ -22,6 +22,9 @@Quoted 7 lines
kernel/trace/trace_events_hist.c line 26
Quoted 4 lines
kernel/trace/trace_events_hist.c +2
-0#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_constand 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:
Quoted 7 lines
#define
strncmp_prefix(str, prefix)\
strncmp(str, prefix, strlen(prefix))in
include/linux/string.hAnd 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
Steven Rostedt
maintainerRe: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings
In reply to Tom ZanussiQuoted 15 lines · On Wed, 19 Dec 2018 14:38:39 -0600 Tom Zanussi <[email protected]> wrote:
Quoted 14 lines · Steven Rostedt
Quoted 10 lines · On Wed, 19 Dec 2018 12:20:19 -0800 Joe Perches <[email protected]> wrote:
Quoted 4 lines
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.
Quoted 9 lines
Quoted 5 lines
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
Steven Rostedt
maintainerRe: [PATCH 2/7] tracing: Change strlen to sizeof for hist trigger static strings
In reply to Joe PerchesQuoted 8 lines · On Wed, 19 Dec 2018 12:51:59 -0800 Joe Perches <[email protected]> wrote:
Quoted 7 lines
#define
strncmp_prefix(str, prefix)\
strncmp(str, prefix, strlen(prefix))in
include/linux/string.hAnd go around and use that throughout the kernel. By doing a quick grep, I already spotted a few bugs.
Quoted 6 lines
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.
Quoted 6 lines
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
In reply to Steven RostedtQuoted 24 lines · Steven Rostedt
Quoted 21 lines · On Wed, 19 Dec 2018 14:38:39 -0600 Tom Zanussi <[email protected]> wrote:
Quoted 19 lines · Steven Rostedt
Quoted 10 lines · On Wed, 19 Dec 2018 12:20:19 -0800 Joe Perches <[email protected]> wrote:
Quoted 4 lines
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.
Quoted 6 lines
Quoted 3 lines
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.