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