linux-trace-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] libtracefs: Fix enable_disable_all() return value
@ 2021-06-08 13:55 Yordan Karadzhov (VMware)
  2021-06-08 13:55 ` [PATCH 2/4] libtracefs: Fix event_enable_disable() " Yordan Karadzhov (VMware)
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Yordan Karadzhov (VMware) @ 2021-06-08 13:55 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov (VMware)

enable_disable_all() is a static method used internally by the
tracefs_event_enable/disable() API. In some cases its return
value gets propagated as a return of the API function but this
value do not obey the description given in the documentation.

Fixes: fc94d1a (libtracefs: Add tracefs_event_enable/disable() API)
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/tracefs-events.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/tracefs-events.c b/src/tracefs-events.c
index 55316c6..8b2d9ef 100644
--- a/src/tracefs-events.c
+++ b/src/tracefs-events.c
@@ -822,8 +822,10 @@ static int enable_disable_all(struct tracefs_instance *instance,
 			      bool enable)
 {
 	const char *str = enable ? "1" : "0";
+	int ret;
 
-	return tracefs_instance_file_write(instance, "events/enable", str);
+	ret = tracefs_instance_file_write(instance, "events/enable", str);
+	return ret < 0 ? ret : 0;
 }
 
 static int event_enable_disable(struct tracefs_instance *instance,
-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/4] libtracefs: Fix event_enable_disable() return value
  2021-06-08 13:55 [PATCH 1/4] libtracefs: Fix enable_disable_all() return value Yordan Karadzhov (VMware)
@ 2021-06-08 13:55 ` Yordan Karadzhov (VMware)
  2021-06-08 13:55 ` [PATCH 3/4] libtracefs: Fix typo in function name Yordan Karadzhov (VMware)
  2021-06-08 13:55 ` [PATCH 4/4] libtracefs: Propagate the return value of the callback function Yordan Karadzhov (VMware)
  2 siblings, 0 replies; 6+ messages in thread
From: Yordan Karadzhov (VMware) @ 2021-06-08 13:55 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov (VMware)

event_enable_disable() is a static method used internally by the
tracefs_event_enable/disable() API. The returned value do not obey
the description given in the documentation. Note that "ret" must
be set to -1 right before the beginning of the loop.

Fixes: fc94d1a (libtracefs: Add tracefs_event_enable/disable() API)
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/tracefs-events.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/tracefs-events.c b/src/tracefs-events.c
index 8b2d9ef..0fef64f 100644
--- a/src/tracefs-events.c
+++ b/src/tracefs-events.c
@@ -835,7 +835,7 @@ static int event_enable_disable(struct tracefs_instance *instance,
 	regex_t system_re, event_re;
 	char **systems;
 	char **events = NULL;
-	int ret = -1;
+	int ret;
 	int s, e;
 
 	/* Handle all events first */
@@ -860,6 +860,7 @@ static int event_enable_disable(struct tracefs_instance *instance,
 		}
 	}
 
