Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[patch] trace: find the correct ftrace event

2 views
Skip to first unread message

Dan Carpenter

unread,
Mar 20, 2010, 9:30:01 AM3/20/10
to
The original code doesn't work because "call" is never NULL there.

Signed-off-by: Dan Carpenter <err...@gmail.com>
---
My code should work, but it seems like there should but it seems like
there should be a more elegant way to do this?

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 4615f62..6070c70 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1388,16 +1388,19 @@ int ftrace_profile_set_filter(struct perf_event *event, int event_id,
struct event_filter *filter;
struct filter_parse_state *ps;
struct ftrace_event_call *call = NULL;
+ int found = 0;

mutex_lock(&event_mutex);

list_for_each_entry(call, &ftrace_events, list) {
- if (call->id == event_id)
+ if (call->id == event_id) {
+ found = 1;
break;
+ }
}

err = -EINVAL;
- if (!call)
+ if (!found)
goto out_unlock;

err = -EEXIST;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Steven Rostedt

unread,
Mar 20, 2010, 9:40:02 AM3/20/10
to
On Sat, 2010-03-20 at 16:19 +0300, Dan Carpenter wrote:
> The original code doesn't work because "call" is never NULL there.

A one line change would have done the same ;-)

> Signed-off-by: Dan Carpenter <err...@gmail.com>
> ---
> My code should work, but it seems like there should but it seems like
> there should be a more elegant way to do this?
>
> diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> index 4615f62..6070c70 100644
> --- a/kernel/trace/trace_events_filter.c
> +++ b/kernel/trace/trace_events_filter.c
> @@ -1388,16 +1388,19 @@ int ftrace_profile_set_filter(struct perf_event *event, int event_id,
> struct event_filter *filter;
> struct filter_parse_state *ps;
> struct ftrace_event_call *call = NULL;
> + int found = 0;
>
> mutex_lock(&event_mutex);
>
> list_for_each_entry(call, &ftrace_events, list) {
> - if (call->id == event_id)
> + if (call->id == event_id) {
> + found = 1;
> break;
> + }
> }
>
> err = -EINVAL;
> - if (!call)

if (call == &ftrace_events)

-- Steve

Dan Carpenter

unread,
Mar 20, 2010, 10:00:02 AM3/20/10
to

Man... That makes my head hurt. It only works because ->list is the
first element in the struct.

We'd need a cast.

Is that really cleaner?

regards,
dan carpenter

Steven Rostedt

unread,
Mar 20, 2010, 10:10:02 AM3/20/10
to
On Sat, 2010-03-20 at 16:56 +0300, Dan Carpenter wrote:

> > > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
> > > index 4615f62..6070c70 100644
> > > --- a/kernel/trace/trace_events_filter.c
> > > +++ b/kernel/trace/trace_events_filter.c
> > > @@ -1388,16 +1388,19 @@ int ftrace_profile_set_filter(struct perf_event *event, int event_id,
> > > struct event_filter *filter;
> > > struct filter_parse_state *ps;
> > > struct ftrace_event_call *call = NULL;
> > > + int found = 0;
> > >
> > > mutex_lock(&event_mutex);
> > >
> > > list_for_each_entry(call, &ftrace_events, list) {
> > > - if (call->id == event_id)
> > > + if (call->id == event_id) {
> > > + found = 1;
> > > break;
> > > + }
> > > }
> > >
> > > err = -EINVAL;
> > > - if (!call)
> >
> > if (call == &ftrace_events)
> >
>
> Man... That makes my head hurt. It only works because ->list is the
> first element in the struct.
>
> We'd need a cast.

Oops, I missed the fact that it was list_*_entry(). That should be:

if (call->list != &ftrace_events)

No cast needed.

>
> Is that really cleaner?

Perhaps not, but I think it is more elegant ;-)

-- Steve

Dan Carpenter

unread,
Mar 20, 2010, 10:40:01 AM3/20/10
to

Hm... "list" is a struct not a pointer to a struct.

So it would have to be:

if (&call->list == &ftrace_events)
goto out_unlock;

That's how the list_for_each_entry() determines the end of the list as
well.

I'll send a patch to do that.

regards,
dan carpenter

Dan Carpenter

unread,
Mar 20, 2010, 10:40:01 AM3/20/10
to
The original code doesn't work because "call" is never NULL there.

Signed-off-by: Dan Carpenter <err...@gmail.com>
---
The original patch was fine, but this version is more tasteful. ;)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 4615f62..c1a263a 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1397,7 +1397,7 @@ int ftrace_profile_set_filter(struct perf_event *event, int event_id,


}

err = -EINVAL;
- if (!call)

+ if (&call->list == &ftrace_events)


goto out_unlock;

err = -EEXIST;

Steven Rostedt

unread,
Mar 20, 2010, 9:10:02 PM3/20/10
to
On Sat, 2010-03-20 at 17:39 +0300, Dan Carpenter wrote:
> The original code doesn't work because "call" is never NULL there.
>
> Signed-off-by: Dan Carpenter <err...@gmail.com>

Thanks, I'll apply and test this on Monday.

-- Steve

tip-bot for Dan Carpenter

unread,
May 7, 2010, 2:50:02 PM5/7/10
to
Commit-ID: d9f599e1e6d019968b35d2dc63074b9e8964fa69
Gitweb: http://git.kernel.org/tip/d9f599e1e6d019968b35d2dc63074b9e8964fa69
Author: Dan Carpenter <err...@gmail.com>
AuthorDate: Sat, 20 Mar 2010 17:39:11 +0300
Committer: Steven Rostedt <ros...@goodmis.org>
CommitDate: Thu, 6 May 2010 19:49:52 -0400

perf: Fix check at end of event search

The original code doesn't work because "call" is never NULL there.

Signed-off-by: Dan Carpenter <err...@gmail.com>
LKML-Reference: <20100320143911.GF5331@bicker>
Signed-off-by: Steven Rostedt <ros...@goodmis.org>
---
kernel/trace/trace_events_filter.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c
index 88c0b6d..58092d8 100644
--- a/kernel/trace/trace_events_filter.c
+++ b/kernel/trace/trace_events_filter.c
@@ -1398,7 +1398,7 @@ int ftrace_profile_set_filter(struct perf_event *event, int event_id,

0 new messages