| Message ID | 20210802112714.67631-1-y.karadz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] libtraceevent: Add tep_print_selected_fields() | expand |
Steven, The implementation of static void tep_print_fmt_field() is incomplete. If you compare the logic there with the implementation of void tep_print_field() you will see that I am not handling the case of a field that is an "ARRAY". Any ideas what is the best way to handle this case? Thanks! Yordan On 2.08.21 г. 14:27, Yordan Karadzhov (VMware) wrote: > The new method can print only a subset of the unique data fields of > the trace event. The print format is derived from the parsing tokens > (tep_print_parse objects) of the event. As a byproduct of this change > the existing method tep_print_fields() gets upgraded to use the > formats provided by the tokens. > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > src/event-parse.c | 87 ++++++++++++++++++++++++++++++++++++++++++----- > src/event-parse.h | 3 ++ > 2 files changed, 81 insertions(+), 9 deletions(-) > > diff --git a/src/event-parse.c b/src/event-parse.c > index f42ae38..7302f3d 100644 > --- a/src/event-parse.c > +++ b/src/event-parse.c > @@ -3585,6 +3585,8 @@ tep_find_field(struct tep_event *event, const char *name) > return format; > } > > + > + > /** > * tep_find_any_field - find any field by name > * @event: handle for the event > @@ -5333,6 +5335,19 @@ static int is_printable_array(char *p, unsigned int len) > return 1; > } > > +static void dynamic_offset(struct tep_handle *tep, > + struct tep_format_field *field, > + void *data, > + unsigned int *offset, > + unsigned int *len) > +{ > + unsigned long long val; > + > + val = tep_read_number(tep, data + field->offset, field->size); > + *offset = val & SHRT_MAX; > + *len = val >> 16; > +} > + > void tep_print_field(struct trace_seq *s, void *data, > struct tep_format_field *field) > { > @@ -5343,12 +5358,9 @@ void tep_print_field(struct trace_seq *s, void *data, > if (field->flags & TEP_FIELD_IS_ARRAY) { > offset = field->offset; > len = field->size; > - if (field->flags & TEP_FIELD_IS_DYNAMIC) { > - val = tep_read_number(tep, data + offset, len); > - offset = val; > - len = offset >> 16; > - offset &= 0xffff; > - } > + if (field->flags & TEP_FIELD_IS_DYNAMIC) > + dynamic_offset(tep, field, data, &offset, &len); > + > if (field->flags & TEP_FIELD_IS_STRING && > is_printable_array(data + offset, len)) { > trace_seq_printf(s, "%s", (char *)data + offset); > @@ -5398,19 +5410,76 @@ void tep_print_field(struct trace_seq *s, void *data, > } > } > > -void tep_print_fields(struct trace_seq *s, void *data, > - int size __maybe_unused, struct tep_event *event) > +static struct tep_print_parse *parse_format_next(struct tep_print_parse *parse) > +{ > + while (parse) { > + if (strncmp(parse->format, "%", 1) == 0) > + break; > + > + parse = parse->next; > + } > + > + return parse; > +} > + > +static void tep_print_fmt_field(struct trace_seq *s, void *data, > + const char *format, > + struct tep_format_field *field) > +{ > + struct tep_handle *tep = field->event->tep; > + unsigned int len, offset; > + unsigned long long val; > + > + if (field->flags & TEP_FIELD_IS_DYNAMIC) { > + dynamic_offset(tep, field, data, &offset, &len); > + if (len) > + trace_seq_printf(s, format, (char *)data + offset); > + else > + trace_seq_printf(s, format, "(nil)"); > + } else { > + val = tep_read_number(tep, data + field->offset, field->size); > + trace_seq_printf(s, format, val); > + } > +} > + > +void tep_print_selected_fields(struct trace_seq *s, void *data, > + struct tep_event *event, > + int ignore_mask) > { > struct tep_format_field *field; > + struct tep_print_parse *parse; > + unsigned int len; > + int field_mask = 1; > > + parse = event->print_fmt.print_cache; > field = event->format.fields; > while (field) { > + parse = parse_format_next(parse); > + > + if (field_mask & ignore_mask) > + goto next; > + > trace_seq_printf(s, " %s=", field->name); > - tep_print_field(s, data, field); > + > + len = strlen(parse->format); > + if (len > 0 && parse->format[len - 1] == 'x') > + trace_seq_printf(s, "0x"); > + > + tep_print_fmt_field(s, data, parse->format, field); > + > + next: > field = field->next; > + parse = parse->next; > + field_mask *= 2; > } > } > > +void tep_print_fields(struct trace_seq *s, void *data, > + int size __maybe_unused, struct tep_event *event) > +{ > + tep_print_selected_fields(s, data, event, 0); > +} > + > static int print_function(struct trace_seq *s, const char *format, > void *data, int size, struct tep_event *event, > struct tep_print_arg *arg) > diff --git a/src/event-parse.h b/src/event-parse.h > index d4a876f..fe0fbf4 100644 > --- a/src/event-parse.h > +++ b/src/event-parse.h > @@ -547,6 +547,9 @@ void tep_print_field(struct trace_seq *s, void *data, > struct tep_format_field *field); > void tep_print_fields(struct trace_seq *s, void *data, > int size __maybe_unused, struct tep_event *event); > +void tep_print_selected_fields(struct trace_seq *s, void *data, > + struct tep_event *event, > + int ignore_mask); > int tep_strerror(struct tep_handle *tep, enum tep_errno errnum, > char *buf, size_t buflen); > >
On Mon, 2 Aug 2021 14:27:14 +0300 "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote: > The new method can print only a subset of the unique data fields of > the trace event. The print format is derived from the parsing tokens > (tep_print_parse objects) of the event. As a byproduct of this change > the existing method tep_print_fields() gets upgraded to use the > formats provided by the tokens. Why add a new API? Just fix tep_print_field(). > > Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> > --- > src/event-parse.c | 87 ++++++++++++++++++++++++++++++++++++++++++----- > src/event-parse.h | 3 ++ > 2 files changed, 81 insertions(+), 9 deletions(-) > > diff --git a/src/event-parse.c b/src/event-parse.c > index f42ae38..7302f3d 100644 > --- a/src/event-parse.c > +++ b/src/event-parse.c > @@ -3585,6 +3585,8 @@ tep_find_field(struct tep_event *event, const char *name) > return format; > } > > + > + Spurious newlines? > /** > * tep_find_any_field - find any field by name > * @event: handle for the event > @@ -5333,6 +5335,19 @@ static int is_printable_array(char *p, unsigned int len) > return 1; > } > > +static void dynamic_offset(struct tep_handle *tep, > + struct tep_format_field *field, > + void *data, > + unsigned int *offset, > + unsigned int *len) > +{ > + unsigned long long val; > + > + val = tep_read_number(tep, data + field->offset, field->size); > + *offset = val & SHRT_MAX; Don't use SHRT_MAX. Although it is the same, there's no connection of it being a short number (it only matches by coincidence), and I don't want to add any assumptions that this is a short. It's 16 bits, and that's all it is. The fact that a short is also 16 bits is just a coincidence ;-) > + *len = val >> 16; If anything, we should define: #define TEP_OFFSET_MASK 0xffff #define TEP_LEN_SHIFT 16 and then we can: *offset = val & TEP_OFFSET_MASK; *len = val >> TEP_LEN_SHIFT; Which would make it much easier to read and document what it actually is. > +} > + Also, the above should be in a separate cleanup patch that that should remove all the open coded conversions. > void tep_print_field(struct trace_seq *s, void *data, > struct tep_format_field *field) > { > @@ -5343,12 +5358,9 @@ void tep_print_field(struct trace_seq *s, void *data, > if (field->flags & TEP_FIELD_IS_ARRAY) { > offset = field->offset; > len = field->size; > - if (field->flags & TEP_FIELD_IS_DYNAMIC) { > - val = tep_read_number(tep, data + offset, len); > - offset = val; > - len = offset >> 16; > - offset &= 0xffff; > - } > + if (field->flags & TEP_FIELD_IS_DYNAMIC) > + dynamic_offset(tep, field, data, &offset, &len); > + > if (field->flags & TEP_FIELD_IS_STRING && > is_printable_array(data + offset, len)) { > trace_seq_printf(s, "%s", (char *)data + offset); > @@ -5398,19 +5410,76 @@ void tep_print_field(struct trace_seq *s, void *data, > } > } > What I was talking about is to change tep_print_field() to do something like: Take the current tep_print_field() and turn it into static _tep_print_field(). [ Not even compiled tested ] void tep_print_field(struct trace_seq *s, void *data, struct tep_format_field *field) { struct tep_event = field->event; struct tep_print_parse = event->print_fmt.print_cache; struct tep_handle *tep = event->tep; unsigned int offset, len; if (event->flags & TEP_EVENT_FL_FAILED) goto out; if (field->flags & TEP_FIELD_IS_DYNAMIC) dynamic_offset(tep, field, data, &offset, &len); for (;parse; parse = parse->next) { if (parse->type == PRINT_FMT_STRING) continue; if (parse->arg->type != TEP_PRINT_FIELD) continue; if (parse->arg->field.field->field != field) continue; print_parse_data(parse, s, data, size, event); return; } out: /* Not found */ _tep_print_field(s, data, field); } That print_parse_data() would be extracted from print_event_cache() static int print_parse_data(struct tep_print_parse *parse, struct trace_seq *s, void *data, int size, struct tep_event *event) { if (parse->len_as_arg) len_arg = eval_num_arg(data, size, event, parse->len_as_arg); switch (parse->type) { case PRINT_FMT_ARG_DIGIT: print_arg_number(s, parse->format, parse->len_as_arg ? len_arg : -1, data, size, parse->ls, event, parse->arg); break; case PRINT_FMT_ARG_POINTER: print_arg_pointer(s, parse->format, parse->len_as_arg ? len_arg : 1, data, size, event, parse->arg); break; case PRINT_FMT_ARG_STRING: print_arg_string(s, parse->format, parse->len_as_arg ? len_arg : -1, data, size, event, parse->arg); break; case PRINT_FMT_STRING: default: trace_seq_printf(s, "%s", parse->format); /* Return 1 on non field */ return 1; } /* Return 0 on field being processed */ return 0; } static void print_event_cache(struct tep_print_parse *parse, struct trace_seq *s, void *data, int size, struct tep_event *event) { int len_arg; while (parse) { parse = parse->next; parse_parse_data(parse, s, data, size, event); } } > -void tep_print_fields(struct trace_seq *s, void *data, > - int size __maybe_unused, struct tep_event *event) > +static struct tep_print_parse *parse_format_next(struct tep_print_parse *parse) > +{ > + while (parse) { > + if (strncmp(parse->format, "%", 1) == 0) > + break; > + > + parse = parse->next; > + } > + > + return parse; > +} > + > +static void tep_print_fmt_field(struct trace_seq *s, void *data, > + const char *format, > + struct tep_format_field *field) > +{ > + struct tep_handle *tep = field->event->tep; > + unsigned int len, offset; > + unsigned long long val; > + > + if (field->flags & TEP_FIELD_IS_DYNAMIC) { > + dynamic_offset(tep, field, data, &offset, &len); > + if (len) > + trace_seq_printf(s, format, (char *)data + offset); > + else > + trace_seq_printf(s, format, "(nil)"); > + } else { > + val = tep_read_number(tep, data + field->offset, field->size); > + trace_seq_printf(s, format, val); > + } > +} > + > +void tep_print_selected_fields(struct trace_seq *s, void *data, > + struct tep_event *event, > + int ignore_mask) > { > struct tep_format_field *field; > + struct tep_print_parse *parse; > + unsigned int len; > + int field_mask = 1; > > + parse = event->print_fmt.print_cache; > field = event->format.fields; > while (field) { > + parse = parse_format_next(parse); > + > + if (field_mask & ignore_mask) > + goto next; > + > trace_seq_printf(s, " %s=", field->name); > - tep_print_field(s, data, field); > + > + len = strlen(parse->format); > + if (len > 0 && parse->format[len - 1] == 'x') > + trace_seq_printf(s, "0x"); > + > + tep_print_fmt_field(s, data, parse->format, field); > + > + next: > field = field->next; > + parse = parse->next; > + field_mask *= 2; > } > } > > +void tep_print_fields(struct trace_seq *s, void *data, > + int size __maybe_unused, struct tep_event *event) > +{ > + tep_print_selected_fields(s, data, event, 0); > +} > + > static int print_function(struct trace_seq *s, const char *format, > void *data, int size, struct tep_event *event, > struct tep_print_arg *arg) > diff --git a/src/event-parse.h b/src/event-parse.h > index d4a876f..fe0fbf4 100644 > --- a/src/event-parse.h > +++ b/src/event-parse.h > @@ -547,6 +547,9 @@ void tep_print_field(struct trace_seq *s, void *data, > struct tep_format_field *field); > void tep_print_fields(struct trace_seq *s, void *data, > int size __maybe_unused, struct tep_event *event); > +void tep_print_selected_fields(struct trace_seq *s, void *data, > + struct tep_event *event, > + int ignore_mask); Again, I'm not so big on the selected fields version. Just call tep_print_field for each section, as it just adds to the trace_seq() which was created for this purpose (to avoid having to enter a list). -- Steve > int tep_strerror(struct tep_handle *tep, enum tep_errno errnum, > char *buf, size_t buflen); >
On Mon, 2 Aug 2021 14:39:11 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > The implementation of static void tep_print_fmt_field() is incomplete. > If you compare the logic there with the implementation of > void tep_print_field() you will see that I am not handling the case of > a field that is an "ARRAY". > > Any ideas what is the best way to handle this case? I think my reply to your patch should cover this. -- Steve
On 2.08.21 г. 19:29, Steven Rostedt wrote: > What I was talking about is to change tep_print_field() to do something like: > > Take the current tep_print_field() and turn it into static _tep_print_field(). > > [ Not even compiled tested ] > Hi Steven, I am not able to make sense from the code below. > void tep_print_field(struct trace_seq *s, void *data, > struct tep_format_field *field) > { > struct tep_event = field->event; > struct tep_print_parse = event->print_fmt.print_cache; > struct tep_handle *tep = event->tep; > unsigned int offset, len; > > if (event->flags & TEP_EVENT_FL_FAILED) > goto out; > > if (field->flags & TEP_FIELD_IS_DYNAMIC) > dynamic_offset(tep, field, data, &offset, &len); > > for (;parse; parse = parse->next) { You need if (!parse->arg) continue; > if (parse->type == PRINT_FMT_STRING) > continue; > if (parse->arg->type != TEP_PRINT_FIELD) > continue; I can't understand the idea of those two checks. I printed the values and they don't seem to have any selective power. > if (parse->arg->field.field->field != field) > continue; > This does not compile. I guess you mean if (parse->arg->field.field != field) continue; however "parse->arg->field.field" looks like unused memory (NULL or 0xffffffff) and this check always fails. Maybe we must call some of the process_XXX()static functions first in order to make your new version of tep_print_field() works? Thanks! Yordan > print_parse_data(parse, s, data, size, event); > return; > } > > out: > /* Not found */ >
On Tue, 3 Aug 2021 15:42:02 +0300 Yordan Karadzhov <y.karadz@gmail.com> wrote: > On 2.08.21 г. 19:29, Steven Rostedt wrote: > > What I was talking about is to change tep_print_field() to do something like: > > > > Take the current tep_print_field() and turn it into static _tep_print_field(). > > > > [ Not even compiled tested ] > > > > Hi Steven, > > I am not able to make sense from the code below. > > > void tep_print_field(struct trace_seq *s, void *data, > > struct tep_format_field *field) > > { > > struct tep_event = field->event; > > struct tep_print_parse = event->print_fmt.print_cache; > > struct tep_handle *tep = event->tep; > > unsigned int offset, len; > > > > if (event->flags & TEP_EVENT_FL_FAILED) > > goto out; > > > > if (field->flags & TEP_FIELD_IS_DYNAMIC) > > dynamic_offset(tep, field, data, &offset, &len); > > > > for (;parse; parse = parse->next) { > > You need > if (!parse->arg) > continue; Sure. Like I said, I never even compiled this ;-) > > > if (parse->type == PRINT_FMT_STRING) > > continue; > > if (parse->arg->type != TEP_PRINT_FIELD) > > continue; > > I can't understand the idea of those two checks. I printed the values and they don't seem to have any selective power. Well, the print_cache stores portions of the print fmt format string. Where if you have something like: print fmt: "fd: 0x%08lx, offset: 0x%08lx, count: 0x%08lx", ((unsigned long)(REC->fd)), ((unsigned long)(REC->offset)), ((unsigned long)(REC->count)) The "fd: " part is a PRINT_FMT_STRING. We don't want anything to do with that. But the cache part that has fields, should be an arg of type TEP_PRINT_FIELD, which should be mapped to some field to print. That is, one of the REC->fd, REC->offset or REC->count. That's what you are looking to see how to print it. As it also contains how that field should be printed normally. > > > if (parse->arg->field.field->field != field) > > continue; > > > > This does not compile. I guess you mean > if (parse->arg->field.field != field) > continue; > > however "parse->arg->field.field" looks like unused memory (NULL or 0xffffffff) and this check always fails. > > Maybe we must call some of the process_XXX()static functions first in order to make your new version of > tep_print_field() works? How are you calling it? After loading the tep with tracefs_local_events()? Because it should be loaded via tep_parse_event(), which calll parse_args() and load the args to the event. Basically we should be doing what print_event_cache() does, but only for the field we are interested in. -- Steve
diff --git a/src/event-parse.c b/src/event-parse.c index f42ae38..7302f3d 100644 --- a/src/event-parse.c +++ b/src/event-parse.c @@ -3585,6 +3585,8 @@ tep_find_field(struct tep_event *event, const char *name) return format; } + + /** * tep_find_any_field - find any field by name * @event: handle for the event @@ -5333,6 +5335,19 @@ static int is_printable_array(char *p, unsigned int len) return 1; } +static void dynamic_offset(struct tep_handle *tep, + struct tep_format_field *field, + void *data, + unsigned int *offset, + unsigned int *len) +{ + unsigned long long val; + + val = tep_read_number(tep, data + field->offset, field->size); + *offset = val & SHRT_MAX; + *len = val >> 16; +} + void tep_print_field(struct trace_seq *s, void *data, struct tep_format_field *field) { @@ -5343,12 +5358,9 @@ void tep_print_field(struct trace_seq *s, void *data, if (field->flags & TEP_FIELD_IS_ARRAY) { offset = field->offset; len = field->size; - if (field->flags & TEP_FIELD_IS_DYNAMIC) { - val = tep_read_number(tep, data + offset, len); - offset = val; - len = offset >> 16; - offset &= 0xffff; - } + if (field->flags & TEP_FIELD_IS_DYNAMIC) + dynamic_offset(tep, field, data, &offset, &len); + if (field->flags & TEP_FIELD_IS_STRING && is_printable_array(data + offset, len)) { trace_seq_printf(s, "%s", (char *)data + offset); @@ -5398,19 +5410,76 @@ void tep_print_field(struct trace_seq *s, void *data, } } -void tep_print_fields(struct trace_seq *s, void *data, - int size __maybe_unused, struct tep_event *event) +static struct tep_print_parse *parse_format_next(struct tep_print_parse *parse) +{ + while (parse) { + if (strncmp(parse->format, "%", 1) == 0) + break; + + parse = parse->next; + } + + return parse; +} + +static void tep_print_fmt_field(struct trace_seq *s, void *data, + const char *format, + struct tep_format_field *field) +{ + struct tep_handle *tep = field->event->tep; + unsigned int len, offset; + unsigned long long val; + + if (field->flags & TEP_FIELD_IS_DYNAMIC) { + dynamic_offset(tep, field, data, &offset, &len); + if (len) + trace_seq_printf(s, format, (char *)data + offset); + else + trace_seq_printf(s, format, "(nil)"); + } else { + val = tep_read_number(tep, data + field->offset, field->size); + trace_seq_printf(s, format, val); + } +} + +void tep_print_selected_fields(struct trace_seq *s, void *data, + struct tep_event *event, + int ignore_mask) { struct tep_format_field *field; + struct tep_print_parse *parse; + unsigned int len; + int field_mask = 1; + parse = event->print_fmt.print_cache; field = event->format.fields; while (field) { + parse = parse_format_next(parse); + + if (field_mask & ignore_mask) + goto next; + trace_seq_printf(s, " %s=", field->name); - tep_print_field(s, data, field); + + len = strlen(parse->format); + if (len > 0 && parse->format[len - 1] == 'x') + trace_seq_printf(s, "0x"); + + tep_print_fmt_field(s, data, parse->format, field); + + next: field = field->next; + parse = parse->next; + field_mask *= 2; } } +void tep_print_fields(struct trace_seq *s, void *data, + int size __maybe_unused, struct tep_event *event) +{ + tep_print_selected_fields(s, data, event, 0); +} + static int print_function(struct trace_seq *s, const char *format, void *data, int size, struct tep_event *event, struct tep_print_arg *arg) diff --git a/src/event-parse.h b/src/event-parse.h index d4a876f..fe0fbf4 100644 --- a/src/event-parse.h +++ b/src/event-parse.h @@ -547,6 +547,9 @@ void tep_print_field(struct trace_seq *s, void *data, struct tep_format_field *field); void tep_print_fields(struct trace_seq *s, void *data, int size __maybe_unused, struct tep_event *event); +void tep_print_selected_fields(struct trace_seq *s, void *data, + struct tep_event *event, + int ignore_mask); int tep_strerror(struct tep_handle *tep, enum tep_errno errnum, char *buf, size_t buflen);
The new method can print only a subset of the unique data fields of the trace event. The print format is derived from the parsing tokens (tep_print_parse objects) of the event. As a byproduct of this change the existing method tep_print_fields() gets upgraded to use the formats provided by the tokens. Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com> --- src/event-parse.c | 87 ++++++++++++++++++++++++++++++++++++++++++----- src/event-parse.h | 3 ++ 2 files changed, 81 insertions(+), 9 deletions(-)