Re: [Qemu-devel] [PATCH v2 1/6] trace: add trace event iterator APIs

["Daniel P. Berrange" <berrange@redhat.com>]

On Wed, Sep 14, 2016 at 11:53:10PM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
> 
> > Currently methods which want to iterate over trace events,
> > do so using the trace_event_count() and trace_event_id()
> > methods. This leaks the concept of a single ID enum to
> > the callers. There is an alternative trace_event_pattern()
> > method which can be used in an iteration context, but its
> > design is stateless, so is not easy to expand it in the
> > future.
> 
> > This defines a formal iterator API will provide an future
> > proof way of iterating over events.
> 
> > The iterator is also able to apply a pattern match filter
> > to events, further removing the need for the pattern
> 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  trace/control.c | 20 ++++++++++++++++++++
> >  trace/control.h | 27 +++++++++++++++++++++++++++
> >  2 files changed, 47 insertions(+)
> 
> > diff --git a/trace/control.c b/trace/control.c
> > index 05d85ac..b871727 100644
> > --- a/trace/control.c
> > +++ b/trace/control.c
> > @@ -125,6 +125,26 @@ TraceEvent *trace_event_pattern(const char *pat, TraceEvent *ev)
> >      return NULL;
> >  }
>  
> > +void trace_event_iter_init(TraceEventIter *iter, const char *pattern)
> > +{
> > +    iter->event = 0;
> > +    iter->pattern = pattern;
> > +}
> > +
> > +TraceEvent *trace_event_iter_next(TraceEventIter *iter)
> > +{
> > +    while (iter->event < TRACE_EVENT_COUNT) {
> > +        if (!iter->pattern ||
> > +            pattern_glob(iter->pattern,
> > +                         trace_event_get_name(&(trace_events[iter->event])))) {
> > +            return &(trace_events[iter->event]);
> 
> That's a picky one (feel free to ignore), but can you refactor
> "&(trace_events[iter->event])" out into a variable? The long pattern_glob() call
> is a bit hard to parse.
> 
> 
> > +        }
> > +        iter->event++;
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> >  void trace_list_events(void)
> >  {
> >      int i;
> > diff --git a/trace/control.h b/trace/control.h
> > index 27a16fc..c71b405 100644
> > --- a/trace/control.h
> > +++ b/trace/control.h
> > @@ -13,6 +13,10 @@
> >  #include "qemu-common.h"
> >  #include "trace/generated-events.h"
>  
> > +typedef struct TraceEventIter {
> > +    size_t event;
> 
> Shouldn't this be TraceEventID for consistence with "trace/control.h"? But if
> you're going to drop TraceEventID in a later series feel free to ignore me.

For array indexes size_t is always the optimal type, since that matches
the maximum logical size that an array can be. It just happens that
array indexes match TraceEventID but since that'll be killed in the API
later, I figured it best to stick to the normal size_t .


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|