Re: [Qemu-devel] [PATCH v2 2/6] 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       | 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.


> @@ -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()?

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

Cheers,
  Lluis