[PATCH 1/2] rtla/timerlat: Fix parsing of short options with attached arguments

[PATCH 1/2] rtla/timerlat: Fix parsing of short options with attached arguments

[John Kacur]

The timerlat hist command fails to parse short options with attached
numeric arguments (e.g., -p100) due to conflicts between digit characters
used as option values and numeric arguments to other options.

This issue was discovered when testing rtla 7.1.0-rc6 with rteval,
which passes arguments in the compact -p100 format. The rteval tests
failed with the confusing error "no-irq and no-thread set, there is
nothing to do here" even though neither option was specified.

The root cause is two-fold:

1. Digit characters ('0'-'9') were used as short option values for
   long-only options like --no-irq, --no-thread, etc. This caused
   getopt_auto() to generate an option string like 'a:b:...:u0123456:7:8:9'
   which made getopt treat digits as valid option characters.

2. The two-phase option parsing approach (alternating calls between
   common_parse_options() and local option parsing) confused getopt's
   internal state when encountering arguments like -p100.

When a user passed -p100, getopt would incorrectly parse it as three
separate options: -p, -1, -0, and -0, silently setting no_irq and
no_thread flags instead of recognizing "100" as the period argument.

The two-phase parsing was introduced in commit 850cd24cb6d6 ("tools/rtla:
Add common_parse_options()") which first appeared in v7.0-rc1. Prior to
that commit, -p100 worked correctly. The digit characters as option
values existed since the original timerlat implementation, but only
became problematic when combined with the two-phase parsing approach.

Fix this by:

1. Eliminating digit characters from the option string by filtering them
   out in getopt_auto(). This prevents conflicts with numeric arguments.

2. Refactoring timerlat_hist_parse_args() to use single-pass option
   parsing. Instead of alternating between common_parse_options() and
   local parsing, merge all options (common and local) into a single
   option table and parse them in one pass. This matches the approach
   used by cyclictest and other tools.

With these changes, all argument formats work correctly:
  -p 100        (short with space)
  -p100         (short without space)
  --period=100  (long with =)
  --period 100  (long with space)

This maintains compatibility with existing usage while enabling the
compact -p100 format that users expect from similar tools.

Assisted-by: Claude:claude-sonnet-4-5
Signed-off-by: John Kacur <jkacur@redhat.com>
---
 tools/tracing/rtla/src/common.c        |  4 ++
 tools/tracing/rtla/src/timerlat_hist.c | 55 ++++++++++++++++++++++++--
 2 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/tools/tracing/rtla/src/common.c b/tools/tracing/rtla/src/common.c
index 35e3d3aa922e..c2fd051c562c 100644
--- a/tools/tracing/rtla/src/common.c
+++ b/tools/tracing/rtla/src/common.c
@@ -65,6 +65,10 @@ int getopt_auto(int argc, char **argv, const struct option *long_opts)
 		if (long_opts[i].val < 32 || long_opts[i].val > 127)
 			continue;
 
+		/* Skip digit characters to avoid conflicts with numeric arguments */
+		if (long_opts[i].val >= '0' && long_opts[i].val <= '9')
+			continue;
+
 		if (n + 4 >= sizeof(opts))
 			fatal("optstring buffer overflow");
 
diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
index 79142af4f566..c0b6d7c30114 100644
--- a/tools/tracing/rtla/src/timerlat_hist.c
+++ b/tools/tracing/rtla/src/timerlat_hist.c
@@ -787,11 +787,24 @@ static struct common_params
 		static struct option long_options[] = {
 			{"auto",		required_argument,	0, 'a'},
 			{"bucket-size",		required_argument,	0, 'b'},
+			/* Common options */
+			{"cpus",		required_argument,	0, 'c'},
+			{"cgroup",		optional_argument,	0, 'C'},
+			{"debug",		no_argument,		0, 'D'},
+			{"duration",		required_argument,	0, 'd'},
+			{"event",		required_argument,	0, 'e'},
+			/* End common options */
 			{"entries",		required_argument,	0, 'E'},
 			{"help",		no_argument,		0, 'h'},
+			/* Common option */
+			{"house-keeping",	required_argument,	0, 'H'},
+			/* End common option */
 			{"irq",			required_argument,	0, 'i'},
 			{"nano",		no_argument,		0, 'n'},
 			{"period",		required_argument,	0, 'p'},
+			/* Common option */
+			{"priority",		required_argument,	0, 'P'},
+			/* End common option */
 			{"stack",		required_argument,	0, 's'},
 			{"thread",		required_argument,	0, 'T'},
 			{"trace",		optional_argument,	0, 't'},
@@ -819,9 +832,6 @@ static struct common_params
 			{0, 0, 0, 0}
 		};
 
-		if (common_parse_options(argc, argv, &params->common))
-			continue;
-
 		c = getopt_auto(argc, argv, long_options);
 
 		/* detect the end of the options. */
@@ -850,6 +860,35 @@ static struct common_params
 			    params->common.hist.bucket_size >= 1000000)
 				fatal("Bucket size needs to be > 0 and <= 1000000");
 			break;
