| Message ID | 20240108133723.031cf322@gandalf.local.home (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | tracing user_events: Simplify user_event_parse_field() parsing | expand |
On Mon, Jan 08, 2024 at 01:37:23PM -0500, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > Instead of having a bunch of if statements with: > > len = str_has_prefix(field, "__data_loc unsigned "); > if (len) > goto skip_next; > > len = str_has_prefix(field, "__data_loc "); > if (len) > goto skip_next; > > len = str_has_prefix(field, "__rel_loc unsigned "); > if (len) > goto skip_next; > > len = str_has_prefix(field, "__rel_loc "); > if (len) > goto skip_next; > > goto parse; > > skip_next: > > Consolidate it into a negative check and jump to parse if all the > str_has_prefix() calls fail. If one succeeds, it will just continue with > len equal to the proper string: > > if (!(len = str_has_prefix(field, "__data_loc unsigned ")) && > !(len = str_has_prefix(field, "__data_loc ")) && > !(len = str_has_prefix(field, "__rel_loc unsigned ")) && > !(len = str_has_prefix(field, "__rel_loc "))) { > goto parse; > } > > skip_next: > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > kernel/trace/trace_events_user.c | 22 ++++++---------------- > 1 file changed, 6 insertions(+), 16 deletions(-) > > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c > index 9365ce407426..ce0c5f1ded48 100644 > --- a/kernel/trace/trace_events_user.c > +++ b/kernel/trace/trace_events_user.c > @@ -1175,23 +1175,13 @@ static int user_event_parse_field(char *field, struct user_event *user, > goto skip_next; > } > > - len = str_has_prefix(field, "__data_loc unsigned "); > - if (len) > - goto skip_next; > - > - len = str_has_prefix(field, "__data_loc "); > - if (len) > - goto skip_next; > - > - len = str_has_prefix(field, "__rel_loc unsigned "); > - if (len) > - goto skip_next; > - > - len = str_has_prefix(field, "__rel_loc "); > - if (len) > - goto skip_next; > + if (!(len = str_has_prefix(field, "__data_loc unsigned ")) && > + !(len = str_has_prefix(field, "__data_loc ")) && > + !(len = str_has_prefix(field, "__rel_loc unsigned ")) && > + !(len = str_has_prefix(field, "__rel_loc "))) { > + goto parse; > + } This now triggers a checkpatch error: ERROR: do not use assignment in if condition #1184: FILE: kernel/trace/trace_events_user.c:1184: + if (!(len = str_has_prefix(field, "__data_loc unsigned ")) && I personally prefer to keep these files fully checkpatch clean. However, I did test these changes under the self-tests and it passed. Do they bug you that much? :) Thanks, -Beau > > - goto parse; > skip_next: > type = field; > field = strpbrk(field + len, " "); > -- > 2.43.0
On Mon, 8 Jan 2024 21:47:44 +0000 Beau Belgrave <beaub@linux.microsoft.com> wrote: > > - len = str_has_prefix(field, "__rel_loc "); > > - if (len) > > - goto skip_next; > > + if (!(len = str_has_prefix(field, "__data_loc unsigned ")) && > > + !(len = str_has_prefix(field, "__data_loc ")) && > > + !(len = str_has_prefix(field, "__rel_loc unsigned ")) && > > + !(len = str_has_prefix(field, "__rel_loc "))) { > > + goto parse; > > + } > > This now triggers a checkpatch error: > ERROR: do not use assignment in if condition What a horrible message. > #1184: FILE: kernel/trace/trace_events_user.c:1184: > + if (!(len = str_has_prefix(field, "__data_loc unsigned ")) && > > I personally prefer to keep these files fully checkpatch clean. I've stopped using checkpatch years ago because I disagreed with so much it :-p (Including this message) > However, I did test these changes under the self-tests and it passed. > > Do they bug you that much? :) No big deal if you prefer the other way. I was just doing an audit of str_has_prefix() to see what code could be cleaned up that uses it, and I found this code. If you prefer to limit your code to "checkpatch clean", I'll leave it alone. -- Steve
On Mon, 8 Jan 2024 17:13:12 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Mon, 8 Jan 2024 21:47:44 +0000 > Beau Belgrave <beaub@linux.microsoft.com> wrote: > > > > - len = str_has_prefix(field, "__rel_loc "); > > > - if (len) > > > - goto skip_next; > > > + if (!(len = str_has_prefix(field, "__data_loc unsigned ")) && > > > + !(len = str_has_prefix(field, "__data_loc ")) && > > > + !(len = str_has_prefix(field, "__rel_loc unsigned ")) && > > > + !(len = str_has_prefix(field, "__rel_loc "))) { > > > + goto parse; > > > + } > > > > This now triggers a checkpatch error: > > ERROR: do not use assignment in if condition > > What a horrible message. > > > #1184: FILE: kernel/trace/trace_events_user.c:1184: > > + if (!(len = str_has_prefix(field, "__data_loc unsigned ")) && > > > > I personally prefer to keep these files fully checkpatch clean. > > I've stopped using checkpatch years ago because I disagreed with so much it :-p > (Including this message) Note that checkpatch is a guideline and not a rule. The general rule is, if the code looks worse when applying the checkpatch rule, don't do it. - Steve
diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c index 9365ce407426..ce0c5f1ded48 100644 --- a/kernel/trace/trace_events_user.c +++ b/kernel/trace/trace_events_user.c @@ -1175,23 +1175,13 @@ static int user_event_parse_field(char *field, struct user_event *user, goto skip_next; } - len = str_has_prefix(field, "__data_loc unsigned "); - if (len) - goto skip_next; - - len = str_has_prefix(field, "__data_loc "); - if (len) - goto skip_next; - - len = str_has_prefix(field, "__rel_loc unsigned "); - if (len) - goto skip_next; - - len = str_has_prefix(field, "__rel_loc "); - if (len) - goto skip_next; + if (!(len = str_has_prefix(field, "__data_loc unsigned ")) && + !(len = str_has_prefix(field, "__data_loc ")) && + !(len = str_has_prefix(field, "__rel_loc unsigned ")) && + !(len = str_has_prefix(field, "__rel_loc "))) { + goto parse; + } - goto parse; skip_next: type = field; field = strpbrk(field + len, " ");