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

From: Steven Rostedt <rostedt@goodmis.org>
To: Yordan Karadzhov <y.karadz@gmail.com>
Cc: linux-trace-devel@vger.kernel.org
Subject: Re: [PATCH v6 5/5] libtraceevent: Add tep_print_selected_fields()
Date: Fri, 20 Aug 2021 10:35:10 -0400	[thread overview]
Message-ID: <20210820103510.6125dda1@oasis.local.home> (raw)
In-Reply-To: <a15c7f5f-aa16-7e34-f2fe-bf05a13069e5@gmail.com>

On Fri, 20 Aug 2021 16:36:12 +0300
Yordan Karadzhov <y.karadz@gmail.com> wrote:
		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.

I think we rushed the libtraceevent APIs :-(

There's a lot of them I hate, and I agree, passing record would have made more sense.

I've been thinking of reworking a lot of them, but we need to add new APIs.

tep_print_record_fields()
tep_print_record_selected_fields()

??

-- Steve