From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54609) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bkSfn-0004TE-Iv for qemu-devel@nongnu.org; Thu, 15 Sep 2016 05:09:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bkSfi-0004Vv-9b for qemu-devel@nongnu.org; Thu, 15 Sep 2016 05:09:34 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35060) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bkSfi-0004Vo-0F for qemu-devel@nongnu.org; Thu, 15 Sep 2016 05:09:30 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (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 5B3C3552C9 for ; Thu, 15 Sep 2016 09:09:29 +0000 (UTC) Date: Thu, 15 Sep 2016 10:09:26 +0100 From: "Daniel P. Berrange" Message-ID: <20160915090926.GD26068@redhat.com> Reply-To: "Daniel P. Berrange" References: <1473872922-23449-1-git-send-email-berrange@redhat.com> <1473872922-23449-3-git-send-email-berrange@redhat.com> <87lgyuxh2j.fsf@fimbulvetr.bsc.es> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87lgyuxh2j.fsf@fimbulvetr.bsc.es> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 2/6] 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 , Paolo Bonzini On Thu, Sep 15, 2016 at 12:16:52AM +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 | 16 ++++++---- > > trace/control.c | 94 ++++++++++++++++++++++++++++++++++-------------= ---------- > > trace/qmp.c | 16 ++++++---- > > 3 files changed, 76 insertions(+), 50 deletions(-) >=20 > > 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(ReadLineStat= e *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)); > > + TraceEventIter iter; > > + TraceEvent *ev; > > + trace_event_iter_init(&iter, NULL); > > + while ((ev =3D trace_event_iter_next(&iter)) !=3D NULL) { > > + const char *event_name =3D 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 =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)); > > + TraceEventIter iter; > > + TraceEvent *ev; > > + trace_event_iter_init(&iter, NULL); > > + while ((ev =3D trace_event_iter_next(&iter)) !=3D NULL) { > > + const char *event_name =3D 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 !=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; > > } >=20 > You could pass "name" in the pattern argument, and then remove the > strcmp(). It'll be simpler code, but pattern_glob() is less efficient t= han > strcmp(). >=20 > To solve that, maybe you could subsume exact name matching (trace_event= _name()) > and pattern matching into the iterator interface (strcmp() / pattern_gl= ob()) by > either checking trace_event_is_pattern() when initializing the iterator= (pattern > auto-detection), or explicitly passing either a name or pattern argumen= t (if you > want an extra-paranoid API; via two char* or a char*+bool). >=20 > I haven't checked if that would weird other code out when using iterato= rs for a > simple exact match. I tend to think it is overkill to try and munge this exact match case into the pattern match handling. > > @@ -105,21 +106,20 @@ 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, NULL); > > + while ((thisev =3D trace_event_iter_next(&iter)) !=3D NULL) { > > + if (matched) { > > + if (pattern_glob(pat, trace_event_get_name(thisev))) { > > + return thisev; > > + } > > + } else { > > + if (ev =3D=3D thisev) { > > + matched =3D true; > > + } > > } > > - i++; > > } > =20 > > return NULL; >=20 > Shouldn't this pass "pat" to trace_event_iter_init() and then not use > pattern_glob()? Yes, you're right it should have. > I just realized this is dropped on next patch, so ignore me. 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 :|