Re: [Qemu-devel] [PATCH v2 2/6] trace: convert code to use event iterators

[Daniel P. Berrange]

On Thu, Sep 15, 2016 at 12:16:52AM +0200, Lluís Vilanova wrote:
> Daniel P Berrange writes:
> 
> > This converts the HMP/QMP monitor API implementations
> > and some internal trace control methods to use the new
> > trace event iterator APIs.
> 
> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> >  monitor.c       | 16 ++++++----
> >  trace/control.c | 94 
> > ++++++++++++++++++++++++++++++++++-----------------------
> >  trace/qmp.c     | 16 ++++++----
> >  3 files changed, 76 insertions(+), 50 deletions(-)
> 
> > diff --git a/monitor.c b/monitor.c
> > index 5c00373..7b979a6 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3335,9 +3335,11 @@ void info_trace_events_completion(ReadLineState *rs, 
> > int nb_args, const char *st
> >      len = strlen(str);
> >      readline_set_completion_index(rs, len);
> >      if (nb_args == 2) {
> > -        TraceEventID id;
> > -        for (id = 0; id < trace_event_count(); id++) {
> > -            const char *event_name = 
> > trace_event_get_name(trace_event_id(id));
> > +        TraceEventIter iter;
> > +        TraceEvent *ev;
> > +        trace_event_iter_init(&iter, NULL);
> > +        while ((ev = trace_event_iter_next(&iter)) != NULL) {
> > +            const char *event_name = trace_event_get_name(ev);
> >              if (!strncmp(str, event_name, len)) {
> >                  readline_add_completion(rs, event_name);
> >              }
> > @@ -3352,9 +3354,11 @@ void trace_event_completion(ReadLineState *rs, int 
> > nb_args, const char *str)
> >      len = strlen(str);
> >      readline_set_completion_index(rs, len);
> >      if (nb_args == 2) {
> > -        TraceEventID id;
> > -        for (id = 0; id < trace_event_count(); id++) {
> > -            const char *event_name = 
> > trace_event_get_name(trace_event_id(id));
> > +        TraceEventIter iter;
> > +        TraceEvent *ev;
> > +        trace_event_iter_init(&iter, NULL);
> > +        while ((ev = trace_event_iter_next(&iter)) != NULL) {
> > +            const char *event_name = trace_event_get_name(ev);
> >              if (!strncmp(str, event_name, len)) {
> >                  readline_add_completion(rs, event_name);
> >              }
> > diff --git a/trace/control.c b/trace/control.c
> > index b871727..8fa7ed6 100644
> > --- a/trace/control.c
> > +++ b/trace/control.c
> > @@ -60,9 +60,10 @@ TraceEvent *trace_event_name(const char *name)
> >  {
> >      assert(name != NULL);
>  
> > -    TraceEventID i;
> > -    for (i = 0; i < trace_event_count(); i++) {
> > -        TraceEvent *ev = trace_event_id(i);
> > +    TraceEventIter iter;
> > +    TraceEvent *ev;
> > +    trace_event_iter_init(&iter, NULL);
> > +    while ((ev = trace_event_iter_next(&iter)) != NULL) {
> >          if (strcmp(trace_event_get_name(ev), name) == 0) {
> >              return ev;
> >          }
> 
> You could pass "name" in the pattern argument, and then remove the
> strcmp(). It'll be simpler code, but pattern_glob() is less efficient than
> strcmp().
> 
> To solve that, maybe you could subsume exact name matching 
> (trace_event_name())
> and pattern matching into the iterator interface (strcmp() / pattern_glob()) 
> by
> either checking trace_event_is_pattern() when initializing the iterator 
> (pattern
> auto-detection), or explicitly passing either a name or pattern argument (if 
> you
> want an extra-paranoid API; via two char* or a char*+bool).
> 
> I haven't checked if that would weird other code out when using iterators for 
> a
> simple exact match.

I tend to think it is overkill to try and munge this exact match
case into the pattern match handling.

> > @@ -105,21 +106,20 @@ TraceEvent *trace_event_pattern(const char *pat, 
> > TraceEvent *ev)
> >  {
> >      assert(pat != NULL);
>  
> > -    TraceEventID i;
> > -
> > -    if (ev == NULL) {
> > -        i = -1;
> > -    } else {
> > -        i = trace_event_get_id(ev);
> > -    }
> > -    i++;
> > -
> > -    while (i < trace_event_count()) {
> > -        TraceEvent *res = trace_event_id(i);
> > -        if (pattern_glob(pat, trace_event_get_name(res))) {
> > -            return res;
> > +    bool matched = ev ? false : true;
> > +    TraceEventIter iter;
> > +    TraceEvent *thisev;
> > +    trace_event_iter_init(&iter, NULL);
> > +    while ((thisev = trace_event_iter_next(&iter)) != NULL) {
> > +        if (matched) {
> > +            if (pattern_glob(pat, trace_event_get_name(thisev))) {
> > +                return thisev;
> > +            }
> > +        } else {
> > +            if (ev == thisev) {
> > +                matched = true;
> > +            }
> >          }
> > -        i++;
> >      }
>  
> >      return NULL;
> 
> Shouldn't this pass "pat" to trace_event_iter_init() and then not use
> pattern_glob()?

Yes, you're right it should have.

> I just realized this is dropped on next patch, so ignore me.

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 :|