[PATCH v2 00/18] rtla: Code quality and robustness improvements

[PATCH v2 00/18] rtla: Code quality and robustness improvements

[Wander Lairson Costa]

This patch series addresses several code quality and robustness issues
in the rtla (Real-Time Linux Analysis) tool. The changes focus on
eliminating potential buffer overflows, fixing NULL pointer dereferences,
improving error handling, and simplifying code maintenance through better
abstractions and helper functions.

The series introduces safer string handling practices, including proper
null termination after read() operations, correct buffer sizing for
strncpy(), and volatile qualification for signal handler variables. It
replaces unsafe functions like atoi() with robust error-checking
alternatives, eliminates magic numbers in favor of named constants, and
adds compile-time string length calculations to prevent buffer overruns.

Additionally, the series reduces code duplication by introducing helper
macros and functions for common patterns like action iteration, argument
parsing, and threshold restart logic. It also includes minor cleanups
such as removing redundant operations, unused headers, and fixing
documentation inconsistencies. These improvements make the rtla codebase
safer, more maintainable, and more consistent with kernel coding
standards.

Changes:

v2:
- exit on memory allocation failure
- remove redundant strlen() calls
- fix possible race on condition on stop_tracing variable access
- ensure null termination on read() calls
- fix checkpatch reports
- make extract_args() an inline function
- add the usage of common_restart() in more places

Wander Lairson Costa (18):
  rtla: Exit on memory allocation failures during initialization
  rtla: Use strdup() to simplify code
  rtla: Introduce for_each_action() helper
  rtla: Replace atoi() with a robust strtoi()
  rtla: Simplify argument parsing
  rtla: Use strncmp_static() in more places
  rtla: Introduce common_restart() helper
  rtla: Use standard exit codes for result enum
  rtla: Remove redundant memset after calloc
  rtla: Replace magic number with MAX_PATH
  rtla: Remove unused headers
  rtla: Fix NULL pointer dereference in actions_parse
  rtla: Fix buffer size for strncpy in timerlat_aa
  rtla: Add generated output files to gitignore
  rtla: Make stop_tracing variable volatile
  rtla: Ensure null termination after read operations in utils.c
  rtla: Fix parse_cpu_set() return value documentation
  rtla: Simplify code by caching string lengths

 tools/tracing/rtla/.gitignore          |   4 +
 tools/tracing/rtla/src/actions.c       | 114 +++++++++++++++----------
 tools/tracing/rtla/src/actions.h       |  13 ++-
 tools/tracing/rtla/src/common.c        |  67 ++++++++++-----
 tools/tracing/rtla/src/common.h        |  11 ++-
 tools/tracing/rtla/src/osnoise.c       |  28 ++----
 tools/tracing/rtla/src/osnoise_hist.c  |  26 ++----
 tools/tracing/rtla/src/osnoise_top.c   |  25 ++----
 tools/tracing/rtla/src/timerlat.c      |   5 +-
 tools/tracing/rtla/src/timerlat_aa.c   |   4 +-
 tools/tracing/rtla/src/timerlat_hist.c |  44 ++++------
 tools/tracing/rtla/src/timerlat_top.c  |  46 ++++------
 tools/tracing/rtla/src/timerlat_u.c    |   4 +-
 tools/tracing/rtla/src/trace.c         |  59 +++++--------
 tools/tracing/rtla/src/trace.h         |   4 +-
 tools/tracing/rtla/src/utils.c         |  99 ++++++++++++++++++---
 tools/tracing/rtla/src/utils.h         |  26 ++++--
 17 files changed, 335 insertions(+), 244 deletions(-)

-- 
2.52.0

[PATCH v2 01/18] rtla: Exit on memory allocation failures during initialization

[Wander Lairson Costa]

Most memory allocations in rtla happen during early initialization
before any resources are acquired that would require cleanup. In these
cases, propagating allocation errors just adds complexity without any
benefit. There's nothing to clean up, and the program must exit anyway.

This patch introduces fatal allocation wrappers (calloc_fatal,
reallocarray_fatal, strdup_fatal) that call fatal() on allocation
failure. These wrappers simplify the code by eliminating unnecessary
error propagation paths.

The patch converts early allocations to use these wrappers in
actions_init() and related action functions, osnoise_context_alloc()
and osnoise_init_tool(), trace_instance_init() and trace event
functions, and parameter structure allocations in main functions.

This simplifies the code while maintaining the same behavior: immediate
exit on allocation failure during initialization. Allocations that
require cleanup, such as those in histogram allocation functions with
goto cleanup paths, are left unchanged and continue to return errors.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/actions.c       | 50 ++++++++++++--------------
 tools/tracing/rtla/src/actions.h       |  8 ++---
 tools/tracing/rtla/src/osnoise.c       | 22 ++++--------
 tools/tracing/rtla/src/osnoise_hist.c  | 25 ++++---------
 tools/tracing/rtla/src/osnoise_top.c   | 25 ++++---------
 tools/tracing/rtla/src/timerlat_hist.c | 24 ++++---------
 tools/tracing/rtla/src/timerlat_top.c  | 25 ++++---------
 tools/tracing/rtla/src/trace.c         | 30 ++++------------
 tools/tracing/rtla/src/trace.h         |  4 +--
 tools/tracing/rtla/src/utils.c         | 35 ++++++++++++++++++
 tools/tracing/rtla/src/utils.h         |  3 ++
 11 files changed, 108 insertions(+), 143 deletions(-)

diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
index 8945aee58d511..ff7811e175930 100644
--- a/tools/tracing/rtla/src/actions.c
+++ b/tools/tracing/rtla/src/actions.c
@@ -15,7 +15,7 @@ void
 actions_init(struct actions *self)
 {
 	self->size = action_default_size;
-	self->list = calloc(self->size, sizeof(struct action));
+	self->list = calloc_fatal(self->size, sizeof(struct action));
 	self->len = 0;
 	self->continue_flag = false;
 
@@ -50,8 +50,10 @@ static struct action *
 actions_new(struct actions *self)
 {
 	if (self->len >= self->size) {
-		self->size *= 2;
-		self->list = realloc(self->list, self->size * sizeof(struct action));
+		const size_t new_size = self->size * 2;
+
+		self->list = reallocarray_fatal(self->list, new_size, sizeof(struct action));
+		self->size = new_size;
 	}
 
 	return &self->list[self->len++];
@@ -60,25 +62,21 @@ actions_new(struct actions *self)
 /*
  * actions_add_trace_output - add an action to output trace
  */
-int
+void
 actions_add_trace_output(struct actions *self, const char *trace_output)
 {
 	struct action *action = actions_new(self);
 
 	self->present[ACTION_TRACE_OUTPUT] = true;
 	action->type = ACTION_TRACE_OUTPUT;
-	action->trace_output = calloc(strlen(trace_output) + 1, sizeof(char));
-	if (!action->trace_output)
-		return -1;
+	action->trace_output = calloc_fatal(strlen(trace_output) + 1, sizeof(char));
 	strcpy(action->trace_output, trace_output);
-
-	return 0;
 }
 
 /*
  * actions_add_trace_output - add an action to send signal to a process
  */
-int
+void
 actions_add_signal(struct actions *self, int signal, int pid)
 {
 	struct action *action = actions_new(self);
@@ -87,40 +85,32 @@ actions_add_signal(struct actions *self, int signal, int pid)
 	action->type = ACTION_SIGNAL;
 	action->signal = signal;
 	action->pid = pid;
-
-	return 0;
 }
 
 /*
  * actions_add_shell - add an action to execute a shell command
  */
-int
+void
 actions_add_shell(struct actions *self, const char *command)
 {
 	struct action *action = actions_new(self);
 
 	self->present[ACTION_SHELL] = true;
 	action->type = ACTION_SHELL;
-	action->command = calloc(strlen(command) + 1, sizeof(char));
-	if (!action->command)
-		return -1;
+	action->command = calloc_fatal(strlen(command) + 1, sizeof(char));
 	strcpy(action->command, command);
-
-	return 0;
 }
 
 /*
  * actions_add_continue - add an action to resume measurement
  */
-int
+void
 actions_add_continue(struct actions *self)
 {
 	struct action *action = actions_new(self);
 
 	self->present[ACTION_CONTINUE] = true;
 	action->type = ACTION_CONTINUE;
-
-	return 0;
 }
 
 /*
@@ -174,7 +164,8 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
 				/* Only one argument allowed */
 				return -1;
 		}
-		return actions_add_trace_output(self, trace_output);
+		actions_add_trace_output(self, trace_output);
+		break;
 	case ACTION_SIGNAL:
 		/* Takes two arguments, num (signal) and pid */
 		while (token != NULL) {
@@ -197,21 +188,26 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
 			/* Missing argument */
 			return -1;
 
-		return actions_add_signal(self, signal, pid);
+		actions_add_signal(self, signal, pid);
+		break;
 	case ACTION_SHELL:
 		if (token == NULL)
 			return -1;
-		if (strlen(token) > 8 && strncmp(token, "command=", 8) == 0)
-			return actions_add_shell(self, token + 8);
-		return -1;
+		if (strlen(token) > 8 && strncmp(token, "command=", 8))
+			return -1;
+		actions_add_shell(self, token + 8);
+		break;
 	case ACTION_CONTINUE:
 		/* Takes no argument */
 		if (token != NULL)
 			return -1;
-		return actions_add_continue(self);
+		actions_add_continue(self);
+		break;
 	default:
 		return -1;
 	}
+
+	return 0;
 }
 
 /*
diff --git a/tools/tracing/rtla/src/actions.h b/tools/tracing/rtla/src/actions.h
index a4f9b570775b5..fb366d6509b8b 100644
--- a/tools/tracing/rtla/src/actions.h
+++ b/tools/tracing/rtla/src/actions.h
@@ -44,9 +44,9 @@ struct actions {
 
 void actions_init(struct actions *self);
 void actions_destroy(struct actions *self);
-int actions_add_trace_output(struct actions *self, const char *trace_output);
-int actions_add_signal(struct actions *self, int signal, int pid);
-int actions_add_shell(struct actions *self, const char *command);
-int actions_add_continue(struct actions *self);
+void actions_add_trace_output(struct actions *self, const char *trace_output);
+void actions_add_signal(struct actions *self, int signal, int pid);
+void actions_add_shell(struct actions *self, const char *command);
+void actions_add_continue(struct actions *self);
 int actions_parse(struct actions *self, const char *trigger, const char *tracefn);
 int actions_perform(struct actions *self);
diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c
index 312c511fa0044..c5b41ec26b0a4 100644
--- a/tools/tracing/rtla/src/osnoise.c
+++ b/tools/tracing/rtla/src/osnoise.c
@@ -938,9 +938,7 @@ struct osnoise_context *osnoise_context_alloc(void)
 {
 	struct osnoise_context *context;
 
-	context = calloc(1, sizeof(*context));
-	if (!context)
-		return NULL;
+	context = calloc_fatal(1, sizeof(*context));
 
 	context->orig_stop_us		= OSNOISE_OPTION_INIT_VAL;
 	context->stop_us		= OSNOISE_OPTION_INIT_VAL;
@@ -1017,24 +1015,16 @@ void osnoise_destroy_tool(struct osnoise_tool *top)
 struct osnoise_tool *osnoise_init_tool(char *tool_name)
 {
 	struct osnoise_tool *top;
-	int retval;
-
-	top = calloc(1, sizeof(*top));
-	if (!top)
-		return NULL;
 
+	top = calloc_fatal(1, sizeof(*top));
 	top->context = osnoise_context_alloc();
-	if (!top->context)
-		goto out_err;
 
-	retval = trace_instance_init(&top->trace, tool_name);
-	if (retval)
-		goto out_err;
+	if (trace_instance_init(&top->trace, tool_name)) {
+		osnoise_destroy_tool(top);
+		return NULL;
+	}
 
 	return top;
-out_err:
-	osnoise_destroy_tool(top);
-	return NULL;
 }
 
 /*
diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c
index ff8c231e47c47..af7d8621dd9d7 100644
--- a/tools/tracing/rtla/src/osnoise_hist.c
+++ b/tools/tracing/rtla/src/osnoise_hist.c
@@ -474,9 +474,7 @@ static struct common_params
 	int c;
 	char *trace_output = NULL;
 
-	params = calloc(1, sizeof(*params));
-	if (!params)
-		exit(1);
+	params = calloc_fatal(1, sizeof(*params));
 
 	actions_init(&params->common.threshold_actions);
 	actions_init(&params->common.end_actions);
@@ -564,9 +562,6 @@ static struct common_params
 			break;
 		case 'e':
 			tevent = trace_event_alloc(optarg);
-			if (!tevent)
-				fatal("Error alloc trace event");
-
 			if (params->common.events)
 				tevent->next = params->common.events;
 
@@ -631,22 +626,16 @@ static struct common_params
 			params->common.hist.with_zeros = 1;
 			break;
 		case '4': /* trigger */
