From: "Steven Rostedt (Google)" <[email protected]>
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/[email protected]/
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
kernel/trace/trace_events_hist.c | 49 ++++++++++++++++----------------
1 file changed, 24 insertions(+), 25 deletions(-)
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;
}
--
2.42.0
On Mon, Jan 8, 2024 at 3:31 AM Steven Rostedt <[email protected]> wrote:
>
> From: "Steven Rostedt (Google)" <[email protected]>
>
> 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/[email protected]/
>
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
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;
...
}
--
With Best Regards,
Andy Shevchenko
On Mon, 8 Jan 2024 10:32:14 +0200
Andy Shevchenko <[email protected]> wrote:
> On Mon, Jan 8, 2024 at 3:31 AM Steven Rostedt <[email protected]> wrote:
> >
> > From: "Steven Rostedt (Google)" <[email protected]>
> >
> > 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/[email protected]/
> >
> > Signed-off-by: Steven Rostedt (Google) <[email protected]>
>
> 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