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 <stefa...@redhat.com> > > Signed-off-by: Daniel P. Berrange <berra...@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 :|