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

[Lluís Vilanova]

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

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


> +                if (!trace_event_get_state_static(ev)) {
> +                    error_report("WARNING: trace event '%s' is not 
> traceable",
> +                                 line_ptr);
> +                    return;
> +                }
> +                match = true;
> +            }
>          }
> +        if (match) {
> +            /* start tracing */
> +            trace_event_set_state_dynamic(ev, enable);
> +            if (!is_pattern) {
> +                return;
> +            }
> +        }
> +    }
> +
> +    if (!is_pattern) {
> +        error_report("WARNING: trace event '%s' does not exist",
> +                     line_ptr);
>      }
>  }
 
> @@ -293,8 +307,10 @@ char *trace_opt_parse(const char *optarg)
 
>  void trace_init_vcpu_events(void)
>  {
> -    TraceEvent *ev = NULL;
> -    while ((ev = trace_event_pattern("*", ev)) != NULL) {
> +    TraceEventIter iter;
> +    TraceEvent *ev;
> +    trace_event_iter_init(&iter, NULL);
> +    while ((ev = trace_event_iter_next(&iter)) != NULL) {
>          if (trace_event_is_vcpu(ev) &&
>              trace_event_get_state_static(ev) &&
>              trace_event_get_state_dynamic(ev)) {
> diff --git a/trace/qmp.c b/trace/qmp.c
> index 11d2564..88a907b 100644
> --- a/trace/qmp.c
> +++ b/trace/qmp.c
> @@ -52,8 +52,10 @@ static bool check_events(bool has_vcpu, bool 
> ignore_unavailable, bool is_pattern
>          return true;
>      } else {
>          /* error for unavailable events */
> -        TraceEvent *ev = NULL;
> -        while ((ev = trace_event_pattern(name, ev)) != NULL) {
> +        TraceEventIter iter;
> +        TraceEvent *ev;
> +        trace_event_iter_init(&iter, name);
> +        while ((ev = trace_event_iter_next(&iter)) != NULL) {
>              if (!ignore_unavailable && !trace_event_get_state_static(ev)) {
>                  error_setg(errp, "event \"%s\" is disabled", 
> trace_event_get_name(ev));
>                  return false;
> @@ -69,6 +71,7 @@ TraceEventInfoList *qmp_trace_event_get_state(const char 
> *name,
>  {
>      Error *err = NULL;
>      TraceEventInfoList *events = NULL;
> +    TraceEventIter iter;
>      TraceEvent *ev;
>      bool is_pattern = trace_event_is_pattern(name);
>      CPUState *cpu;
> @@ -86,8 +89,8 @@ TraceEventInfoList *qmp_trace_event_get_state(const char 
> *name,
>      }
 
>      /* Get states (all errors checked above) */
> -    ev = NULL;
> -    while ((ev = trace_event_pattern(name, ev)) != NULL) {
> +    trace_event_iter_init(&iter, is_pattern ? name : NULL);
> +    while ((ev = trace_event_iter_next(&iter)) != NULL) {
>          TraceEventInfoList *elem;
>          bool is_vcpu = trace_event_is_vcpu(ev);
>          if (has_vcpu && !is_vcpu) {
> @@ -132,6 +135,7 @@ void qmp_trace_event_set_state(const char *name, bool 
> enable,
>                                 Error **errp)
>  {
>      Error *err = NULL;
> +    TraceEventIter iter;
>      TraceEvent *ev;
>      bool is_pattern = trace_event_is_pattern(name);
>      CPUState *cpu;
> @@ -150,8 +154,8 @@ void qmp_trace_event_set_state(const char *name, bool 
> enable,
>      }
 
>      /* Apply changes (all errors checked above) */
> -    ev = NULL;
> -    while ((ev = trace_event_pattern(name, ev)) != NULL) {
> +    trace_event_iter_init(&iter, name);
> +    while ((ev = trace_event_iter_next(&iter)) != NULL) {
>          if (!trace_event_get_state_static(ev) ||
>              (has_vcpu && !trace_event_is_vcpu(ev))) {
>              continue;
> -- 
> 2.7.4


Thanks,
  Lluis