-			if (params->common.events) {
-				retval = trace_event_add_trigger(params->common.events, optarg);
-				if (retval)
-					fatal("Error adding trigger %s", optarg);
-			} else {
+			if (params->common.events)
+				trace_event_add_trigger(params->common.events, optarg);
+			else
 				fatal("--trigger requires a previous -e");
-			}
 			break;
 		case '5': /* filter */
-			if (params->common.events) {
-				retval = trace_event_add_filter(params->common.events, optarg);
-				if (retval)
-					fatal("Error adding filter %s", optarg);
-			} else {
+			if (params->common.events)
+				trace_event_add_filter(params->common.events, optarg);
+			else
 				fatal("--filter requires a previous -e");
-			}
 			break;
 		case '6':
 			params->common.warmup = get_llong_from_str(optarg);
diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c
index 04c699bdd7363..31b7e92d76fe4 100644
--- a/tools/tracing/rtla/src/osnoise_top.c
+++ b/tools/tracing/rtla/src/osnoise_top.c
@@ -327,9 +327,7 @@ struct common_params *osnoise_top_parse_args(int argc, char **argv)
 	int c;
 	char *trace_output = NULL;
 
-	params = calloc(1, sizeof(*params));
-	if (!params)
-		exit(1);
+	params = calloc_fatal(1, sizeof(*params));
 
 	actions_init(&params->common.threshold_actions);
 	actions_init(&params->common.end_actions);
@@ -410,9 +408,6 @@ struct common_params *osnoise_top_parse_args(int argc, char **argv)
 			break;
 		case 'e':
 			tevent = trace_event_alloc(optarg);
-			if (!tevent)
-				fatal("Error alloc trace event");
-
 			if (params->common.events)
 				tevent->next = params->common.events;
 			params->common.events = tevent;
@@ -462,22 +457,16 @@ struct common_params *osnoise_top_parse_args(int argc, char **argv)
 			params->threshold = get_llong_from_str(optarg);
 			break;
 		case '0': /* trigger */
-			if (params->common.events) {
-				retval = trace_event_add_trigger(params->common.events, optarg);
-				if (retval)
-					fatal("Error adding trigger %s", optarg);
-			} else {
+			if (params->common.events)
+				trace_event_add_trigger(params->common.events, optarg);
+			else
 				fatal("--trigger requires a previous -e");
-			}
 			break;
 		case '1': /* filter */
-			if (params->common.events) {
-				retval = trace_event_add_filter(params->common.events, optarg);
-				if (retval)
-					fatal("Error adding filter %s", optarg);
-			} else {
+			if (params->common.events)
+				trace_event_add_filter(params->common.events, optarg);
+			else
 				fatal("--filter requires a previous -e");
-			}
 			break;
 		case '2':
 			params->common.warmup = get_llong_from_str(optarg);
diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
index 1fb471a787b79..226167c88c8d6 100644
--- a/tools/tracing/rtla/src/timerlat_hist.c
+++ b/tools/tracing/rtla/src/timerlat_hist.c
@@ -772,9 +772,7 @@ static struct common_params
 	int c;
 	char *trace_output = NULL;
 
-	params = calloc(1, sizeof(*params));
-	if (!params)
-		exit(1);
+	params = calloc_fatal(1, sizeof(*params));
 
 	actions_init(&params->common.threshold_actions);
 	actions_init(&params->common.end_actions);
@@ -883,8 +881,6 @@ static struct common_params
 			break;
 		case 'e':
 			tevent = trace_event_alloc(optarg);
-			if (!tevent)
-				fatal("Error alloc trace event");
 
 			if (params->common.events)
 				tevent->next = params->common.events;
@@ -963,22 +959,16 @@ static struct common_params
 			params->common.hist.with_zeros = 1;
 			break;
 		case '6': /* trigger */
-			if (params->common.events) {
-				retval = trace_event_add_trigger(params->common.events, optarg);
-				if (retval)
-					fatal("Error adding trigger %s", optarg);
-			} else {
+			if (params->common.events)
+				trace_event_add_trigger(params->common.events, optarg);
+			else
 				fatal("--trigger requires a previous -e");
-			}
 			break;
 		case '7': /* filter */
-			if (params->common.events) {
-				retval = trace_event_add_filter(params->common.events, optarg);
-				if (retval)
-					fatal("Error adding filter %s", optarg);
-			} else {
+			if (params->common.events)
+				trace_event_add_filter(params->common.events, optarg);
+			else
 				fatal("--filter requires a previous -e");
-			}
 			break;
 		case '8':
 			params->dma_latency = get_llong_from_str(optarg);
diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c
index 29c2c1f717ed7..31e1514a2528d 100644
--- a/tools/tracing/rtla/src/timerlat_top.c
+++ b/tools/tracing/rtla/src/timerlat_top.c
@@ -544,9 +544,7 @@ static struct common_params
 	int c;
 	char *trace_output = NULL;
 
-	params = calloc(1, sizeof(*params));
-	if (!params)
-		exit(1);
+	params = calloc_fatal(1, sizeof(*params));
 
 	actions_init(&params->common.threshold_actions);
 	actions_init(&params->common.end_actions);
@@ -655,9 +653,6 @@ static struct common_params
 			break;
 		case 'e':
 			tevent = trace_event_alloc(optarg);
-			if (!tevent)
-				fatal("Error alloc trace event");
-
 			if (params->common.events)
 				tevent->next = params->common.events;
 			params->common.events = tevent;
@@ -713,22 +708,16 @@ static struct common_params
 			params->common.user_data = true;
 			break;
 		case '0': /* trigger */
-			if (params->common.events) {
-				retval = trace_event_add_trigger(params->common.events, optarg);
-				if (retval)
-					fatal("Error adding trigger %s", optarg);
-			} else {
+			if (params->common.events)
+				trace_event_add_trigger(params->common.events, optarg);
+			else
 				fatal("--trigger requires a previous -e");
-			}
 			break;
 		case '1': /* filter */
-			if (params->common.events) {
-				retval = trace_event_add_filter(params->common.events, optarg);
-				if (retval)
-					fatal("Error adding filter %s", optarg);
-			} else {
+			if (params->common.events)
+				trace_event_add_filter(params->common.events, optarg);
+			else
 				fatal("--filter requires a previous -e");
-			}
 			break;
 		case '2': /* dma-latency */
 			params->dma_latency = get_llong_from_str(optarg);
diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
index 69cbc48d53d3a..b22bb844b71f3 100644
--- a/tools/tracing/rtla/src/trace.c
+++ b/tools/tracing/rtla/src/trace.c
@@ -192,9 +192,7 @@ void trace_instance_destroy(struct trace_instance *trace)
  */
 int trace_instance_init(struct trace_instance *trace, char *tool_name)
 {
-	trace->seq = calloc(1, sizeof(*trace->seq));
-	if (!trace->seq)
-		goto out_err;
+	trace->seq = calloc_fatal(1, sizeof(*trace->seq));
 
 	trace_seq_init(trace->seq);
 
@@ -275,15 +273,9 @@ struct trace_events *trace_event_alloc(const char *event_string)
 {
 	struct trace_events *tevent;
 
-	tevent = calloc(1, sizeof(*tevent));
-	if (!tevent)
-		return NULL;
+	tevent = calloc_fatal(1, sizeof(*tevent));
 
-	tevent->system = strdup(event_string);
-	if (!tevent->system) {
-		free(tevent);
-		return NULL;
-	}
+	tevent->system = strdup_fatal(event_string);
 
 	tevent->event = strstr(tevent->system, ":");
 	if (tevent->event) {
@@ -297,31 +289,23 @@ struct trace_events *trace_event_alloc(const char *event_string)
 /*
  * trace_event_add_filter - record an event filter
  */
-int trace_event_add_filter(struct trace_events *event, char *filter)
+void trace_event_add_filter(struct trace_events *event, char *filter)
 {
 	if (event->filter)
 		free(event->filter);
 
-	event->filter = strdup(filter);
-	if (!event->filter)
-		return 1;
-
-	return 0;
+	event->filter = strdup_fatal(filter);
 }
 
 /*
  * trace_event_add_trigger - record an event trigger action
  */
-int trace_event_add_trigger(struct trace_events *event, char *trigger)
+void trace_event_add_trigger(struct trace_events *event, char *trigger)
 {
 	if (event->trigger)
 		free(event->trigger);
 
-	event->trigger = strdup(trigger);
-	if (!event->trigger)
-		return 1;
-
-	return 0;
+	event->trigger = strdup_fatal(trigger);
 }
 
 /*
diff --git a/tools/tracing/rtla/src/trace.h b/tools/tracing/rtla/src/trace.h
index 1e5aee4b828dd..95b911a2228b2 100644
--- a/tools/tracing/rtla/src/trace.h
+++ b/tools/tracing/rtla/src/trace.h
@@ -45,6 +45,6 @@ void trace_events_destroy(struct trace_instance *instance,
 int trace_events_enable(struct trace_instance *instance,
 			  struct trace_events *events);
 
-int trace_event_add_filter(struct trace_events *event, char *filter);
-int trace_event_add_trigger(struct trace_events *event, char *trigger);
+void trace_event_add_filter(struct trace_events *event, char *filter);
+void trace_event_add_trigger(struct trace_events *event, char *trigger);
 int trace_set_buffer_size(struct trace_instance *trace, int size);
diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c
index 9cf5a0098e9aa..acf95afa25b5a 100644
--- a/tools/tracing/rtla/src/utils.c
+++ b/tools/tracing/rtla/src/utils.c
@@ -1000,3 +1000,38 @@ char *parse_optional_arg(int argc, char **argv)
 		return NULL;
 	}
 }
+
+static inline void fatal_alloc(void)
+{
+	fatal("Error allocating memory\n");
+}
+
+void *calloc_fatal(size_t n, size_t size)
+{
+	void *p = calloc(n, size);
+
+	if (!p)
+		fatal_alloc();
+
+	return p;
+}
+
+void *reallocarray_fatal(void *p, size_t n, size_t size)
+{
+	p = reallocarray(p, n, size);
+
+	if (!p)
+		fatal_alloc();
+
+	return p;
+}
+
+char *strdup_fatal(const char *s)
+{
+	char *p = strdup(s);
+
+	if (!p)
+		fatal_alloc();
+
+	return p;
+}
diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
index 091df4ba45877..0ed2c7275f2c5 100644
--- a/tools/tracing/rtla/src/utils.h
+++ b/tools/tracing/rtla/src/utils.h
@@ -68,6 +68,9 @@ int set_comm_sched_attr(const char *comm_prefix, struct sched_attr *attr);
 int set_comm_cgroup(const char *comm_prefix, const char *cgroup);
 int set_pid_cgroup(pid_t pid, const char *cgroup);
 int set_cpu_dma_latency(int32_t latency);
+void *calloc_fatal(size_t n, size_t size);
+void *reallocarray_fatal(void *p, size_t n, size_t size);
+char *strdup_fatal(const char *s);
 #ifdef HAVE_LIBCPUPOWER_SUPPORT
 int save_cpu_idle_disable_state(unsigned int cpu);
 int restore_cpu_idle_disable_state(unsigned int cpu);
-- 
2.52.0

[PATCH v2 02/18] rtla: Use strdup() to simplify code

[Wander Lairson Costa]

The actions_add_trace_output() and actions_add_shell() functions were
using calloc() followed by strcpy() to allocate and copy a string.
This can be simplified by using strdup(), which allocates memory and
copies the string in a single step.

Replace the calloc() and strcpy() calls with strdup(), making the
code more concise and readable.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/actions.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
index ff7811e175930..090d514fe4126 100644
--- a/tools/tracing/rtla/src/actions.c
+++ b/tools/tracing/rtla/src/actions.c
@@ -69,8 +69,7 @@ actions_add_trace_output(struct actions *self, const char *trace_output)
 
 	self->present[ACTION_TRACE_OUTPUT] = true;
 	action->type = ACTION_TRACE_OUTPUT;
-	action->trace_output = calloc_fatal(strlen(trace_output) + 1, sizeof(char));
-	strcpy(action->trace_output, trace_output);
+	action->trace_output = strdup_fatal(trace_output);
 }
 
 /*
@@ -97,8 +96,7 @@ actions_add_shell(struct actions *self, const char *command)
 
 	self->present[ACTION_SHELL] = true;
 	action->type = ACTION_SHELL;
-	action->command = calloc_fatal(strlen(command) + 1, sizeof(char));
-	strcpy(action->command, command);
+	action->command = strdup_fatal(command);
 }
 
 /*
-- 
2.52.0

[PATCH v2 03/18] rtla: Introduce for_each_action() helper

[Wander Lairson Costa]

The for loop to iterate over the list of actions is used in
more than one place. To avoid code duplication and improve
readability, introduce a for_each_action() helper macro.

Replace the open-coded for loops with the new helper.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/actions.c | 6 ++++--
 tools/tracing/rtla/src/actions.h | 5 +++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
index 090d514fe4126..a4d0dc47e6aa1 100644
--- a/tools/tracing/rtla/src/actions.c
+++ b/tools/tracing/rtla/src/actions.c
@@ -32,7 +32,9 @@ void
 actions_destroy(struct actions *self)
 {
 	/* Free any action-specific data */
-	for (struct action *action = self->list; action < self->list + self->len; action++) {
+	struct action *action;
+
+	for_each_action(self, action) {
 		if (action->type == ACTION_SHELL)
 			free(action->command);
 		if (action->type == ACTION_TRACE_OUTPUT)
@@ -217,7 +219,7 @@ actions_perform(struct actions *self)
 	int pid, retval;
 	const struct action *action;
 
-	for (action = self->list; action < self->list + self->len; action++) {
+	for_each_action(self, action) {
 		switch (action->type) {
 		case ACTION_TRACE_OUTPUT:
 			retval = save_trace_to_file(self->trace_output_inst, action->trace_output);
diff --git a/tools/tracing/rtla/src/actions.h b/tools/tracing/rtla/src/actions.h
index fb366d6509b8b..034048682fefb 100644
--- a/tools/tracing/rtla/src/actions.h
+++ b/tools/tracing/rtla/src/actions.h
@@ -42,6 +42,11 @@ struct actions {
 	struct tracefs_instance *trace_output_inst;
 };
 
+#define for_each_action(actions, action)			\
+	for ((action) = (actions)->list;			\
+	     (action) < (actions)->list + (actions)->len;	\
+	     (action)++)
+
 void actions_init(struct actions *self);
 void actions_destroy(struct actions *self);
 void actions_add_trace_output(struct actions *self, const char *trace_output);
-- 
2.52.0

[PATCH v2 04/18] rtla: Replace atoi() with a robust strtoi()

[Wander Lairson Costa]

The atoi() function does not perform error checking, which can lead to
undefined behavior when parsing invalid or out-of-range strings. This
can cause issues when parsing user-provided numerical inputs, such as
signal numbers, PIDs, or CPU lists.

To address this, introduce a new strtoi() helper function that safely
converts a string to an integer. This function validates the input and
checks for overflows, returning a negative value on  failure.

Replace all calls to atoi() with the new strtoi() function and add
proper error handling to make the parsing more robust and prevent
potential issues.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/actions.c |  7 +++---
 tools/tracing/rtla/src/utils.c   | 40 ++++++++++++++++++++++++++++----
 tools/tracing/rtla/src/utils.h   |  2 ++
 3 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
index a4d0dc47e6aa1..e933c2c68b208 100644
--- a/tools/tracing/rtla/src/actions.c
+++ b/tools/tracing/rtla/src/actions.c
@@ -170,12 +170,13 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
 		/* Takes two arguments, num (signal) and pid */
 		while (token != NULL) {
 			if (strlen(token) > 4 && strncmp(token, "num=", 4) == 0) {
-				signal = atoi(token + 4);
+				if (strtoi(token + 4, &signal))
+					return -1;
 			} else if (strlen(token) > 4 && strncmp(token, "pid=", 4) == 0) {
 				if (strncmp(token + 4, "parent", 7) == 0)
 					pid = -1;
-				else
-					pid = atoi(token + 4);
+				else if (strtoi(token + 4, &pid))
+					return -1;
 			} else {
 				/* Invalid argument */
 				return -1;
diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c
index acf95afa25b5a..f3e129d17a82b 100644
--- a/tools/tracing/rtla/src/utils.c
+++ b/tools/tracing/rtla/src/utils.c
@@ -17,6 +17,7 @@
 #include <fcntl.h>
 #include <sched.h>
 #include <stdio.h>
+#include <limits.h>
 
 #include "utils.h"
 
@@ -127,16 +128,18 @@ int parse_cpu_set(char *cpu_list, cpu_set_t *set)
 	nr_cpus = sysconf(_SC_NPROCESSORS_CONF);
 
 	for (p = cpu_list; *p; ) {
-		cpu = atoi(p);
-		if (cpu < 0 || (!cpu && *p != '0') || cpu >= nr_cpus)
+		if (strtoi(p, &cpu))
+			goto err;
+		if (cpu < 0 || cpu >= nr_cpus)
 			goto err;
 
 		while (isdigit(*p))
 			p++;
 		if (*p == '-') {
 			p++;
-			end_cpu = atoi(p);
-			if (end_cpu < cpu || (!end_cpu && *p != '0') || end_cpu >= nr_cpus)
+			if (strtoi(p, &end_cpu))
+				goto err;
+			if (end_cpu < cpu || end_cpu >= nr_cpus)
 				goto err;
 			while (isdigit(*p))
 				p++;
@@ -337,6 +340,7 @@ int set_comm_sched_attr(const char *comm_prefix, struct sched_attr *attr)
 	struct dirent *proc_entry;
 	DIR *procfs;
 	int retval;
+	int pid;
 
 	if (strlen(comm_prefix) >= MAX_PATH) {
 		err_msg("Command prefix is too long: %d < strlen(%s)\n",
@@ -356,8 +360,12 @@ int set_comm_sched_attr(const char *comm_prefix, struct sched_attr *attr)
 		if (!retval)
 			continue;
 
+		if (strtoi(proc_entry->d_name, &pid)) {
+			err_msg("'%s' is not a valid pid", proc_entry->d_name);
+			goto out_err;
+		}
 		/* procfs_is_workload_pid confirmed it is a pid */
-		retval = __set_sched_attr(atoi(proc_entry->d_name), attr);
+		retval = __set_sched_attr(pid, attr);
 		if (retval) {
 			err_msg("Error setting sched attributes for pid:%s\n", proc_entry->d_name);
 			goto out_err;
@@ -1035,3 +1043,25 @@ char *strdup_fatal(const char *s)
 
 	return p;
 }
+
+/*
+ * strtoi - convert string to integer with error checking
+ *
+ * Returns 0 on success, -1 if conversion fails or result is out of int range.
+ */
+int strtoi(const char *s, int *res)
+{
+	char *end_ptr;
+	long lres;
+
+	if (!*s)
+		return -1;
+
+	errno = 0;
+	lres = strtol(s, &end_ptr, 0);
+	if (errno || *end_ptr || lres > INT_MAX || lres < INT_MIN)
+		return -1;
+
+	*res = (int) lres;
+	return 0;
+}
diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
index 0ed2c7275f2c5..efbf798650306 100644
--- a/tools/tracing/rtla/src/utils.h
+++ b/tools/tracing/rtla/src/utils.h
@@ -3,6 +3,7 @@
 #include <stdint.h>
 #include <time.h>
 #include <sched.h>
+#include <stdbool.h>
 
 /*
  * '18446744073709551615\0'
@@ -85,6 +86,7 @@ static inline int set_deepest_cpu_idle_state(unsigned int cpu, unsigned int stat
 static inline int have_libcpupower_support(void) { return 0; }
 #endif /* HAVE_LIBCPUPOWER_SUPPORT */
 int auto_house_keeping(cpu_set_t *monitored_cpus);
+__attribute__((__warn_unused_result__)) int strtoi(const char *s, int *res);
 
 #define ns_to_usf(x) (((double)x/1000))
 #define ns_to_per(total, part) ((part * 100) / (double)total)
-- 
2.52.0

[PATCH v2 05/18] rtla: Simplify argument parsing

[Wander Lairson Costa]

The actions_parse() function uses open-coded logic to extract arguments
from a string. This includes manual length checks and strncmp() calls,
which can be verbose and error-prone.

To simplify and improve the robustness of argument parsing, introduce a
new extract_arg() helper macro. This macro extracts the value from a
"key=value" pair, making the code more concise and readable.

Also, introduce STRING_LENGTH() and strncmp_static() macros to
perform compile-time calculations of string lengths and safer string
comparisons.

Refactor actions_parse() to use these new helpers, resulting in
cleaner and more maintainable code.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/actions.c | 57 +++++++++++++++++++++++---------
 tools/tracing/rtla/src/utils.h   | 14 ++++++--
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
index e933c2c68b208..b50bf9f9adf28 100644
--- a/tools/tracing/rtla/src/actions.c
+++ b/tools/tracing/rtla/src/actions.c
@@ -113,6 +113,29 @@ actions_add_continue(struct actions *self)
 	action->type = ACTION_CONTINUE;
 }
 
+static inline const char *__extract_arg(const char *token, const char *opt, size_t opt_len)
+{
+	const size_t tok_len = strlen(token);
+
+	if (tok_len <= opt_len)
+		return NULL;
+
+	if (strncmp(token, opt, opt_len))
+		return NULL;
+
+	return token + opt_len;
+}
+
+/*
+ * extract_arg - extract argument value from option token
+ * @token: option token (e.g., "file=trace.txt")
+ * @opt: option name to match (e.g., "file")
+ *
+ * Returns pointer to argument value after "=" if token matches "opt=",
+ * otherwise returns NULL.
+ */
+#define extract_arg(token, opt) __extract_arg(token, opt "=", STRING_LENGTH(opt "="))
+
 /*
  * actions_parse - add an action based on text specification
  */
@@ -122,6 +145,7 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
 	enum action_type type = ACTION_NONE;
 	const char *token;
 	char trigger_c[strlen(trigger) + 1];
+	const char *arg_value;
 
 	/* For ACTION_SIGNAL */
 	int signal = 0, pid = 0;
@@ -152,12 +176,10 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
 		if (token == NULL)
 			trace_output = tracefn;
 		else {
-			if (strlen(token) > 5 && strncmp(token, "file=", 5) == 0) {
-				trace_output = token + 5;
-			} else {
+			trace_output = extract_arg(token, "file");
+			if (!trace_output)
 				/* Invalid argument */
 				return -1;
-			}
 
 			token = strtok(NULL, ",");
 			if (token != NULL)
@@ -169,17 +191,21 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
 	case ACTION_SIGNAL:
 		/* Takes two arguments, num (signal) and pid */
 		while (token != NULL) {
-			if (strlen(token) > 4 && strncmp(token, "num=", 4) == 0) {
-				if (strtoi(token + 4, &signal))
-					return -1;
-			} else if (strlen(token) > 4 && strncmp(token, "pid=", 4) == 0) {
-				if (strncmp(token + 4, "parent", 7) == 0)
-					pid = -1;
-				else if (strtoi(token + 4, &pid))
+			arg_value = extract_arg(token, "num");
+			if (arg_value) {
+				if (strtoi(arg_value, &signal))
 					return -1;
 			} else {
-				/* Invalid argument */
-				return -1;
+				arg_value = extract_arg(token, "pid");
+				if (arg_value) {
+					if (strncmp_static(arg_value, "parent") == 0)
+						pid = -1;
+					else if (strtoi(arg_value, &pid))
+						return -1;
+				} else {
+					/* Invalid argument */
+					return -1;
+				}
 			}
 
 			token = strtok(NULL, ",");
@@ -194,9 +220,10 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
 	case ACTION_SHELL:
 		if (token == NULL)
 			return -1;
-		if (strlen(token) > 8 && strncmp(token, "command=", 8))
+		arg_value = extract_arg(token, "command");
+		if (!arg_value)
 			return -1;
-		actions_add_shell(self, token + 8);
+		actions_add_shell(self, arg_value);
 		break;
 	case ACTION_CONTINUE:
 		/* Takes no argument */
diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
index efbf798650306..7fa3ac5e0bfb6 100644
--- a/tools/tracing/rtla/src/utils.h
+++ b/tools/tracing/rtla/src/utils.h
@@ -13,8 +13,18 @@
 #define MAX_NICE		20
 #define MIN_NICE		-19
 
-#define container_of(ptr, type, member)({			\
-	const typeof(((type *)0)->member) *__mptr = (ptr);	\
+#ifndef ARRAY_SIZE
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
+#endif
+
+/* Calculate string length at compile time (excluding null terminator) */
+#define STRING_LENGTH(s) (ARRAY_SIZE(s) - sizeof(*(s)))
+
+/* Compare string with static string, length determined at compile time */
+#define strncmp_static(s1, s2) strncmp(s1, s2, STRING_LENGTH(s2))
+
+#define container_of(ptr, type, member)({				\
+	const typeof(((type *)0)->member) * __mptr = (ptr);		\
 	(type *)((char *)__mptr - offsetof(type, member)) ; })
 
 extern int config_debug;
-- 
2.52.0

[PATCH v2 06/18] rtla: Use strncmp_static() in more places

[Wander Lairson Costa]

The recently introduced strncmp_static() helper provides a safer way
to compare strings with static strings by determining the length at
compile time.

Replace several open-coded strncmp() calls with strncmp_static() to
improve code readability and robustness. This change affects the
parsing of command-line arguments and environment variables.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/osnoise.c  | 2 +-
 tools/tracing/rtla/src/timerlat.c | 4 ++--
 tools/tracing/rtla/src/trace.c    | 2 +-
 tools/tracing/rtla/src/utils.c    | 8 ++++----
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c
index c5b41ec26b0a4..f2ec2da7b6d3a 100644
--- a/tools/tracing/rtla/src/osnoise.c
+++ b/tools/tracing/rtla/src/osnoise.c
@@ -1219,7 +1219,7 @@ int osnoise_main(int argc, char *argv[])
 
 	if ((strcmp(argv[1], "-h") == 0) || (strcmp(argv[1], "--help") == 0)) {
 		osnoise_usage(0);
-	} else if (strncmp(argv[1], "-", 1) == 0) {
+	} else if (strncmp_static(argv[1], "-") == 0) {
 		/* the user skipped the tool, call the default one */
 		run_tool(&osnoise_top_ops, argc, argv);
 		exit(0);
diff --git a/tools/tracing/rtla/src/timerlat.c b/tools/tracing/rtla/src/timerlat.c
index df4f9bfe34331..ac2ec89d3b6ba 100644
--- a/tools/tracing/rtla/src/timerlat.c
+++ b/tools/tracing/rtla/src/timerlat.c
@@ -34,7 +34,7 @@ timerlat_apply_config(struct osnoise_tool *tool, struct timerlat_params *params)
 	 * Try to enable BPF, unless disabled explicitly.
 	 * If BPF enablement fails, fall back to tracefs mode.
 	 */
-	if (getenv("RTLA_NO_BPF") && strncmp(getenv("RTLA_NO_BPF"), "1", 2) == 0) {
+	if (getenv("RTLA_NO_BPF") && strncmp_static(getenv("RTLA_NO_BPF"), "1") == 0) {
 		debug_msg("RTLA_NO_BPF set, disabling BPF\n");
 		params->mode = TRACING_MODE_TRACEFS;
 	} else if (!tep_find_event_by_name(tool->trace.tep, "osnoise", "timerlat_sample")) {
@@ -271,7 +271,7 @@ int timerlat_main(int argc, char *argv[])
 
 	if ((strcmp(argv[1], "-h") == 0) || (strcmp(argv[1], "--help") == 0)) {
 		timerlat_usage(0);
-	} else if (strncmp(argv[1], "-", 1) == 0) {
+	} else if (strncmp_static(argv[1], "-") == 0) {
 		/* the user skipped the tool, call the default one */
 		run_tool(&timerlat_top_ops, argc, argv);
 		exit(0);
diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
index b22bb844b71f3..45328c5121f79 100644
--- a/tools/tracing/rtla/src/trace.c
+++ b/tools/tracing/rtla/src/trace.c
@@ -356,7 +356,7 @@ static void trace_event_save_hist(struct trace_instance *instance,
 		return;
 
 	/* is this a hist: trigger? */
-	retval = strncmp(tevent->trigger, "hist:", strlen("hist:"));
+	retval = strncmp_static(tevent->trigger, "hist:");
 	if (retval)
 		return;
 
diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c
index f3e129d17a82b..e0f31e5cae844 100644
--- a/tools/tracing/rtla/src/utils.c
+++ b/tools/tracing/rtla/src/utils.c
@@ -211,15 +211,15 @@ long parse_ns_duration(char *val)
 	t = strtol(val, &end, 10);
 
 	if (end) {
-		if (!strncmp(end, "ns", 2)) {
+		if (!strncmp_static(end, "ns")) {
 			return t;
-		} else if (!strncmp(end, "us", 2)) {
+		} else if (!strncmp_static(end, "us")) {
 			t *= 1000;
 			return t;
-		} else if (!strncmp(end, "ms", 2)) {
+		} else if (!strncmp_static(end, "ms")) {
 			t *= 1000 * 1000;
 			return t;
-		} else if (!strncmp(end, "s", 1)) {
+		} else if (!strncmp_static(end, "s")) {
 			t *= 1000 * 1000 * 1000;
 			return t;
 		}
-- 
2.52.0

[PATCH v2 07/18] rtla: Introduce common_restart() helper

[Wander Lairson Costa]

A few functions duplicate the logic for handling threshold actions.
When a threshold is reached, these functions stop the trace, perform
actions, and restart the trace if configured to continue.

Create a new helper function, common_restart(), to centralize this
shared logic and avoid code duplication. This function now handles the
threshold actions and restarts the necessary trace instances.

Refactor the affected functions main loops to call the new helper.
This makes the code cleaner and more maintainable.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/common.c        | 65 +++++++++++++++++++-------
 tools/tracing/rtla/src/common.h        |  9 ++++
 tools/tracing/rtla/src/timerlat_hist.c | 20 ++++----
 tools/tracing/rtla/src/timerlat_top.c  | 20 ++++----
 4 files changed, 78 insertions(+), 36 deletions(-)

diff --git a/tools/tracing/rtla/src/common.c b/tools/tracing/rtla/src/common.c
index b197037fc58b3..d608ffe12e7b0 100644
--- a/tools/tracing/rtla/src/common.c
+++ b/tools/tracing/rtla/src/common.c
@@ -95,6 +95,37 @@ common_apply_config(struct osnoise_tool *tool, struct common_params *params)
 }
 
 
+/**
+ * common_restart - handle threshold actions and optionally restart tracing
+ * @tool: pointer to the osnoise_tool instance containing trace contexts
+ * @params: timerlat parameters with threshold action configuration
+ *
+ * Return:
+ *   RESTART_OK - Actions executed successfully and tracing restarted
+ *   RESTART_STOP - Actions executed but 'continue' flag not set, stop tracing
+ *   RESTART_ERROR - Failed to restart tracing after executing actions
+ */
+enum restart_result
+common_restart(const struct osnoise_tool *tool, struct common_params *params)
+{
+	actions_perform(&params->threshold_actions);
+
+	if (!params->threshold_actions.continue_flag)
+		/* continue flag not set, break */
+		return RESTART_STOP;
+
+	/* continue action reached, re-enable tracing */
+	if (tool->record && trace_instance_start(&tool->record->trace))
+		goto err;
+	if (tool->aa && trace_instance_start(&tool->aa->trace))
+		goto err;
+	return RESTART_OK;
+
+err:
+	err_msg("Error restarting trace\n");
+	return RESTART_ERROR;
+}
+
 int run_tool(struct tool_ops *ops, int argc, char *argv[])
 {
 	struct common_params *params;
@@ -272,17 +303,16 @@ int top_main_loop(struct osnoise_tool *tool)
 				/* stop tracing requested, do not perform actions */
 				return 0;
 
-			actions_perform(&params->threshold_actions);
+			enum restart_result result;
+
+			result = common_restart(tool, params);
 
-			if (!params->threshold_actions.continue_flag)
-				/* continue flag not set, break */
+			if (result == RESTART_STOP)
 				return 0;
 
-			/* continue action reached, re-enable tracing */
-			if (record)
-				trace_instance_start(&record->trace);
-			if (tool->aa)
-				trace_instance_start(&tool->aa->trace);
+			if (result == RESTART_ERROR)
+				return -1;
+
 			trace_instance_start(trace);
 		}
 
@@ -323,18 +353,17 @@ int hist_main_loop(struct osnoise_tool *tool)
 				/* stop tracing requested, do not perform actions */
 				break;
 
-			actions_perform(&params->threshold_actions);
+			enum restart_result result;
 
-			if (!params->threshold_actions.continue_flag)
-				/* continue flag not set, break */
-				break;
+			result = common_restart(tool, params);
+
+			if (result == RESTART_STOP)
+				return 0;
 
-			/* continue action reached, re-enable tracing */
-			if (tool->record)
-				trace_instance_start(&tool->record->trace);
-			if (tool->aa)
-				trace_instance_start(&tool->aa->trace);
-			trace_instance_start(&tool->trace);
+			if (result == RESTART_ERROR)
+				return -1;
+
+			trace_instance_start(trace);
 		}
 
 		/* is there still any user-threads ? */
diff --git a/tools/tracing/rtla/src/common.h b/tools/tracing/rtla/src/common.h
index 9ec2b7632c376..f2c9e21c03651 100644
--- a/tools/tracing/rtla/src/common.h
+++ b/tools/tracing/rtla/src/common.h
@@ -143,6 +143,15 @@ struct tool_ops {
 	void (*free)(struct osnoise_tool *tool);
 };
 
+enum restart_result {
+	RESTART_OK,
+	RESTART_STOP,
+	RESTART_ERROR = -1,
+};
+
+enum restart_result
+common_restart(const struct osnoise_tool *tool, struct common_params *params);
+
 int osnoise_set_cpus(struct osnoise_context *context, char *cpus);
 void osnoise_restore_cpus(struct osnoise_context *context);
 
diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
index 226167c88c8d6..27fc98270da59 100644
--- a/tools/tracing/rtla/src/timerlat_hist.c
+++ b/tools/tracing/rtla/src/timerlat_hist.c
@@ -17,6 +17,7 @@
 #include "timerlat.h"
 #include "timerlat_aa.h"
 #include "timerlat_bpf.h"
+#include "common.h"
 
 struct timerlat_hist_cpu {
 	int			*irq;
@@ -1100,18 +1101,19 @@ static int timerlat_hist_bpf_main_loop(struct osnoise_tool *tool)
 
 		if (!stop_tracing) {
 			/* Threshold overflow, perform actions on threshold */
-			actions_perform(&params->common.threshold_actions);
+			enum restart_result result;
 
-			if (!params->common.threshold_actions.continue_flag)
-				/* continue flag not set, break */
+			result = common_restart(tool, &params->common);
+			if (result == RESTART_STOP)
 				break;
 
-			/* continue action reached, re-enable tracing */
-			if (tool->record)
-				trace_instance_start(&tool->record->trace);
-			if (tool->aa)
-				trace_instance_start(&tool->aa->trace);
-			timerlat_bpf_restart_tracing();
+			if (result == RESTART_ERROR)
+				return -1;
+
+			if (timerlat_bpf_restart_tracing()) {
+				err_msg("Error restarting BPF trace\n");
+				return -1;
+			}
 		}
 	}
 	timerlat_bpf_detach();
diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c
index 31e1514a2528d..f7e85dc918ef3 100644
--- a/tools/tracing/rtla/src/timerlat_top.c
+++ b/tools/tracing/rtla/src/timerlat_top.c
@@ -18,6 +18,7 @@
 #include "timerlat.h"
 #include "timerlat_aa.h"
 #include "timerlat_bpf.h"
+#include "common.h"
 
 struct timerlat_top_cpu {
 	unsigned long long	irq_count;
@@ -869,18 +870,19 @@ timerlat_top_bpf_main_loop(struct osnoise_tool *tool)
 
 		if (wait_retval != 0) {
 			/* Stopping requested by tracer */
-			actions_perform(&params->common.threshold_actions);
+			enum restart_result result;
 
-			if (!params->common.threshold_actions.continue_flag)
-				/* continue flag not set, break */
+			result = common_restart(tool, &params->common);
+			if (result == RESTART_STOP)
 				break;
 
-			/* continue action reached, re-enable tracing */
-			if (tool->record)
-				trace_instance_start(&tool->record->trace);
-			if (tool->aa)
-				trace_instance_start(&tool->aa->trace);
-			timerlat_bpf_restart_tracing();
+			if (result == RESTART_ERROR)
+				return -1;
+
+			if (timerlat_bpf_restart_tracing()) {
+				err_msg("Error restarting BPF trace\n");
+				return -1;
+			}
 		}
 
 		/* is there still any user-threads ? */
-- 
2.52.0

[PATCH v2 08/18] rtla: Use standard exit codes for result enum

[Wander Lairson Costa]

The result enum defines custom values for PASSED, ERROR, and FAILED.
These values correspond to standard exit codes EXIT_SUCCESS and
EXIT_FAILURE.

Update the enum to use the standard macros EXIT_SUCCESS and
EXIT_FAILURE to improve readability and adherence to standard C
practices.

The FAILED value is implicitly assigned EXIT_FAILURE + 1, so there
is no need to assign an explicit value.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/utils.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
index 7fa3ac5e0bfb6..5286a4c7165d3 100644
--- a/tools/tracing/rtla/src/utils.h
+++ b/tools/tracing/rtla/src/utils.h
@@ -4,6 +4,7 @@
 #include <time.h>
 #include <sched.h>
 #include <stdbool.h>
+#include <stdlib.h>
 
 /*
  * '18446744073709551615\0'
@@ -102,7 +103,7 @@ __attribute__((__warn_unused_result__)) int strtoi(const char *s, int *res);
 #define ns_to_per(total, part) ((part * 100) / (double)total)
 
 enum result {
-	PASSED = 0, /* same as EXIT_SUCCESS */
-	ERROR = 1,  /* same as EXIT_FAILURE, an error in arguments */
-	FAILED = 2, /* test hit the stop tracing condition */
+	PASSED	= EXIT_SUCCESS,
+	ERROR	= EXIT_FAILURE,
+	FAILED, /* test hit the stop tracing condition */
 };
-- 
2.52.0

[PATCH v2 09/18] rtla: Remove redundant memset after calloc

[Wander Lairson Costa]

The actions struct is allocated using calloc, which already returns
zeroed memory. The subsequent memset call to zero the 'present' member
is therefore redundant.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/actions.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
index b50bf9f9adf28..00bbc94dec1bd 100644
--- a/tools/tracing/rtla/src/actions.c
+++ b/tools/tracing/rtla/src/actions.c
@@ -19,8 +19,6 @@ actions_init(struct actions *self)
 	self->len = 0;
 	self->continue_flag = false;
 
-	memset(&self->present, 0, sizeof(self->present));
-
 	/* This has to be set by the user */
 	self->trace_output_inst = NULL;
 }
-- 
2.52.0

[PATCH v2 10/18] rtla: Replace magic number with MAX_PATH

[Wander Lairson Costa]

The trace functions use a buffer to manipulate strings that will be
written to tracefs files. These buffers are defined with a magic number
of 1024, which is a common source of vulnerabilities.

Replace the magic number 1024 with the MAX_PATH macro to make the code
safer and more readable. While at it, replace other instances of the
magic number with ARRAY_SIZE() when the buffer is locally defined.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/osnoise.c    |  4 ++--
 tools/tracing/rtla/src/timerlat_u.c |  4 ++--
 tools/tracing/rtla/src/trace.c      | 20 ++++++++++----------
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/tools/tracing/rtla/src/osnoise.c b/tools/tracing/rtla/src/osnoise.c
index f2ec2da7b6d3a..68927d799dde5 100644
--- a/tools/tracing/rtla/src/osnoise.c
+++ b/tools/tracing/rtla/src/osnoise.c
@@ -52,7 +52,7 @@ char *osnoise_get_cpus(struct osnoise_context *context)
 int osnoise_set_cpus(struct osnoise_context *context, char *cpus)
 {
 	char *orig_cpus = osnoise_get_cpus(context);
-	char buffer[1024];
+	char buffer[MAX_PATH];
 	int retval;
 
 	if (!orig_cpus)
@@ -62,7 +62,7 @@ int osnoise_set_cpus(struct osnoise_context *context, char *cpus)
 	if (!context->curr_cpus)
 		return -1;
 
-	snprintf(buffer, 1024, "%s\n", cpus);
+	snprintf(buffer, ARRAY_SIZE(buffer), "%s\n", cpus);
 
 	debug_msg("setting cpus to %s from %s", cpus, context->orig_cpus);
 
diff --git a/tools/tracing/rtla/src/timerlat_u.c b/tools/tracing/rtla/src/timerlat_u.c
index ce68e39d25fde..efe2f72686486 100644
--- a/tools/tracing/rtla/src/timerlat_u.c
+++ b/tools/tracing/rtla/src/timerlat_u.c
@@ -32,7 +32,7 @@
 static int timerlat_u_main(int cpu, struct timerlat_u_params *params)
 {
 	struct sched_param sp = { .sched_priority = 95 };
-	char buffer[1024];
+	char buffer[MAX_PATH];
 	int timerlat_fd;
 	cpu_set_t set;
 	int retval;
@@ -83,7 +83,7 @@ static int timerlat_u_main(int cpu, struct timerlat_u_params *params)
 
 	/* add should continue with a signal handler */
 	while (true) {
-		retval = read(timerlat_fd, buffer, 1024);
+		retval = read(timerlat_fd, buffer, ARRAY_SIZE(buffer));
 		if (retval < 0)
 			break;
 	}
diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
index 45328c5121f79..0a81a2e4667ef 100644
--- a/tools/tracing/rtla/src/trace.c
+++ b/tools/tracing/rtla/src/trace.c
@@ -314,7 +314,7 @@ void trace_event_add_trigger(struct trace_events *event, char *trigger)
 static void trace_event_disable_filter(struct trace_instance *instance,
 				       struct trace_events *tevent)
 {
-	char filter[1024];
+	char filter[MAX_PATH];
 	int retval;
 
 	if (!tevent->filter)
@@ -326,7 +326,7 @@ static void trace_event_disable_filter(struct trace_instance *instance,
 	debug_msg("Disabling %s:%s filter %s\n", tevent->system,
 		  tevent->event ? : "*", tevent->filter);
 
-	snprintf(filter, 1024, "!%s\n", tevent->filter);
+	snprintf(filter, ARRAY_SIZE(filter), "!%s\n", tevent->filter);
 
 	retval = tracefs_event_file_write(instance->inst, tevent->system,
 					  tevent->event, "filter", filter);
@@ -345,7 +345,7 @@ static void trace_event_save_hist(struct trace_instance *instance,
 {
 	int retval, index, out_fd;
 	mode_t mode = 0644;
-	char path[1024];
+	char path[MAX_PATH];
 	char *hist;
 
 	if (!tevent)
@@ -360,7 +360,7 @@ static void trace_event_save_hist(struct trace_instance *instance,
 	if (retval)
 		return;
 
-	snprintf(path, 1024, "%s_%s_hist.txt", tevent->system, tevent->event);
+	snprintf(path, ARRAY_SIZE(path), "%s_%s_hist.txt", tevent->system, tevent->event);
 
 	printf("  Saving event %s:%s hist to %s\n", tevent->system, tevent->event, path);
 
@@ -392,7 +392,7 @@ static void trace_event_save_hist(struct trace_instance *instance,
 static void trace_event_disable_trigger(struct trace_instance *instance,
 					struct trace_events *tevent)
 {
-	char trigger[1024];
+	char trigger[MAX_PATH];
 	int retval;
 
 	if (!tevent->trigger)
@@ -406,7 +406,7 @@ static void trace_event_disable_trigger(struct trace_instance *instance,
 
 	trace_event_save_hist(instance, tevent);
 
-	snprintf(trigger, 1024, "!%s\n", tevent->trigger);
+	snprintf(trigger, ARRAY_SIZE(trigger), "!%s\n", tevent->trigger);
 
 	retval = tracefs_event_file_write(instance->inst, tevent->system,
 					  tevent->event, "trigger", trigger);
@@ -445,7 +445,7 @@ void trace_events_disable(struct trace_instance *instance,
 static int trace_event_enable_filter(struct trace_instance *instance,
 				     struct trace_events *tevent)
 {
-	char filter[1024];
+	char filter[MAX_PATH];
 	int retval;
 
 	if (!tevent->filter)
@@ -457,7 +457,7 @@ static int trace_event_enable_filter(struct trace_instance *instance,
 		return 1;
 	}
 
-	snprintf(filter, 1024, "%s\n", tevent->filter);
+	snprintf(filter, ARRAY_SIZE(filter), "%s\n", tevent->filter);
 
 	debug_msg("Enabling %s:%s filter %s\n", tevent->system,
 		  tevent->event ? : "*", tevent->filter);
@@ -480,7 +480,7 @@ static int trace_event_enable_filter(struct trace_instance *instance,
 static int trace_event_enable_trigger(struct trace_instance *instance,
 				      struct trace_events *tevent)
 {
-	char trigger[1024];
+	char trigger[MAX_PATH];
 	int retval;
 
 	if (!tevent->trigger)
@@ -492,7 +492,7 @@ static int trace_event_enable_trigger(struct trace_instance *instance,
 		return 1;
 	}
 
-	snprintf(trigger, 1024, "%s\n", tevent->trigger);
+	snprintf(trigger, ARRAY_SIZE(trigger), "%s\n", tevent->trigger);
 
 	debug_msg("Enabling %s:%s trigger %s\n", tevent->system,
 		  tevent->event ? : "*", tevent->trigger);
-- 
2.52.0

[PATCH v2 11/18] rtla: Remove unused headers

[Wander Lairson Costa]

Remove unused includes for <errno.h> and <signal.h> to clean up the
code and reduce unnecessary dependencies.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/osnoise_hist.c | 1 -
 tools/tracing/rtla/src/timerlat.c     | 1 -
 tools/tracing/rtla/src/timerlat_top.c | 1 -
 tools/tracing/rtla/src/trace.c        | 1 -
 4 files changed, 4 deletions(-)

diff --git a/tools/tracing/rtla/src/osnoise_hist.c b/tools/tracing/rtla/src/osnoise_hist.c
index af7d8621dd9d7..c8e681f732315 100644
--- a/tools/tracing/rtla/src/osnoise_hist.c
+++ b/tools/tracing/rtla/src/osnoise_hist.c
@@ -9,7 +9,6 @@
 #include <string.h>
 #include <signal.h>
 #include <unistd.h>
-#include <errno.h>
 #include <stdio.h>
 #include <time.h>
 
diff --git a/tools/tracing/rtla/src/timerlat.c b/tools/tracing/rtla/src/timerlat.c
index ac2ec89d3b6ba..b3d63506c4a62 100644
--- a/tools/tracing/rtla/src/timerlat.c
+++ b/tools/tracing/rtla/src/timerlat.c
@@ -9,7 +9,6 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
-#include <errno.h>
 #include <fcntl.h>
 #include <stdio.h>
 #include <sched.h>
diff --git a/tools/tracing/rtla/src/timerlat_top.c b/tools/tracing/rtla/src/timerlat_top.c
index f7e85dc918ef3..79ee66743bf1d 100644
--- a/tools/tracing/rtla/src/timerlat_top.c
+++ b/tools/tracing/rtla/src/timerlat_top.c
@@ -11,7 +11,6 @@
 #include <unistd.h>
 #include <stdio.h>
 #include <time.h>
-#include <errno.h>
 #include <sched.h>
 #include <pthread.h>
 
diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
index 0a81a2e4667ef..092fcab77dc4c 100644
--- a/tools/tracing/rtla/src/trace.c
+++ b/tools/tracing/rtla/src/trace.c
@@ -2,7 +2,6 @@
 #define _GNU_SOURCE
 #include <sys/sendfile.h>
 #include <tracefs.h>
-#include <signal.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <errno.h>
-- 
2.52.0

[PATCH v2 12/18] rtla: Fix NULL pointer dereference in actions_parse

[Wander Lairson Costa]

The actions_parse() function uses strtok() to tokenize the trigger
string, but does not check if the returned token is NULL before
passing it to strcmp(). If the trigger parameter is an empty string
or contains only delimiter characters, strtok() returns NULL, causing
strcmp() to dereference a NULL pointer and crash the program.

This issue can be triggered by malformed user input or edge cases in
trigger string parsing. Add a NULL check immediately after the strtok()
call to validate that a token was successfully extracted before using
it. If no token is found, the function now returns -1 to indicate a
parsing error.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/actions.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
index 00bbc94dec1bd..b0d68b5de08db 100644
--- a/tools/tracing/rtla/src/actions.c
+++ b/tools/tracing/rtla/src/actions.c
@@ -153,6 +153,8 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
 
 	strcpy(trigger_c, trigger);
 	token = strtok(trigger_c, ",");
+	if (!token)
+		return -1;
 
 	if (strcmp(token, "trace") == 0)
 		type = ACTION_TRACE_OUTPUT;
-- 
2.52.0

[PATCH v2 13/18] rtla: Fix buffer size for strncpy in timerlat_aa

[Wander Lairson Costa]

The run_thread_comm and current_comm character arrays in struct
timerlat_aa_data are defined with size MAX_COMM (24 bytes), but
strncpy() is called with MAX_COMM as the size parameter. If the
source string is exactly MAX_COMM bytes or longer, strncpy() will
copy exactly MAX_COMM bytes without null termination, potentially
causing buffer overruns when these strings are later used.

Increase the buffer sizes to MAX_COMM+1 to ensure there is always
room for the null terminator. This guarantees that even when strncpy()
copies the maximum number of characters, the buffer remains properly
null-terminated and safe to use in subsequent string operations.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/timerlat_aa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/tracing/rtla/src/timerlat_aa.c b/tools/tracing/rtla/src/timerlat_aa.c
index 31e66ea2b144c..d310fe65abace 100644
--- a/tools/tracing/rtla/src/timerlat_aa.c
+++ b/tools/tracing/rtla/src/timerlat_aa.c
@@ -47,7 +47,7 @@ struct timerlat_aa_data {
 	 * note: "unsigned long long" because they are fetch using tep_get_field_val();
 	 */
 	unsigned long long	run_thread_pid;
-	char			run_thread_comm[MAX_COMM];
+	char			run_thread_comm[MAX_COMM+1];
 	unsigned long long	thread_blocking_duration;
 	unsigned long long	max_exit_idle_latency;
 
@@ -88,7 +88,7 @@ struct timerlat_aa_data {
 	/*
 	 * Current thread.
 	 */
-	char			current_comm[MAX_COMM];
+	char			current_comm[MAX_COMM+1];
 	unsigned long long	current_pid;
 
 	/*
-- 
2.52.0

[PATCH v2 14/18] rtla: Add generated output files to gitignore

[Wander Lairson Costa]

The rtla tool generates various output files during testing and
execution, including custom trace outputs and histogram data. These
files are artifacts of running the tool with different options and
should not be tracked in version control.

Add gitignore entries for custom_filename.txt, osnoise_irq_noise_hist.txt,
osnoise_trace.txt, and timerlat_trace.txt to prevent accidentally
committing these generated files. This aligns with the existing pattern
of ignoring build artifacts and generated headers like *.skel.h.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/.gitignore | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/tracing/rtla/.gitignore b/tools/tracing/rtla/.gitignore
index 1a394ad26cc1e..4d39d64ac08c0 100644
--- a/tools/tracing/rtla/.gitignore
+++ b/tools/tracing/rtla/.gitignore
@@ -5,3 +5,7 @@ fixdep
 feature
 FEATURE-DUMP
 *.skel.h
+custom_filename.txt
+osnoise_irq_noise_hist.txt
+osnoise_trace.txt
+timerlat_trace.txt
-- 
2.52.0

[PATCH v2 15/18] rtla: Make stop_tracing variable volatile

[Wander Lairson Costa]

The stop_tracing global variable is accessed from both the signal
handler context and the main program flow without synchronization.
This creates a potential race condition where compiler optimizations
could cache the variable value in registers, preventing the signal
handler's updates from being visible to other parts of the program.

Add the volatile qualifier to stop_tracing in both common.c and
common.h to ensure all accesses to this variable bypass compiler
optimizations and read directly from memory. This guarantees that
when the signal handler sets stop_tracing, the change is immediately
visible to the main program loop, preventing potential hangs or
delayed shutdown when termination signals are received.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/common.c | 2 +-
 tools/tracing/rtla/src/common.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/tracing/rtla/src/common.c b/tools/tracing/rtla/src/common.c
index d608ffe12e7b0..1e6542a1e9630 100644
--- a/tools/tracing/rtla/src/common.c
+++ b/tools/tracing/rtla/src/common.c
@@ -8,7 +8,7 @@
 #include "common.h"
 
 struct trace_instance *trace_inst;
-int stop_tracing;
+volatile int stop_tracing;
 
 static void stop_trace(int sig)
 {
diff --git a/tools/tracing/rtla/src/common.h b/tools/tracing/rtla/src/common.h
index f2c9e21c03651..283641f3e7c9b 100644
--- a/tools/tracing/rtla/src/common.h
+++ b/tools/tracing/rtla/src/common.h
@@ -54,7 +54,7 @@ struct osnoise_context {
 };
 
 extern struct trace_instance *trace_inst;
-extern int stop_tracing;
+extern volatile int stop_tracing;
 
 struct hist_params {
 	char			no_irq;
-- 
2.52.0

[PATCH v2 16/18] rtla: Ensure null termination after read operations in utils.c

[Wander Lairson Costa]

Add explicit null termination and buffer initialization for read()
operations in procfs_is_workload_pid() and get_self_cgroup() functions.
The read() system call does not null-terminate the data it reads, and
when the buffer is filled to capacity, subsequent string operations
will read past the buffer boundary searching for a null terminator.

In procfs_is_workload_pid(), explicitly set buffer[MAX_PATH-1] to '\0'
to ensure the buffer is always null-terminated before passing it to
strncmp(). In get_self_cgroup(), use memset() to zero the path buffer
before reading, which ensures null termination when retval is less than
MAX_PATH. Additionally, set path[MAX_PATH-1] to '\0' after the read to
handle the case where the buffer is filled completely.

These defensive buffer handling practices prevent potential buffer
overruns and align with the ongoing buffer safety improvements across
the rtla codebase.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/utils.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c
index e0f31e5cae844..508b8891acd86 100644
--- a/tools/tracing/rtla/src/utils.c
+++ b/tools/tracing/rtla/src/utils.c
@@ -317,6 +317,7 @@ static int procfs_is_workload_pid(const char *comm_prefix, struct dirent *proc_e
 	if (retval <= 0)
 		return 0;
 
+	buffer[MAX_PATH-1] = '\0';
 	retval = strncmp(comm_prefix, buffer, strlen(comm_prefix));
 	if (retval)
 		return 0;
@@ -750,6 +751,7 @@ static int get_self_cgroup(char *self_cg, int sizeof_self_cg)
 	if (fd < 0)
 		return 0;
 
+	memset(path, 0, sizeof(path));
 	retval = read(fd, path, MAX_PATH);
 
 	close(fd);
@@ -757,6 +759,7 @@ static int get_self_cgroup(char *self_cg, int sizeof_self_cg)
 	if (retval <= 0)
 		return 0;
 
+	path[MAX_PATH-1] = '\0';
 	start = path;
 
 	start = strstr(start, ":");
-- 
2.52.0

[PATCH v2 17/18] rtla: Fix parse_cpu_set() return value documentation

[Wander Lairson Costa]

Correct the return value documentation for parse_cpu_set() function
in utils.c. The comment incorrectly stated that the function returns
1 on success and 0 on failure, but the actual implementation returns
0 on success and 1 on failure, following the common error-on-nonzero
convention used throughout the codebase.

This documentation fix ensures that developers reading the code
understand the correct return value semantics and prevents potential
misuse of the function's return value in conditional checks.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c
index 508b8891acd86..4093030e446ab 100644
--- a/tools/tracing/rtla/src/utils.c
+++ b/tools/tracing/rtla/src/utils.c
@@ -113,7 +113,7 @@ void get_duration(time_t start_time, char *output, int output_size)
  * Receives a cpu list, like 1-3,5 (cpus 1, 2, 3, 5), and then set
  * filling cpu_set_t argument.
  *
- * Returns 1 on success, 0 otherwise.
+ * Returns 0 on success, 1 otherwise.
  */
 int parse_cpu_set(char *cpu_list, cpu_set_t *set)
 {
-- 
2.52.0

[PATCH v2 18/18] rtla: Simplify code by caching string lengths

[Wander Lairson Costa]

Simplify trace_event_save_hist() and set_comm_cgroup() by computing
string lengths once and storing them in local variables, rather than
calling strlen() multiple times on the same unchanged strings. This
makes the code clearer by eliminating redundant function calls and
improving readability.

In trace_event_save_hist(), the write loop previously called strlen()
on the hist buffer twice per iteration for both the size calculation
and loop condition. Store the length in hist_len before entering the
loop. In set_comm_cgroup(), strlen() was called on cgroup_path up to
three times in succession. Store the result in cg_path_len to use in
both the offset calculation and size parameter for subsequent append
operations.

This simplification makes the code easier to read and maintain without
changing program behavior.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 tools/tracing/rtla/src/trace.c |  6 ++++--
 tools/tracing/rtla/src/utils.c | 11 +++++++----
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/tracing/rtla/src/trace.c b/tools/tracing/rtla/src/trace.c
index 092fcab77dc4c..223ab97e50aed 100644
--- a/tools/tracing/rtla/src/trace.c
+++ b/tools/tracing/rtla/src/trace.c
@@ -346,6 +346,7 @@ static void trace_event_save_hist(struct trace_instance *instance,
 	mode_t mode = 0644;
 	char path[MAX_PATH];
 	char *hist;
+	size_t hist_len;
 
 	if (!tevent)
 		return;
@@ -376,9 +377,10 @@ static void trace_event_save_hist(struct trace_instance *instance,
 	}
 
 	index = 0;
+	hist_len = strlen(hist);
 	do {
-		index += write(out_fd, &hist[index], strlen(hist) - index);
-	} while (index < strlen(hist));
+		index += write(out_fd, &hist[index], hist_len - index);
+	} while (index < hist_len);
 
 	free(hist);
 out_close:
diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c
index 4093030e446ab..aee7f02b1e9b4 100644
--- a/tools/tracing/rtla/src/utils.c
+++ b/tools/tracing/rtla/src/utils.c
@@ -870,6 +870,7 @@ int set_comm_cgroup(const char *comm_prefix, const char *cgroup)
 	DIR *procfs;
 	int retval;
 	int cg_fd;
+	size_t cg_path_len;
 
 	if (strlen(comm_prefix) >= MAX_PATH) {
 		err_msg("Command prefix is too long: %d < strlen(%s)\n",
@@ -883,16 +884,18 @@ int set_comm_cgroup(const char *comm_prefix, const char *cgroup)
 		return 0;
 	}
 
+	cg_path_len = strlen(cgroup_path);
+
 	if (!cgroup) {
-		retval = get_self_cgroup(&cgroup_path[strlen(cgroup_path)],
-				sizeof(cgroup_path) - strlen(cgroup_path));
+		retval = get_self_cgroup(&cgroup_path[cg_path_len],
+				sizeof(cgroup_path) - cg_path_len);
 		if (!retval) {
 			err_msg("Did not find self cgroup\n");
 			return 0;
 		}
 	} else {
-		snprintf(&cgroup_path[strlen(cgroup_path)],
-				sizeof(cgroup_path) - strlen(cgroup_path), "%s/", cgroup);
+		snprintf(&cgroup_path[cg_path_len],
+				sizeof(cgroup_path) - cg_path_len, "%s/", cgroup);
 	}
 
 	snprintf(cgroup_procs, MAX_PATH, "%s/cgroup.procs", cgroup_path);
-- 
2.52.0

Re: [PATCH v2 13/18] rtla: Fix buffer size for strncpy in timerlat_aa

[Steven Rostedt]

On Tue,  6 Jan 2026 08:49:49 -0300
Wander Lairson Costa <wander@redhat.com> wrote:

> The run_thread_comm and current_comm character arrays in struct
> timerlat_aa_data are defined with size MAX_COMM (24 bytes), but
> strncpy() is called with MAX_COMM as the size parameter. If the
> source string is exactly MAX_COMM bytes or longer, strncpy() will
> copy exactly MAX_COMM bytes without null termination, potentially
> causing buffer overruns when these strings are later used.

We should implement something like the kernel has of "strscpy()" which not
only truncates but also adds a null terminating byte.

> 
> Increase the buffer sizes to MAX_COMM+1 to ensure there is always
> room for the null terminator. This guarantees that even when strncpy()
> copies the maximum number of characters, the buffer remains properly
> null-terminated and safe to use in subsequent string operations.
> 
> Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> ---
>  tools/tracing/rtla/src/timerlat_aa.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/tracing/rtla/src/timerlat_aa.c b/tools/tracing/rtla/src/timerlat_aa.c
> index 31e66ea2b144c..d310fe65abace 100644
> --- a/tools/tracing/rtla/src/timerlat_aa.c
> +++ b/tools/tracing/rtla/src/timerlat_aa.c
> @@ -47,7 +47,7 @@ struct timerlat_aa_data {
>  	 * note: "unsigned long long" because they are fetch using tep_get_field_val();
>  	 */
>  	unsigned long long	run_thread_pid;
> -	char			run_thread_comm[MAX_COMM];
> +	char			run_thread_comm[MAX_COMM+1];

The reason why I suggest strscpy() is because now you just made every this
unaligned in the struct. 24 bytes fits nicely as 3 8 byte words. Now by
adding another byte, you just added 7 bytes of useless padding between this
and the next field.

-- Steve


>  	unsigned long long	thread_blocking_duration;
>  	unsigned long long	max_exit_idle_latency;
>  
> @@ -88,7 +88,7 @@ struct timerlat_aa_data {
>  	/*
>  	 * Current thread.
>  	 */
> -	char			current_comm[MAX_COMM];
> +	char			current_comm[MAX_COMM+1];
>  	unsigned long long	current_pid;
>  
>  	/*

Re: [PATCH v2 15/18] rtla: Make stop_tracing variable volatile

[Steven Rostedt]

On Tue,  6 Jan 2026 08:49:51 -0300
Wander Lairson Costa <wander@redhat.com> wrote:

> Add the volatile qualifier to stop_tracing in both common.c and
> common.h to ensure all accesses to this variable bypass compiler
> optimizations and read directly from memory. This guarantees that
> when the signal handler sets stop_tracing, the change is immediately
> visible to the main program loop, preventing potential hangs or
> delayed shutdown when termination signals are received.

In the kernel, this is handled via the READ_ONCE() macro. Perhaps rtla
should implement that too.

-- Steve

Re: [PATCH v2 15/18] rtla: Make stop_tracing variable volatile

[Crystal Wood]

On Tue, 2026-01-06 at 11:05 -0500, Steven Rostedt wrote:
> On Tue,  6 Jan 2026 08:49:51 -0300
> Wander Lairson Costa <wander@redhat.com> wrote:
> 
> > Add the volatile qualifier to stop_tracing in both common.c and
> > common.h to ensure all accesses to this variable bypass compiler
> > optimizations and read directly from memory. This guarantees that
> > when the signal handler sets stop_tracing, the change is immediately
> > visible to the main program loop, preventing potential hangs or
> > delayed shutdown when termination signals are received.
> 
> In the kernel, this is handled via the READ_ONCE() macro. Perhaps rtla
> should implement that too.

Or just get it from tools/include/linux/compiler.h.  No need to reinvent
the wheel (even though several other tools do).

That said, signal safety is a pretty routine use for volatile.

-Crystal

Re: [PATCH v2 07/18] rtla: Introduce common_restart() helper

[Tomas Glozar]

út 6. 1. 2026 v 14:42 odesílatel Wander Lairson Costa
<wander@redhat.com> napsal:
>
> A few functions duplicate the logic for handling threshold actions.
> When a threshold is reached, these functions stop the trace, perform
> actions, and restart the trace if configured to continue.
>
> Create a new helper function, common_restart(), to centralize this
> shared logic and avoid code duplication. This function now handles the
> threshold actions and restarts the necessary trace instances.
>
> Refactor the affected functions main loops to call the new helper.
> This makes the code cleaner and more maintainable.
>

The deduplication idea is good, but I find the name of the helper
quite confusing. The main function of the helper is not to restart
tracing, it is to handle a latency threshold overflow - restarting
tracing is only one of possible effects, and one that is only applied
when using --on-threshold continue which is not the most common use
case. Could something like common_handle_stop_tracing() perhaps be
better?

> +enum restart_result {
> +       RESTART_OK,
> +       RESTART_STOP,
> +       RESTART_ERROR = -1,
> +};

Do we really need a separate return value enum just for this one helper?

Tomas

Re: [PATCH v2 07/18] rtla: Introduce common_restart() helper

[Wander Lairson Costa]

On Wed, Jan 7, 2026 at 9:03 AM Tomas Glozar <tglozar@redhat.com> wrote:
>
> út 6. 1. 2026 v 14:42 odesílatel Wander Lairson Costa
> <wander@redhat.com> napsal:
> >
> > A few functions duplicate the logic for handling threshold actions.
> > When a threshold is reached, these functions stop the trace, perform
> > actions, and restart the trace if configured to continue.
> >
> > Create a new helper function, common_restart(), to centralize this
> > shared logic and avoid code duplication. This function now handles the
> > threshold actions and restarts the necessary trace instances.
> >
> > Refactor the affected functions main loops to call the new helper.
> > This makes the code cleaner and more maintainable.
> >
>
> The deduplication idea is good, but I find the name of the helper
> quite confusing. The main function of the helper is not to restart
> tracing, it is to handle a latency threshold overflow - restarting
> tracing is only one of possible effects, and one that is only applied
> when using --on-threshold continue which is not the most common use
> case. Could something like common_handle_stop_tracing() perhaps be
> better?
>

Sure, I will change the name in v3.

> > +enum restart_result {
> > +       RESTART_OK,
> > +       RESTART_STOP,
> > +       RESTART_ERROR = -1,
> > +};
>
> Do we really need a separate return value enum just for this one helper?
>

If it was success/failure type of return value, we wouldn't need.
However, a three state code, I think it is worth for code readiness.
Do you have something else in mind?

> Tomas
>

Re: [PATCH v2 13/18] rtla: Fix buffer size for strncpy in timerlat_aa

[Wander Lairson Costa]

On Tue, Jan 6, 2026 at 1:03 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue,  6 Jan 2026 08:49:49 -0300
> Wander Lairson Costa <wander@redhat.com> wrote:
>
...
> >       unsigned long long      run_thread_pid;
> > -     char                    run_thread_comm[MAX_COMM];
> > +     char                    run_thread_comm[MAX_COMM+1];
>
> The reason why I suggest strscpy() is because now you just made every this
> unaligned in the struct. 24 bytes fits nicely as 3 8 byte words. Now by
> adding another byte, you just added 7 bytes of useless padding between this
> and the next field.
>

Hrm, I missed that issue. Maybe I should have set MAX_COMM to 23.
Anyway, in v3 I will switch to strscpy() (maybe strlcpy() does the job).

> -- Steve
>
>
> >       unsigned long long      thread_blocking_duration;
> >       unsigned long long      max_exit_idle_latency;
> >
> > @@ -88,7 +88,7 @@ struct timerlat_aa_data {
> >       /*
> >        * Current thread.
> >        */
> > -     char                    current_comm[MAX_COMM];
> > +     char                    current_comm[MAX_COMM+1];
> >       unsigned long long      current_pid;
> >
> >       /*
>

Re: [PATCH v2 15/18] rtla: Make stop_tracing variable volatile

[Wander Lairson Costa]

On Tue, Jan 6, 2026 at 1:05 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue,  6 Jan 2026 08:49:51 -0300
> Wander Lairson Costa <wander@redhat.com> wrote:
>
> > Add the volatile qualifier to stop_tracing in both common.c and
> > common.h to ensure all accesses to this variable bypass compiler
> > optimizations and read directly from memory. This guarantees that
> > when the signal handler sets stop_tracing, the change is immediately
> > visible to the main program loop, preventing potential hangs or
> > delayed shutdown when termination signals are received.
>
> In the kernel, this is handled via the READ_ONCE() macro. Perhaps rtla
> should implement that too.
>

I considered that, but, in this use case, I saw no point because it
didn't bring any advantage and volatile was simpler.
Furthermore, as Crystal pointed out, using volatile for variables
shared with signals is a pretty standard practice.

> -- Steve
>

Re: [PATCH v2 07/18] rtla: Introduce common_restart() helper

[Tomas Glozar]

st 7. 1. 2026 v 13:43 odesílatel Wander Lairson Costa
<wander@redhat.com> napsal:
> >
> > The deduplication idea is good, but I find the name of the helper
> > quite confusing. The main function of the helper is not to restart
> > tracing, it is to handle a latency threshold overflow - restarting
> > tracing is only one of possible effects, and one that is only applied
> > when using --on-threshold continue which is not the most common use
> > case. Could something like common_handle_stop_tracing() perhaps be
> > better?
> >
>
> Sure, I will change the name in v3.
>

Thanks.

> > > +enum restart_result {
> > > +       RESTART_OK,
> > > +       RESTART_STOP,
> > > +       RESTART_ERROR = -1,
> > > +};
> >
> > Do we really need a separate return value enum just for this one helper?
> >
>
> If it was success/failure type of return value, we wouldn't need.
> However, a three state code, I think it is worth for code readiness.
> Do you have something else in mind?
>

The main loop can simply use the continue flag, just like in the old
version, no need to duplicate that information into the return value
of common_restart().

Tomas

Re: [PATCH v2 07/18] rtla: Introduce common_restart() helper

[Wander Lairson Costa]

On Wed, Jan 7, 2026 at 10:47 AM Tomas Glozar <tglozar@redhat.com> wrote:
>
> st 7. 1. 2026 v 13:43 odesílatel Wander Lairson Costa
> <wander@redhat.com> napsal:
> > >
> > > The deduplication idea is good, but I find the name of the helper
> > > quite confusing. The main function of the helper is not to restart
> > > tracing, it is to handle a latency threshold overflow - restarting
> > > tracing is only one of possible effects, and one that is only applied
> > > when using --on-threshold continue which is not the most common use
> > > case. Could something like common_handle_stop_tracing() perhaps be
> > > better?
> > >
> >
> > Sure, I will change the name in v3.
> >
>
> Thanks.
>
> > > > +enum restart_result {
> > > > +       RESTART_OK,
> > > > +       RESTART_STOP,
> > > > +       RESTART_ERROR = -1,
> > > > +};
> > >
> > > Do we really need a separate return value enum just for this one helper?
> > >
> >
> > If it was success/failure type of return value, we wouldn't need.
> > However, a three state code, I think it is worth for code readiness.
> > Do you have something else in mind?
> >
>
> The main loop can simply use the continue flag, just like in the old
> version, no need to duplicate that information into the return value
> of common_restart().
>

Ok, I will change it in v3.

> Tomas
>

Re: [PATCH v2 15/18] rtla: Make stop_tracing variable volatile

[Steven Rostedt]

On Wed, 7 Jan 2026 10:24:43 -0300
Wander Lairson Costa <wander@redhat.com> wrote:

> > In the kernel, this is handled via the READ_ONCE() macro. Perhaps rtla
> > should implement that too.
> >  
> 
> I considered that, but, in this use case, I saw no point because it
> didn't bring any advantage and volatile was simpler.
> Furthermore, as Crystal pointed out, using volatile for variables
> shared with signals is a pretty standard practice.

OK, I've just been broken in by Linus yelling at anyone defining any
variable as volatile ;-)  I now avoid it at all costs, and only have the
locations that need to be volatile defined that way.

-- Steve