Re: [PATCH 1/2] trace-cmd: remove parsing_failures APIs from libtraceevent

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 17 Apr 2019 09:10:26 +0000
Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:

Let's not add this. Instead add a "parsing_failures" to the
> > tracecmd_input handle, and add:
> >
> > int tracecmd_get_parsing_failures(struct tracecmd_input *handle)
> > {
> >         return handle->parsing_failures;
> > }
> >  
> I hesitated if to add new API, or use additional parameter to the
> existing functions.
> The reason for this change is to remove "parsing_failures" from
> traceevent library,
> that's why I decided not to move it to trace-cmd library. Using new
> library API is nicer,
> I can reimplement it in this way, but we may have the same concerns
> when trace-cmd
> library comes out.
> 

That's a valid concern, but there is a difference between the
libtraceevent and libftrace (or whatever we decide to name it ;-)

The trace-cmd code will be a higher level library on top of
libtraceevent. The main difference is that the trace-cmd code has
algorithms that deal with the counter but the tep code did not. We
incorrectly added that counter to the tep code only because a higher
layer needed it. The tep layer did not understand the context of that
counter.

My concern with the counter in the tep code was that it was at the
wrong level. That is, the tep code had no processing for that counter.
To put it a different way, the tep code did not understand the context
of the counter. We had to add API to set and reset that counter, which
was a clear indication that the tep library shouldn't be the one to
store it.

Now at the trace-cmd code level (libftrace), it most definitely
understands the context of that counter. Thus the counter should be
stored at that level. We didn't need to add APIs to the trace-cmd code
to reset that counter, or increment it. Because the trace-cmd library
knows the context of the counter. That's how one knows if the library
should store the counter or not.

Let's make the parsing_failures part of the tracecmd_input handler and
it will simplify this patch quite a bit.

Does that make sense?

-- Steve


> > > +{
> > > +     return _tracecmd_read_headers(handle, parsing_failures);
> > > +}
> > > +  
> >
> > [..]
> >  
> > > diff --git a/tracecmd/trace-read.c b/tracecmd/trace-read.c
> > > index 52fa1bd..fe116cc 100644
> > > --- a/tracecmd/trace-read.c
> > > +++ b/tracecmd/trace-read.c
> > > @@ -1417,6 +1417,7 @@ void trace_report (int argc, char **argv)
> > >       unsigned long long tsoffset = 0;
> > >       unsigned long long ts2secs = 0;
> > >       unsigned long long ts2sc;
> > > +     int parsing_failures;
> > >       int show_stat = 0;
> > >       int show_funcs = 0;
> > >       int show_endian = 0;
> > > @@ -1714,10 +1715,10 @@ void trace_report (int argc, char **argv)
> > >                       tracecmd_print_events(handle, print_event);
> > >                       return;
> > >               }
> > > -
> > > -             ret = tracecmd_read_headers(handle);
> > > +             parsing_failures = 0;
> > > +             ret = tracecmd_read_headers_failures(handle, &parsing_failures);  
> >
> > Here we should do:
> >
> >                 ret = tracecmd_read_headers(handle);
> >  
> > >               if (check_event_parsing) {
> > > -                     if (ret || tep_get_parsing_failures(pevent))  
> >
> >                         if (ret || tracecmd_get_parsing_failures(handle))
> >
> > -- Steve
> >  
> > > +                     if (ret || parsing_failures)
> > >                               exit(EINVAL);
> > >                       else
> > >                               exit(0);  
> >  
> 
>