Re: [Qemu-devel] [PATCH v3 02/18] trace: convert code to use event iterators

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

On Mon, Sep 19, 2016 at 06:59:17PM +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 <stefanha@redhat.com>
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  monitor.c       | 26 ++++++++--------
> >  trace/control.c | 92 +++++++++++++++++++++++++++++++++------------------------
> >  trace/qmp.c     | 16 ++++++----
> >  3 files changed, 78 insertions(+), 56 deletions(-)
> 
> > diff --git a/monitor.c b/monitor.c
> > index 5c00373..ae70157 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -3335,13 +3335,14 @@ 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));
> > -            if (!strncmp(str, event_name, len)) {
> > -                readline_add_completion(rs, event_name);
> > -            }
> > +        TraceEventIter iter;
> > +        TraceEvent *ev;
> > +        char *pattern = g_strdup_printf("%s*", str);
> > +        trace_event_iter_init(&iter, pattern);
> > +        while ((ev = trace_event_iter_next(&iter)) != NULL) {
> > +            readline_add_completion(rs, trace_event_get_name(ev));
> >          }
> > +        g_free(pattern);
> >      }
> >  }
>  
> > @@ -3352,13 +3353,14 @@ 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));
> > -            if (!strncmp(str, event_name, len)) {
> > -                readline_add_completion(rs, event_name);
> > -            }
> > +        TraceEventIter iter;
> > +        TraceEvent *ev;
> > +        char *pattern = g_strdup_printf("%s*", str);
> > +        trace_event_iter_init(&iter, pattern);
> > +        while ((ev = trace_event_iter_next(&iter)) != NULL) {
> > +            readline_add_completion(rs, trace_event_get_name(ev));
> >          }
> > +        g_free(pattern);
> >      } else if (nb_args == 3) {
> >          add_completion_option(rs, str, "on");
> >          add_completion_option(rs, str, "off");
> > diff --git a/trace/control.c b/trace/control.c
> > index 1a96049..b471146 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;
> >          }
> > @@ -105,21 +106,18 @@ 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, pat);
> > +    while ((thisev = trace_event_iter_next(&iter)) != NULL) {
> > +        if (matched) {
> > +            return thisev;
> > +        } else {
> > +            if (ev == thisev) {
> > +                matched = true;
> > +            }
> >          }
> > -        i++;
> >      }
>  
> >      return NULL;
> > @@ -148,10 +146,11 @@ TraceEvent *trace_event_iter_next(TraceEventIter *iter)
>  
> >  void trace_list_events(void)
> >  {
> > -    int i;
> > -    for (i = 0; i < trace_event_count(); i++) {
> > -        TraceEvent *res = trace_event_id(i);
> > -        fprintf(stderr, "%s\n", trace_event_get_name(res));
> > +    TraceEventIter iter;
> > +    TraceEvent *ev;
> > +    trace_event_iter_init(&iter, NULL);
> > +    while ((ev = trace_event_iter_next(&iter)) != NULL) {
> > +        fprintf(stderr, "%s\n", trace_event_get_name(ev));
> >      }
> >  }
>  
> > @@ -159,25 +158,40 @@ static void do_trace_enable_events(const char *line_buf)
> >  {
> >      const bool enable = ('-' != line_buf[0]);
> >      const char *line_ptr = enable ? line_buf : line_buf + 1;
> > -
> > -    if (trace_event_is_pattern(line_ptr)) {
> > -        TraceEvent *ev = NULL;
> > -        while ((ev = trace_event_pattern(line_ptr, ev)) != NULL) {
> > +    TraceEventIter iter;
> > +    TraceEvent *ev;
> > +    bool is_pattern = trace_event_is_pattern(line_ptr);
> > +
> > +    trace_event_iter_init(&iter, is_pattern ? line_ptr : NULL);
> > +    while ((ev = trace_event_iter_next(&iter)) != NULL) {
> > +        bool match = false;
> > +        if (is_pattern) {
> >              if (trace_event_get_state_static(ev)) {
> > -                trace_event_set_state_dynamic_init(ev, enable);
> > +                match = true;
> >              }
> > -        }
> > -    } else {
> > -        TraceEvent *ev = trace_event_name(line_ptr);
> > -        if (ev == NULL) {
> > -            error_report("WARNING: trace event '%s' does not exist",
> > -                         line_ptr);
> > -        } else if (!trace_event_get_state_static(ev)) {
> > -            error_report("WARNING: trace event '%s' is not traceable",
> > -                         line_ptr);
> >          } else {
> > -            trace_event_set_state_dynamic_init(ev, enable);
> > +            if (g_str_equal(trace_event_get_name(ev),
> > +                            line_ptr)) {
> 
> Why do some places use strcmp() and others g_str_equal() (the former are only on
> previously existing lines).

g_str_equal is clearer to follow, as you can see what the
intended semantics are, without having to look to the end of
the expression for a == 0 or != 0 and then remember which of
them means eq vs not-eq.

> If you maintain calls to trace_event_name() instead of iterating on both paths,
> the function used to compare names would be "unique". And would also make the
> warning logic at the end easier to follow (even if you call
> _set_state_dynamic_init() from two places instead of one).

We can actually simplify in a different way - if pattern_glob is
passed a pattern without any wildcards, it'll do an exact match,
so we can just rely on that in both cases.



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