lkml.org 
[lkml]   [2019]   [Jul]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] tools/lib/traceevent, tools/perf: Move struct tep_handler definition in a local header file
    Date
    Hi Andres,

    On Fri, Jul 26, 2019 at 6:58 AM Andres Freund <andres@anarazel.de> wrote:
    >
    > Hi,
    >
    ...
    >
    > Is just plain wrong, as:
    >
    > > - return pevent->events[idx];
    > > + return (all_events + idx);
    >
    > that's not a valid conversion. ->events isn't an array of tep_handle,
    > it's an array of tep_handle* (and even if it were, the previous notation
    > would have needed to dereference the value to make it comparable). What
    > this does is look idx behind the individually allocated event. Which is
    > incorrect.
    >
    You are right, it is a bug.
    >
    ...
    >
    > The following patch fixes this for me. I can polish it up, but I'm
    > wondering if I'm missing something here?
    >
    > diff --git i/tools/lib/traceevent/event-parse.h w/tools/lib/traceevent/event-parse.h
    > index 642f68ab5fb2..7ebc9b5308d4 100644
    > --- i/tools/lib/traceevent/event-parse.h
    > +++ w/tools/lib/traceevent/event-parse.h
    > @@ -517,6 +517,7 @@ int tep_read_number_field(struct tep_format_field *field, const void *data,
    > unsigned long long *value);
    >
    > struct tep_event *tep_get_first_event(struct tep_handle *tep);
    > +struct tep_event *tep_get_event(struct tep_handle *tep, int index);
    > int tep_get_events_count(struct tep_handle *tep);
    > struct tep_event *tep_find_event(struct tep_handle *tep, int id);
    >
    > diff --git i/tools/perf/util/trace-event-parse.c w/tools/perf/util/trace-event-parse.c
    > index 62bc61155dd1..6a035ffd58ac 100644
    > --- i/tools/perf/util/trace-event-parse.c
    > +++ w/tools/perf/util/trace-event-parse.c
    > @@ -179,28 +179,26 @@ struct tep_event *trace_find_next_event(struct tep_handle *pevent,
    > {
    > static int idx;
    > int events_count;
    > - struct tep_event *all_events;
    >
    > - all_events = tep_get_first_event(pevent);
    > events_count = tep_get_events_count(pevent);
    > - if (!pevent || !all_events || events_count < 1)
    > + if (!pevent || events_count < 1)
    > return NULL;
    >
    > if (!event) {
    > idx = 0;
    > - return all_events;
    > + return tep_get_event(pevent, 0);
    > }
    >
    > - if (idx < events_count && event == (all_events + idx)) {
    > + if (idx < events_count && event == tep_get_event(pevent, idx)) {
    > idx++;
    > if (idx == events_count)
    > return NULL;
    > - return (all_events + idx);
    > + return tep_get_event(pevent, idx);
    > }
    >
    > for (idx = 1; idx < events_count; idx++) {
    > - if (event == (all_events + (idx - 1)))
    > - return (all_events + idx);
    > + if (event == tep_get_event(pevent, idx - 1))
    > + return tep_get_event(pevent, idx);
    > }
    > return NULL;
    > }
    >
    >

    I'm OK with the first part of the patch, the changes in
    tools/lib/traceevent/event-parse.h.
    tep_get_event() is an API and is omitted in the header by mistake, it
    should be there.

    As for the fix for the crash itself, I think it would be better to
    implement your suggestion -
    removing trace_find_next_event() at all and replacing it with
    tep_get_events_count() and
    tep_get_event() loop.

    >
    >
    > Not related to this crash, but it also seems that the whole
    > trace_find_next_event() API ought to be removed. Back when it was
    >
    > -struct event *trace_find_next_event(struct event *event)
    > -{
    > - if (!event)
    > - return event_list;
    > -
    > - return event->next;
    > -}
    >
    > it made some sense, but the changes in
    >
    > commit aaf045f72335653b24784d6042be8e4aee114403
    > Author: Steven Rostedt <srostedt@redhat.com>
    > Date: 2012-04-06 00:47:56 +0200
    >
    > perf: Have perf use the new libtraceevent.a library
    >
    > seem to make the current API somewhat absurd, as evidenced by the
    > complexity in trace_find_next_event(). I mean even just removing that
    > function and changing the two callsites to simple for loops with
    > tep_get_events_count() & tep_get_event() ought to be a lot better.
    >
    > Greetings,
    >
    > Andres Freund

    Thanks!

    --
    Tzvetomir (Ceco) Stoyanov
    VMware Open Source Technology Center
    \
     
     \ /
      Last update: 2019-07-26 10:26    [W:4.973 / U:0.280 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site