From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56628) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmKrt-0007uW-5s for qemu-devel@nongnu.org; Tue, 20 Sep 2016 09:13:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmKrl-0007jW-Sx for qemu-devel@nongnu.org; Tue, 20 Sep 2016 09:13:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42388) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmKrl-0007jJ-Iv for qemu-devel@nongnu.org; Tue, 20 Sep 2016 09:13:41 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F01613D955 for ; Tue, 20 Sep 2016 13:13:40 +0000 (UTC) Date: Tue, 20 Sep 2016 14:13:38 +0100 From: "Daniel P. Berrange" Message-ID: <20160920131337.GI25490@redhat.com> Reply-To: "Daniel P. Berrange" References: <1474296549-29171-1-git-send-email-berrange@redhat.com> <1474296549-29171-3-git-send-email-berrange@redhat.com> <87r38fakre.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87r38fakre.fsf@fimbulvetr.bsc.es> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 02/18] trace: convert code to use event iterators List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org, Stefan Hajnoczi On Mon, Sep 19, 2016 at 06:59:17PM +0200, Llu=C3=ADs Vilanova wrote: > Daniel P Berrange writes: >=20 > > This converts the HMP/QMP monitor API implementations > > and some internal trace control methods to use the new > > trace event iterator APIs. >=20 > > Reviewed-by: Stefan Hajnoczi > > Signed-off-by: Daniel P. Berrange > > --- > > monitor.c | 26 ++++++++-------- > > trace/control.c | 92 +++++++++++++++++++++++++++++++++--------------= ---------- > > trace/qmp.c | 16 ++++++---- > > 3 files changed, 78 insertions(+), 56 deletions(-) >=20 > > 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(ReadLineSta= te *rs, int nb_args, const char *st > > len =3D strlen(str); > > readline_set_completion_index(rs, len); > > if (nb_args =3D=3D 2) { > > - TraceEventID id; > > - for (id =3D 0; id < trace_event_count(); id++) { > > - const char *event_name =3D trace_event_get_name(trace_ev= ent_id(id)); > > - if (!strncmp(str, event_name, len)) { > > - readline_add_completion(rs, event_name); > > - } > > + TraceEventIter iter; > > + TraceEvent *ev; > > + char *pattern =3D g_strdup_printf("%s*", str); > > + trace_event_iter_init(&iter, pattern); > > + while ((ev =3D trace_event_iter_next(&iter)) !=3D NULL) { > > + readline_add_completion(rs, trace_event_get_name(ev)); > > } > > + g_free(pattern); > > } > > } > =20 > > @@ -3352,13 +3353,14 @@ void trace_event_completion(ReadLineState *rs= , int nb_args, const char *str) > > len =3D strlen(str); > > readline_set_completion_index(rs, len); > > if (nb_args =3D=3D 2) { > > - TraceEventID id; > > - for (id =3D 0; id < trace_event_count(); id++) { > > - const char *event_name =3D trace_event_get_name(trace_ev= ent_id(id)); > > - if (!strncmp(str, event_name, len)) { > > - readline_add_completion(rs, event_name); > > - } > > + TraceEventIter iter; > > + TraceEvent *ev; > > + char *pattern =3D g_strdup_printf("%s*", str); > > + trace_event_iter_init(&iter, pattern); > > + while ((ev =3D trace_event_iter_next(&iter)) !=3D NULL) { > > + readline_add_completion(rs, trace_event_get_name(ev)); > > } > > + g_free(pattern); > > } else if (nb_args =3D=3D 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 !=3D NULL); > =20 > > - TraceEventID i; > > - for (i =3D 0; i < trace_event_count(); i++) { > > - TraceEvent *ev =3D trace_event_id(i); > > + TraceEventIter iter; > > + TraceEvent *ev; > > + trace_event_iter_init(&iter, NULL); > > + while ((ev =3D trace_event_iter_next(&iter)) !=3D NULL) { > > if (strcmp(trace_event_get_name(ev), name) =3D=3D 0) { > > return ev; > > } > > @@ -105,21 +106,18 @@ TraceEvent *trace_event_pattern(const char *pat= , TraceEvent *ev) > > { > > assert(pat !=3D NULL); > =20 > > - TraceEventID i; > > - > > - if (ev =3D=3D NULL) { > > - i =3D -1; > > - } else { > > - i =3D trace_event_get_id(ev); > > - } > > - i++; > > - > > - while (i < trace_event_count()) { > > - TraceEvent *res =3D trace_event_id(i); > > - if (pattern_glob(pat, trace_event_get_name(res))) { > > - return res; > > + bool matched =3D ev ? false : true; > > + TraceEventIter iter; > > + TraceEvent *thisev; > > + trace_event_iter_init(&iter, pat); > > + while ((thisev =3D trace_event_iter_next(&iter)) !=3D NULL) { > > + if (matched) { > > + return thisev; > > + } else { > > + if (ev =3D=3D thisev) { > > + matched =3D true; > > + } > > } > > - i++; > > } > =20 > > return NULL; > > @@ -148,10 +146,11 @@ TraceEvent *trace_event_iter_next(TraceEventIte= r *iter) > =20 > > void trace_list_events(void) > > { > > - int i; > > - for (i =3D 0; i < trace_event_count(); i++) { > > - TraceEvent *res =3D 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 =3D trace_event_iter_next(&iter)) !=3D NULL) { > > + fprintf(stderr, "%s\n", trace_event_get_name(ev)); > > } > > } > =20 > > @@ -159,25 +158,40 @@ static void do_trace_enable_events(const char *= line_buf) > > { > > const bool enable =3D ('-' !=3D line_buf[0]); > > const char *line_ptr =3D enable ? line_buf : line_buf + 1; > > - > > - if (trace_event_is_pattern(line_ptr)) { > > - TraceEvent *ev =3D NULL; > > - while ((ev =3D trace_event_pattern(line_ptr, ev)) !=3D NULL)= { > > + TraceEventIter iter; > > + TraceEvent *ev; > > + bool is_pattern =3D trace_event_is_pattern(line_ptr); > > + > > + trace_event_iter_init(&iter, is_pattern ? line_ptr : NULL); > > + while ((ev =3D trace_event_iter_next(&iter)) !=3D NULL) { > > + bool match =3D false; > > + if (is_pattern) { > > if (trace_event_get_state_static(ev)) { > > - trace_event_set_state_dynamic_init(ev, enable); > > + match =3D true; > > } > > - } > > - } else { > > - TraceEvent *ev =3D trace_event_name(line_ptr); > > - if (ev =3D=3D 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)) { >=20 > Why do some places use strcmp() and others g_str_equal() (the former ar= e 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 =3D=3D 0 or !=3D 0 and then remember which of them means eq vs not-eq. > If you maintain calls to trace_event_name() instead of iterating on bot= h paths, > the function used to compare names would be "unique". And would also ma= ke 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 --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|