+	ret = -1;
 	for (s = 0; systems[s]; s++) {
 		if (system && !match(systems[s], &system_re))
 			continue;
-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/4] libtracefs: Fix typo in function name
  2021-06-08 13:55 [PATCH 1/4] libtracefs: Fix enable_disable_all() return value Yordan Karadzhov (VMware)
  2021-06-08 13:55 ` [PATCH 2/4] libtracefs: Fix event_enable_disable() " Yordan Karadzhov (VMware)
@ 2021-06-08 13:55 ` Yordan Karadzhov (VMware)
  2021-06-08 13:55 ` [PATCH 4/4] libtracefs: Propagate the return value of the callback function Yordan Karadzhov (VMware)
  2 siblings, 0 replies; 6+ messages in thread
From: Yordan Karadzhov (VMware) @ 2021-06-08 13:55 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov (VMware)

Fixes: 5c013e7 (libtracefs: New APIs for trace options)
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 include/tracefs.h   | 2 +-
 src/tracefs-tools.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/tracefs.h b/include/tracefs.h
index 08dda13..e29b550 100644
--- a/include/tracefs.h
+++ b/include/tracefs.h
@@ -159,7 +159,7 @@ bool tracefs_option_is_supported(struct tracefs_instance *instance, enum tracefs
 const struct tracefs_options_mask *tracefs_options_get_enabled(struct tracefs_instance *instance);
 bool tracefs_option_is_enabled(struct tracefs_instance *instance, enum tracefs_option_id id);
 int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option_id id);
-int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id);
+int tracefs_option_disable(struct tracefs_instance *instance, enum tracefs_option_id id);
 const char *tracefs_option_name(enum tracefs_option_id id);
 enum tracefs_option_id tracefs_option_id(const char *name);
 
diff --git a/src/tracefs-tools.c b/src/tracefs-tools.c
index 993fb3c..ca40d4d 100644
--- a/src/tracefs-tools.c
+++ b/src/tracefs-tools.c
@@ -307,13 +307,13 @@ int tracefs_option_enable(struct tracefs_instance *instance, enum tracefs_option
 }
 
 /**
- * tracefs_option_diasble - Disable trace option
+ * tracefs_option_disable - Disable trace option
  * @instance: ftrace instance, can be NULL for the top instance
  * @id: trace option id
  *
  * Returns -1 in case of an error or 0 otherwise
  */
-int tracefs_option_diasble(struct tracefs_instance *instance, enum tracefs_option_id id)
+int tracefs_option_disable(struct tracefs_instance *instance, enum tracefs_option_id id)
 {
 	return trace_config_option(instance, id, false);
 }
-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 4/4] libtracefs: Propagate the return value of the callback function
  2021-06-08 13:55 [PATCH 1/4] libtracefs: Fix enable_disable_all() return value Yordan Karadzhov (VMware)
  2021-06-08 13:55 ` [PATCH 2/4] libtracefs: Fix event_enable_disable() " Yordan Karadzhov (VMware)
  2021-06-08 13:55 ` [PATCH 3/4] libtracefs: Fix typo in function name Yordan Karadzhov (VMware)
@ 2021-06-08 13:55 ` Yordan Karadzhov (VMware)
  2021-06-09 17:00   ` Steven Rostedt
  2 siblings, 1 reply; 6+ messages in thread
From: Yordan Karadzhov (VMware) @ 2021-06-08 13:55 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov (VMware)

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.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/tracefs-events.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/tracefs-events.c b/src/tracefs-events.c
index 0fef64f..0a87d28 100644
--- a/src/tracefs-events.c
+++ b/src/tracefs-events.c
@@ -131,7 +131,8 @@ static int read_cpu_pages(struct tep_handle *tep, struct cpu_iterate *cpus, int
 		}
 		if (j < count) {
 			if (callback(cpus[j].event, &cpus[j].record, cpus[j].cpu, callback_context))
-				break;
+				return -1;
+
 			cpus[j].event = NULL;
 			read_next_record(tep, cpus + j);
 		} else {
-- 
2.27.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 4/4] libtracefs: Propagate the return value of the callback function
  2021-06-08 13:55 ` [PATCH 4/4] libtracefs: Propagate the return value of the callback function Yordan Karadzhov (VMware)
@ 2021-06-09 17:00   ` Steven Rostedt
  2021-06-11 11:03     ` Yordan Karadzhov (VMware)
  0 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2021-06-09 17:00 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware); +Cc: linux-trace-devel

[ 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.

-- Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 4/4] libtracefs: Propagate the return value of the callback function
  2021-06-09 17:00   ` Steven Rostedt
@ 2021-06-11 11:03     ` Yordan Karadzhov (VMware)
  0 siblings, 0 replies; 6+ messages in thread
From: Yordan Karadzhov (VMware) @ 2021-06-11 11:03 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel



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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-06-11 11:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-08 13:55 [PATCH 1/4] libtracefs: Fix enable_disable_all() return value Yordan Karadzhov (VMware)
2021-06-08 13:55 ` [PATCH 2/4] libtracefs: Fix event_enable_disable() " Yordan Karadzhov (VMware)
2021-06-08 13:55 ` [PATCH 3/4] libtracefs: Fix typo in function name Yordan Karadzhov (VMware)
2021-06-08 13:55 ` [PATCH 4/4] libtracefs: Propagate the return value of the callback function Yordan Karadzhov (VMware)
2021-06-09 17:00   ` Steven Rostedt
2021-06-11 11:03     ` Yordan Karadzhov (VMware)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).