Re: [PATCH] tools/lib/traceevent, tools/perf: Move struct tep_handler definition in a local header file

[Tzvetomir Stoyanov <tstoyanov@vmware.com>]

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