On Thu, Sep 15, 2016 at 12:16:52AM +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 | 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.
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 != 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()?
Yes, you're right it should have.
> I just realized this is dropped on next patch, so ignore me.
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 :|
