Re: [PATCH 3/3] tools/lib/traceevent: Implement new traceevent APIs for accessing struct tep_handler fields

[Tzvetomir Stoyanov <tstoyanov@vmware.com>]

On Wed, Mar 20, 2019 at 6:32 PM Matt Helsley <mhelsley@vmware.com> wrote:
>
>
>
> > On Mar 19, 2019, at 4:19 AM, Tzvetomir Stoyanov <tstoyanov@vmware.com> wrote:
> >
> > As struct tep_handler definition is not exposed as part of libtraceevent API, its fields
> > cannot be accessed directly by the library users. This patch implements new APIs, which
> > can be used to access the struct tep_handler fields:
> > tep_reset_flag(), tep_check_flag(), tep_set_parsing_failures(), tep_get_parsing_failures()
> > tep_get_header_page_ts_size(), tep_is_old_format(), tep_set_print_raw() and tep_set_test_filters()
> >
> > Signed-off-by: Tzvetomir Stoyanov <tstoyanov@vmware.com>
> > ---
> > tools/lib/traceevent/event-parse-api.c   | 140 +++++++++++++++++++++--
> > tools/lib/traceevent/event-parse-local.h |   2 +
> > tools/lib/traceevent/event-parse.h       |  12 +-
> > tools/lib/traceevent/plugin_kvm.c        |   2 +-
> > 4 files changed, 146 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/lib/traceevent/event-parse-api.c b/tools/lib/traceevent/event-parse-api.c
> > index 3716a9142aef..2bd2bf7c499d 100644
> > --- a/tools/lib/traceevent/event-parse-api.c
> > +++ b/tools/lib/traceevent/event-parse-api.c
> > @@ -8,6 +8,22 @@
> > #include "event-parse-local.h"
> > #include "event-utils.h"
> >
> > +/**
> > + * tep_get_event - returns the event with the given index
> > + * @tep: a handle to the tep_handle
> > + * @index: index of the requested event, in the range 0 .. nr_events
> > + *
> > + * This returns pointer to the element of the events array with the given index
> > + * If @tep is NULL, or @index is not in the range 0 .. nr_events, NULL is returned.
> > + */
> > +struct tep_event *tep_get_event(struct tep_handle *tep, int index)
> > +{
> > +     if (tep && tep->events && index < tep->nr_events)
> > +             return tep->events[index];
> > +
> > +     return NULL;
> > +}
> > +
> > /**
> >  * tep_get_first_event - returns the first event in the events array
> >  * @tep: a handle to the tep_handle
> > @@ -17,10 +33,7 @@
> >  */
> > struct tep_event *tep_get_first_event(struct tep_handle *tep)
> > {
> > -     if (tep && tep->events)
> > -             return tep->events[0];
> > -
> > -     return NULL;
> > +     return tep_get_event(tep, 0);
> > }
> >
> > /**
> > @@ -45,12 +58,41 @@ int tep_get_events_count(struct tep_handle *tep)
> >  *
> >  * This sets a flag or combination of flags from enum tep_flag
> >  */
> > -void tep_set_flag(struct tep_handle *tep, int flag)
> > +void tep_set_flag(struct tep_handle *tep, enum tep_flag flag)
> > {
> >       if (tep)
> >               tep->flags |= flag;
> > }
> >
> > +/**
> > + * tep_reset_flag - reset event parser flag
> > + * @tep: a handle to the tep_handle
> > + * @flag: flag, or combination of flags to be reseted
>
> s/reseted/reset/
>
> > + * can be any combination from enum tep_flag
> > + *
> > + * This resets a flag or combination of flags from enum tep_flag
> > + */
> > +void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag)
> > +{
> > +     if (tep)
> > +             tep->flags &= ~flag;
> > +}
>
> nit: rename to reset_flags since it’s one or more flags?
>
> > +
> > +/**
> > + * tep_check_flag - check the state of event parser flag
> > + * @tep: a handle to the tep_handle
> > + * @flag: flag, or combination of flags to be checked
> > + * can be any combination from enum tep_flag
> > + *
> > + * This checks the state of a flag or combination of flags from enum tep_flag
> > + */
> > +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag)
> > +{
> > +     if (tep)
> > +             return (tep->flags & flag);
> > +     return 0;
> > +}
>
> This returns a subset of the flags directly — it doesn’t really check them -- that’s up to the caller.
> So  I’d say this is more of a “getter" than a “checker”.
>
> If returning a “boolean” is the true intent of the API then it should be:
>
> return (tep->flags & flag) == flag;
>
> > +
> > unsigned short tep_data2host2(struct tep_handle *pevent, unsigned short data)
> > {
> >       unsigned short swap;
> > @@ -113,6 +155,20 @@ int tep_get_header_page_size(struct tep_handle *pevent)
> >       return 0;
> > }
> >
> > +/**
> > + * tep_get_header_page_ts_size - get size of the time stamp in the header page
> > + * @tep: a handle to the tep_handle
> > + *
> > + * This returns size of the time stamp in the header page
> > + * If @tep is NULL, 0 is returned.
> > + */
> > +int tep_get_header_page_ts_size(struct tep_handle *tep)
> > +{
> > +     if (tep)
> > +             return tep->header_page_ts_size;
> > +     return 0;
> > +}
> > +
> > /**
> >  * tep_get_cpus - get the number of CPUs
> >  * @pevent: a handle to the tep_handle
> > @@ -194,13 +250,13 @@ void tep_set_page_size(struct tep_handle *pevent, int _page_size)
> > }
> >
> > /**
> > - * tep_file_bigendian - get if the file is in big endian order
> > + * tep_is_file_bigendian - get if the file is in big endian order
> >  * @pevent: a handle to the tep_handle
> >  *
> >  * This returns if the file is in big endian order
> >  * If @pevent is NULL, 0 is returned.
> >  */
> > -int tep_file_bigendian(struct tep_handle *pevent)
> > +int tep_is_file_bigendian(struct tep_handle *pevent)
> > {
> >       if (pevent)
> >               return pevent->file_bigendian;
>
> strictly speaking this is more of a getter than a boolean test
>
> <snip>
>
> > +/**
> > + * tep_is_old_format - get if an old kernel is used
> > + * @tep: a handle to the tep_handle
> > + *
> > + * This returns 1, if an old kernel is used to generate the tracing events or
> > + * 0 if a new kernel is used. Old kernels did not have header page info.
> > + * If @pevent is NULL, 0 is returned.
> > + */
> > +int tep_is_old_format(struct tep_handle *tep)
> > +{
> > +     if (tep)
> > +             return tep->old_format;
>
> Same here.
>
> There may be value in restricting these to boolean returns. Otherwise this function of the API and those implemented like it could be “abused” as getters. That might complicate things in the future if, someday, it would be nice to change things around inside the opaque struct tep_handle’s  old_format field without breaking callers of tep_is_old_format().
>
> int tep_is_foo(...)
> {
>         if (tep)
>                 return (tep->value & foo)  == foo; /* flag test */
> …
>
> or
>                 return !!(tep->value); /*  is it non-zero test */
>
> Also, for  these “tep_is_” functions it might be more descriptive to use bool as a return value. Folks may have vetoed use of “bool" before I started looking at this project so feel free to ignore me if that’s the case.
>

All these new APIs are used only in trace-cmd / kernelshark, so they
can be changed / renamed with minimal efforts.
Although the patch was originally intended as syncing trace-cmd repo
upstream, now is the right time to clean up the traceevent API,
I'll backport again renamed / changed  APIs in trace-cmd, when the
patched is merged upstream. I prefer to keep the "checker" names and
to
change the logic and returned types to "bool", as APIs callers (in
trace-cmd) expect it.

Thanks Matt

> <snip>
>
> > diff --git a/tools/lib/traceevent/event-parse.h b/tools/lib/traceevent/event-parse.h
> > index aec48f2aea8a..f695688f38fd 100644
> > --- a/tools/lib/traceevent/event-parse.h
> > +++ b/tools/lib/traceevent/event-parse.h
> > @@ -408,7 +408,9 @@ void tep_print_plugins(struct trace_seq *s,
> > /* tep_handle */
> > typedef char *(tep_func_resolver_t)(void *priv,
> >                                   unsigned long long *addrp, char **modp);
> > -void tep_set_flag(struct tep_handle *tep, int flag);
> > +void tep_set_flag(struct tep_handle *tep, enum tep_flag flag);
> > +void tep_reset_flag(struct tep_handle *tep, enum tep_flag flag);
> > +int tep_check_flag(struct tep_handle *tep, enum tep_flag flag);
> >
> > static inline int tep_host_bigendian(void)
> > {
>
> Does another series rename this to: tep_is_host_bigendian() ?
>
> Cheers,
>      -Matt
>


-- 

Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center