Re: [PATCH 4/4] libtracefs: Propagate the return value of the callback function

["Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx>]

On 9.06.21 г. 20:00, Steven Rostedt wrote:
> [ Sending again, but this time with "Reply-all" to include the mailing list ]
>
> On Tue,  8 Jun 2021 16:55:03 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> wrote:
>
> > Currently the return value of the callback function can be used to
> > stop pulling data from the trace buffer, however this return value
> > is lost and the user has no idea if tracefs_iterate_raw_events()
> > terminated because there was no more data or because this was
> > requested from the callback function. If we propagate the return
> > value of the callback, this can be used in cases like the one below:
> >
> > int callback(struct tep_event *event, struct tep_record *record,
> > 	     int cpu, void *py_func)
> > {
> > 	....
> >
> > 	return (something) ? 0 : 1
> > }
> >
> > int main()
> > {
> > 	int ret;
> >
> > 	....
> >
> > 	while(ret == 0)
> > 		ret = tracefs_iterate_raw_events(tep, instance,
> > 		                                 NULL, 0,
> > 		                                 callback, NULL);
> >
> > 	....
> >
> > Here the user can effectively terminate the pulling the data
> > from inside the callback.
>
> With this change, the user can't tell if returned -1 due to an error or
> because the callback ended early, (which is not considered an error).
>
> The proper way for an application to handle this, is to pass in a
> context structure, and have the callback set a value if it exits early
> or not. I've done this already for needing the same information.
>
> No change to libtracefs is needed. This functionality is already
> available with the current design:
>
> struct my_struct {
> 	bool	stopped;
> };
>
> int callback(struct tep_event *event, struct tep_record *record,
> 		    int cpu, void *data)
> {
> 	struct my_struct *my_data = data;
>
> 	[..]
> 	if (condition) {
> 		my_data->stopped = true;
> 		return 1;
> 	}
> 	return 0;
> }
>
> int main()
> {
> 	struct my_struct my_data = { .stopped = false };
> 	int ret = 0;
>
> 	while (!ret && !my_data.stopped)
> 		ret = tracefs_iterate_raw_events(tep, instance, NULL, 0,
> 					callback, &my_data);
> }
>
> Now, if you are using this within python, and you want the python
> wrapper to pass data as well, you just need to add that to the struct:
>
> struct my_struct {
> 	bool		stopped;
> 	func		*python_callback;
> 	void		*python_data;
> }
>
> int callback(struct tep_event *event, struct tep_record *record,
> 		    int cpu, void *data)
> {
> 	struct my_struct *my_data = data;
> 	int ret;
>
> 	[..]
> 	ret = my_data->python_callback(event, record, cpu, data->python_data);
>
> 	if (ret) {
> 		my_data->stopped = true;
> 		return 1;
> 	}
> 	return 0;
> }
>
> int python_iterator(pthyon_callback, python_data)
> {
> 	struct my_struct my_data = { .stopped = false };
> 	int ret = 0;
>
> 	my_data.python_data = python_data;
> 	my_data.python_callback = python_callback;
>
> 	while (!ret && !my_data.stopped)
> 		ret = tracefs_iterate_raw_events(tep, instance, NULL, 0,
> 					callback, &my_data);
> }
>
> So this patch isn't needed.

I agree. Your solution is better.

Thanks!
Yordan

> -- Steve