Re: [PATCH v6 5/5] libtraceevent: Add tep_print_selected_fields()

[Yordan Karadzhov <y.karadz@gmail.com>]

On 19.08.21 г. 19:55, Steven Rostedt wrote:
> On Thu, 12 Aug 2021 11:59:29 +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.
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   src/event-parse.c | 26 ++++++++++++++++++++++----
>>   src/event-parse.h |  3 +++
>>   2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/event-parse.c b/src/event-parse.c
>> index 0795135..645506e 100644
>> --- a/src/event-parse.c
>> +++ b/src/event-parse.c
>> @@ -5455,20 +5455,38 @@ void tep_print_field(struct trace_seq *s, void *data,
>>   	_tep_print_field(s, data, field, NULL);
>>   }
>>   
>> -void tep_print_fields(struct trace_seq *s, void *data,
>> -		      int size __maybe_unused, struct tep_event *event)
>> +static inline void
>> +print_selected_fields(struct trace_seq *s, void *data,
>> +		      struct tep_event *event,
>> +		      unsigned long long ignore_mask)
>>   {
>>   	struct tep_print_parse *parse = event->print_fmt.print_cache;
>>   	struct tep_format_field *field;
>> +	unsigned long long field_mask = 1;
>>   
>>   	field = event->format.fields;
>> -	while (field) {
>> +	for (;field; field = field->next, field_mask *= 2) {
> 
> The above should be:
> 
> 	for(; field; field = field->next, field_mask <<= 1) {
> 
> As a shift should be done with a shift operator and not a
> multiplication.
> 
> Other than that, the rest looks good. I'll go ahead and pull in patches
> 1 and 2.
> 
> -- Steve
> 
> 
>> +		if (field_mask & ignore_mask)
>> +			continue;
>> +
>>   		trace_seq_printf(s, " %s=", field->name);
>>   		_tep_print_field(s, data, field, &parse);
>> -		field = field->next;
>>   	}
>>   }
>>   
>> +void tep_print_selected_fields(struct trace_seq *s, void *data,
> 
> As the above is an API, it needs a kernel doc type comment, and also an
> addition to the man pages. The man page may be a separate patch.
> 

I was thinking of maybe changing the second argument of the function to

void tep_print_selected_fields(struct trace_seq *s,
			       struct tep_record *record,
			       struct tep_event *event,
			       unsigned long long ignore_mask)


> -- Steve
> 
> 
>> +			       struct tep_event *event,
>> +			       unsigned long long ignore_mask)
>> +{
>> +	print_selected_fields(s, data, event, ignore_mask);

respectively here we will have

	print_selected_fields(s, record->data, event, ignore_mask);
>> +}

This way the call will look cleaner.

	tep_print_selected_fields(s, record, event, mask);

instead of

	tep_print_selected_fields(s, record->data, event, mask);

But on the other hand, this will make the new API inconsistent with the existing
"tep_print_fields()" API bellow.

What do you think?

Thanks!
Yordan


>> +
>> +void tep_print_fields(struct trace_seq *s, void *data,
>> +		      int size __maybe_unused, struct tep_event *event)
>> +{
>> +	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..e3638cf 100644
>> --- a/src/event-parse.h
>> +++ b/src/event-parse.h
>> @@ -545,6 +545,9 @@ int tep_cmdline_pid(struct tep_handle *tep, struct tep_cmdline *cmdline);
>>   
>>   void tep_print_field(struct trace_seq *s, void *data,
>>   		     struct tep_format_field *field);
>> +void tep_print_selected_fields(struct trace_seq *s, void *data,
>> +			       struct tep_event *event,
>> +			       unsigned long long ignore_mask);
>>   void tep_print_fields(struct trace_seq *s, void *data,
>>   		      int size __maybe_unused, struct tep_event *event);
>>   int tep_strerror(struct tep_handle *tep, enum tep_errno errnum,
>