+		case 'c':
+			if (parse_cpu_set(optarg, &params->common.monitored_cpus))
+				fatal("Invalid -c cpu list");
+			params->common.cpus = optarg;
+			break;
+		case 'C':
+			params->common.cgroup = 1;
+			params->common.cgroup_name = parse_optional_arg(argc, argv);
+			break;
+		case 'D':
+			config_debug = 1;
+			break;
+		case 'd':
+			params->common.duration = parse_seconds_duration(optarg);
+			if (!params->common.duration)
+				fatal("Invalid -d duration");
+			break;
+		case 'e':
+			{
+				struct trace_events *tevent;
+				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;
+			}
+			break;
 		case 'E':
 			params->common.hist.entries = get_llong_from_str(optarg);
 			if (params->common.hist.entries < 10 ||
@@ -860,6 +899,11 @@ static struct common_params
 		case '?':
 			timerlat_hist_usage();
 			break;
+		case 'H':
+			params->common.hk_cpus = 1;
+			if (parse_cpu_set(optarg, &params->common.hk_cpu_set))
+				fatal("Error parsing house keeping CPUs");
+			break;
 		case 'i':
 			params->common.stop_us = get_llong_from_str(optarg);
 			break;
@@ -874,6 +918,11 @@ static struct common_params
 			if (params->timerlat_period_us > 1000000)
 				fatal("Period longer than 1 s");
 			break;
+		case 'P':
+			if (parse_prio(optarg, &params->common.sched_param) == -1)
+				fatal("Invalid -P priority");
+			params->common.set_sched = 1;
+			break;
 		case 's':
 			params->print_stack = get_llong_from_str(optarg);
 			break;
-- 
2.54.0

[PATCH 2/2] rtla/timerlat: Add tests for option parsing with attached arguments

[John Kacur]

Add tests to verify that numeric arguments work correctly with both
attached and detached formats:
  -p 100        (short with space)
  -p100         (short without space)
  --period=100  (long with =)
  --period 100  (long with space)

These tests prevent regression of the bug fixed in commit eefa8af46ff7
("rtla/timerlat: Fix parsing of short options with attached arguments")
where -p100 was incorrectly parsed as multiple separate options.

The tests verify that:
1. All four argument formats succeed (exit code 0)
2. None trigger the "no-irq and no-thread" error that occurred when
   the bug was present

Assisted-by: Claude:claude-sonnet-4-5
Signed-off-by: John Kacur <jkacur@redhat.com>
---
 tools/tracing/rtla/tests/timerlat.t | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/tracing/rtla/tests/timerlat.t b/tools/tracing/rtla/tests/timerlat.t
index fd4935fd7b49..1a63301f5d70 100644
--- a/tools/tracing/rtla/tests/timerlat.t
+++ b/tools/tracing/rtla/tests/timerlat.t
@@ -42,6 +42,16 @@ check "verify -c/--cpus" \
 check "hist test in nanoseconds" \
 	"timerlat hist -i 2 -c 0 -n -d 10s" 2 "ns"
 
+# Option parsing tests - verify attached numeric arguments work correctly
+check "verify -p with space" \
+	"timerlat hist -p 100 -c 0 -d 1s" 0 "" "no-irq and no-thread"
+check "verify -p without space (attached argument)" \
+	"timerlat hist -p100 -c 0 -d 1s" 0 "" "no-irq and no-thread"
+check "verify --period with equals" \
+	"timerlat hist --period=100 -c 0 -d 1s" 0 "" "no-irq and no-thread"
+check "verify --period with space" \
+	"timerlat hist --period 100 -c 0 -d 1s" 0 "" "no-irq and no-thread"
+
 # Actions tests
 check "trace output through -t" \
 	"timerlat hist -T 2 -t" 2 "^  Saving trace to timerlat_trace.txt$"
-- 
2.54.0

Re: [PATCH 1/2] rtla/timerlat: Fix parsing of short options with attached arguments

[Tomas Glozar]

po 1. 6. 2026 v 23:15 odesílatel John Kacur <jkacur@redhat.com> napsal:
>
> The timerlat hist command fails to parse short options with attached
> numeric arguments (e.g., -p100) due to conflicts between digit characters
> used as option values and numeric arguments to other options.
>
> This issue was discovered when testing rtla 7.1.0-rc6 with rteval,
> which passes arguments in the compact -p100 format. The rteval tests
> failed with the confusing error "no-irq and no-thread set, there is
> nothing to do here" even though neither option was specified.
>
> The root cause is two-fold:
>
> 1. Digit characters ('0'-'9') were used as short option values for
>    long-only options like --no-irq, --no-thread, etc. This caused
>    getopt_auto() to generate an option string like 'a:b:...:u0123456:7:8:9'
>    which made getopt treat digits as valid option characters.
>
> 2. The two-phase option parsing approach (alternating calls between
>    common_parse_options() and local option parsing) confused getopt's
>    internal state when encountering arguments like -p100.
>

What actually happens is that the call to getopt_long() in
common_parse_options() does not recognize -p, but it still increments
the internal static variable nextchar by 1 before falling back to
timerlat_hist_parse_args()'s getopt_long(). That means -p is ignored
entirely and timerlat_hist_parse_args() only sees -100.

Note that options are not required to trigger the bug:

$ rtla timerlat hist -nu -c 0 -d 1s
# RTLA timerlat histogram
# Time unit is microseconds (us)
# Duration:   0 00:00:02
(rtla 7.0)

vs:

$ rtla timerlat hist -nu -c 0 -d 1s
rtla timerlat hist -nu -c 0 -d 1s
# RTLA timerlat histogram
# Time unit is nanoseconds (ns)
# Duration:   0 00:00:02
(rtla 6.19)

Again, the nanosecond option gets dropped by the
common_parse_options() mechanism pushing nextchar to 1.

> When a user passed -p100, getopt would incorrectly parse it as three
> separate options: -p, -1, -0, and -0, silently setting no_irq and
> no_thread flags instead of recognizing "100" as the period argument.
>
> The two-phase parsing was introduced in commit 850cd24cb6d6 ("tools/rtla:
> Add common_parse_options()") which first appeared in v7.0-rc1. Prior to
> that commit, -p100 worked correctly. The digit characters as option
> values existed since the original timerlat implementation, but only
> became problematic when combined with the two-phase parsing approach.
>

Note that RTLA documentation only ever mentions the syntax "-p 100".
Nevertheless, this is a real regression, and it's not unreasonable for
users to assume the syntax without the space also works, as is common
for most commands on Un*x, for example, gcc's -I/include/path syntax.

> Fix this by:
>
> 1. Eliminating digit characters from the option string by filtering them
>    out in getopt_auto(). This prevents conflicts with numeric arguments.
>
> 2. Refactoring timerlat_hist_parse_args() to use single-pass option
>    parsing. Instead of alternating between common_parse_options() and
>    local parsing, merge all options (common and local) into a single
>    option table and parse them in one pass. This matches the approach
>    used by cyclictest and other tools.

This is a partial revert of the common_parse_options() patchset [1],
it does fix the bug but only for one tool (timerlat hist).
getopt_long()'s design does not allow the user to reset its internal
nextchar variable; it can be reset (by calling it with optind = 0, not
1 as the documentation says) but that would require a lot of work, as
we'd have to calculate and restore the original nextchar. It might be
the easiest to revert the entire consolidation patchset [1], if that's
worth it.

[1] https://lore.kernel.org/linux-trace-kernel/20251209100047.2692515-1-costa.shul@redhat.com/T/#u

<truncated>



Tomas