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 | 37 ++++++++++++++++++++++----------
 tools/tracing/rtla/src/utils.h   | 14 ++++++++++--
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
index e23d4f1c5a592..2a01ece78454c 100644
--- a/tools/tracing/rtla/src/actions.c
+++ b/tools/tracing/rtla/src/actions.c
@@ -143,15 +143,30 @@ actions_add_continue(struct actions *self)
 	return 0;
 }
 
+/*
+ * 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) (				\
+	strlen(token) > STRING_LENGTH(opt "=") &&		\
+	!strncmp_static(token, opt "=")				\
+		? (token) + STRING_LENGTH(opt "=") : NULL )
+
 /*
  * actions_parse - add an action based on text specification
  */
 int
 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;
@@ -182,12 +197,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)
@@ -198,14 +211,15 @@ 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))
+			arg_value = extract_arg(token, "num");
+			if (arg_value) {
+				if (!strtoi(arg_value, &signal))
 					return -1;
-			} else if (strlen(token) > 4 && strncmp(token, "pid=", 4) == 0) {
-				if (strncmp(token + 4, "parent", 7) == 0)
+			} else if ((arg_value = extract_arg(token, "pid"))) {
+				if (strncmp_static(arg_value, "parent") == 0)
 					pid = -1;
 				else
-					if (!strtoi(token + 4, &pid))
+					if (!strtoi(arg_value, &pid))
 						return -1;
 			} else {
 				/* Invalid argument */
@@ -223,8 +237,9 @@ 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) == 0)
-			return actions_add_shell(self, token + 8);
+		arg_value = extract_arg(token, "command");
+		if (arg_value)
+			return actions_add_shell(self, arg_value);
 		return -1;
 	case ACTION_CONTINUE:
 		/* Takes no argument */
diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
index 160491f5de91c..f7ff548f7fba7 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.51.1

On Mon, 2025-11-17 at 15:41 -0300, Wander Lairson Costa wrote:

>  
> +/*
> + * 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) (				\
> +	strlen(token) > STRING_LENGTH(opt "=") &&		\
> +	!strncmp_static(token, opt "=")				\
> +		? (token) + STRING_LENGTH(opt "=") : NULL )

This could be implemented as a function (albeit without the
concatenation trick)... but if it must be a macro, at least encase it
with ({ }) so it behaves more like a function.

> +
>  /*
>   * actions_parse - add an action based on text specification
>   */
>  int
>  actions_parse(struct actions *self, const char *trigger, const char *tracefn)
>  {
> +
>  	enum action_type type = ACTION_NONE;

Why this blank line?


> diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
> index 160491f5de91c..f7ff548f7fba7 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);	\
[snip]
> +#define container_of(ptr, type, member)({				\
> +	const typeof(((type *)0)->member) *__mptr = (ptr);		\
>  	(type *)((char *)__mptr - offsetof(type, member)) ; })

It's easier to review patches when they don't make unrelated aesthetic
changes...

-Crystal

On Mon, Nov 24, 2025 at 06:46:33PM -0600, Crystal Wood wrote:
> On Mon, 2025-11-17 at 15:41 -0300, Wander Lairson Costa wrote:
> 
> >  
> > +/*
> > + * 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) (				\
> > +	strlen(token) > STRING_LENGTH(opt "=") &&		\
> > +	!strncmp_static(token, opt "=")				\
> > +		? (token) + STRING_LENGTH(opt "=") : NULL )
> 
> This could be implemented as a function (albeit without the
> concatenation trick)... but if it must be a macro, at least encase it
> with ({ }) so it behaves more like a function.
> 
> > +
> >  /*
> >   * actions_parse - add an action based on text specification
> >   */
> >  int
> >  actions_parse(struct actions *self, const char *trigger, const char *tracefn)
> >  {
> > +
> >  	enum action_type type = ACTION_NONE;
> 
> Why this blank line?
> 

Must be a typo during the rebase process. I will remove it.

> 
> > diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
> > index 160491f5de91c..f7ff548f7fba7 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);	\
> [snip]
> > +#define container_of(ptr, type, member)({				\
> > +	const typeof(((type *)0)->member) *__mptr = (ptr);		\
> >  	(type *)((char *)__mptr - offsetof(type, member)) ; })
> 
> It's easier to review patches when they don't make unrelated aesthetic
> changes...
> 

It is just git messing up the diff. No actual change.

> -Crystal
>

On Tue, 2025-11-25 at 10:45 -0300, Wander Lairson Costa wrote:
> On Mon, Nov 24, 2025 at 06:46:33PM -0600, Crystal Wood wrote:
> > On Mon, 2025-11-17 at 15:41 -0300, Wander Lairson Costa wrote:
> > > diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
> > > index 160491f5de91c..f7ff548f7fba7 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);	\
> > [snip]
> > > +#define container_of(ptr, type, member)({				\
> > > +	const typeof(((type *)0)->member) *__mptr = (ptr);		\
> > >  	(type *)((char *)__mptr - offsetof(type, member)) ; })
> > 
> > It's easier to review patches when they don't make unrelated aesthetic
> > changes...
> > 
> 
> It is just git messing up the diff. No actual change.

The change was to the position of the backslashes.  Not a big deal, just
something that's helpful to avoid to get a cleaner diff.

-Crystal