[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 02/18] trace: convert code to use event itera
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v3 02/18] trace: convert code to use event iterators |
Date: |
Tue, 20 Sep 2016 14:13:38 +0100 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
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 <address@hidden>
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> > 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 :|
- [Qemu-devel] [PATCH v3 00/18] Refactor trace to allow modular build, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 01/18] trace: add trace event iterator APIs, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 03/18] trace: remove some now unused functions, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 05/18] trace: remove duplicate control.h includes in generated-tracers.h, Daniel P. Berrange, 2016/09/19
- [Qemu-devel] [PATCH v3 04/18] trace: remove global 'uint16 dstate[]' array, Daniel P. Berrange, 2016/09/19