| Message ID | 20240107203258.37e26d2b@gandalf.local.home (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4f1991a92cfe89096b2d1f5583a2e093bdd55c37 |
| Headers | show |
| Series | tracing histograms: Simplify parse_actions() function | expand |
On Mon, Jan 8, 2024 at 3:31 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > The parse_actions() function uses 'len = str_has_prefix()' to test which > action is in the string being parsed. But then it goes and repeats the > logic for each different action. This logic can be simplified and > duplicate code can be removed as 'len' contains the length of the found > prefix which should be used for all actions. > Link: https://lore.kernel.org/all/20240107112044.6702cb66@gandalf.local.home/ > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> If you want Link to be formally a tag, you should drop the following blank line. > + if ((len = str_has_prefix(str, "onmatch("))) > + hid = HANDLER_ONMATCH; > + else if ((len = str_has_prefix(str, "onmax("))) > + hid = HANDLER_ONMAX; > + else if ((len = str_has_prefix(str, "onchange("))) > + hid = HANDLER_ONCHANGE; The repeating check for ( might be moved out as well after this like if (str[len] != '(') { // not sure if you need data to be assigned here as well ret = -EINVAL; ... }
On Mon, 8 Jan 2024 10:32:14 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jan 8, 2024 at 3:31 AM Steven Rostedt <rostedt@goodmis.org> wrote: > > > > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > > > The parse_actions() function uses 'len = str_has_prefix()' to test which > > action is in the string being parsed. But then it goes and repeats the > > logic for each different action. This logic can be simplified and > > duplicate code can be removed as 'len' contains the length of the found > > prefix which should be used for all actions. > > > Link: https://lore.kernel.org/all/20240107112044.6702cb66@gandalf.local.home/ > > > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > > If you want Link to be formally a tag, you should drop the following > blank line. The link is for humans not for parsers. > > > > + if ((len = str_has_prefix(str, "onmatch("))) > > + hid = HANDLER_ONMATCH; > > + else if ((len = str_has_prefix(str, "onmax("))) > > + hid = HANDLER_ONMAX; > > + else if ((len = str_has_prefix(str, "onchange("))) > > + hid = HANDLER_ONCHANGE; > > The repeating check for ( might be moved out as well after this like > > if (str[len] != '(') { > // not sure if you need data to be assigned here as well > ret = -EINVAL; > ... > } > Not sure how that makes it any better. It adds more code. I could start with checking the "on" before checking for "match", "max" and "change", but that just makes it more complex. -- Steve
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c index 5ecf3c8bde20..6ece1308d36a 100644 --- a/kernel/trace/trace_events_hist.c +++ b/kernel/trace/trace_events_hist.c @@ -4805,36 +4805,35 @@ static int parse_actions(struct hist_trigger_data *hist_data) int len; for (i = 0; i < hist_data->attrs->n_actions; i++) { + enum handler_id hid = 0; + char *action_str; + str = hist_data->attrs->action_str[i]; - if ((len = str_has_prefix(str, "onmatch("))) { - char *action_str = str + len; + if ((len = str_has_prefix(str, "onmatch("))) + hid = HANDLER_ONMATCH; + else if ((len = str_has_prefix(str, "onmax("))) + hid = HANDLER_ONMAX; + else if ((len = str_has_prefix(str, "onchange("))) + hid = HANDLER_ONCHANGE; - data = onmatch_parse(tr, action_str); - if (IS_ERR(data)) { - ret = PTR_ERR(data); - break; - } - } else if ((len = str_has_prefix(str, "onmax("))) { - char *action_str = str + len; + action_str = str + len; - data = track_data_parse(hist_data, action_str, - HANDLER_ONMAX); - if (IS_ERR(data)) { - ret = PTR_ERR(data); - break; - } - } else if ((len = str_has_prefix(str, "onchange("))) { - char *action_str = str + len; + switch (hid) { + case HANDLER_ONMATCH: + data = onmatch_parse(tr, action_str); + break; + case HANDLER_ONMAX: + case HANDLER_ONCHANGE: + data = track_data_parse(hist_data, action_str, hid); + break; + default: + data = ERR_PTR(-EINVAL); + break; + } - data = track_data_parse(hist_data, action_str, - HANDLER_ONCHANGE); - if (IS_ERR(data)) { - ret = PTR_ERR(data); - break; - } - } else { - ret = -EINVAL; + if (IS_ERR(data)) { + ret = PTR_ERR(data); break; }