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

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

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@gmail.com> 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
>