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

[Lluís Vilanova]

Daniel P Berrange writes:

> 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 <address@hidden>
>> > Signed-off-by: Daniel P. Berrange <address@hidden>
>> > ---
>> >  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.

Yes. I was actually thinking of porting the checks on other functions you
changed to g_str_equal().


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

That'll work too, and it's one of the reasons why I proposed to merge pattern
and exact name matching into the iterator interface.

I only added exact name matches to avoid the extra typing of the iteration loop
and a final check to see if any event matched. But trace_event_name() is only
used in two places, making trace_event_name() less useful.

We only have to be careful with the semantic difference between non-pattern and
pattern-based operations in the UI. Non-pattern operations have to show an error
if there is no match, while pattern-based operations don't complain if the
pattern has no matches.


Cheers,
  Lluis