Linux Trace Kernel
 help / color / mirror / Atom feed
* [PATCH] rtla/cli: Unify and improve handling of invalid option arguments
@ 2026-07-16 14:49 Tomas Glozar
  2026-07-16 16:20 ` Wander Lairson Costa
  0 siblings, 1 reply; 2+ messages in thread
From: Tomas Glozar @ 2026-07-16 14:49 UTC (permalink / raw)
  To: Steven Rostedt, Tomas Glozar
  Cc: John Kacur, Luis Goncalves, Crystal Wood, Costa Shulyupin,
	Wander Lairson Costa, LKML, linux-trace-kernel

The current handling of invalid command line option arguments is
inconsistent:

- opt_llong_callback() treats non-numerical input the same as "-1",
  which might or might not be rejected later.
- opt_int_callback() returns -1 on non-numerical input without an error
  message, which makes parsing fail silently (libsubcmd will
  automatically print the usage of the option only, no error message).
- custom callbacks abort command line parsing using fatal(), which
  displays an error message and exits, without libsubcmd printing the
  usage.

Unify this such that all invalid options, regardless of the format,
print an error message similar to the out of range case:

Error: --opt: 'value' is not a valid XY

followed by the usage of the option, e.g.:

$ rtla timerlat hist --period=1us
 Error: --period: '1us' is not a valid number

 Usage: rtla timerlat hist [<options>] [-h|--help]

    -p, --period <us>     timerlat period in us

As this is a libsubcmd help path, all option parsing failures now return
the exit code of 129 (help).

The unified handling is implemented using a new error message helper,
opt_err(), which is called from two new CLI-specific parsing functions,
strtoll_safe() and strtoi_safe(), as well as from custom helpers.

Option callback tests are updated to cover the new behavior.

Assisted-by: Claude:claude-opus-4-6
Signed-off-by: Tomas Glozar <tglozar@redhat.com>
---
Dependencies:
- "rtla/cli: Unify and improve range validation logic" patch
  Link: https://lore.kernel.org/linux-trace-kernel/20260710131554.338335-1-tglozar@redhat.com/
- "rtla: Allow unsetting non-list custom-callback CLI options" patchset
  Link: https://lore.kernel.org/linux-trace-kernel/20260629083654.1548925-1-tglozar@redhat.com/T/#u
  (transitive dependency through the above patch)

 tools/tracing/rtla/src/cli_p.h                | 126 +++++++++++++-----
 .../rtla/tests/unit/cli_opt_callback.c        |  70 +++++++---
 2 files changed, 144 insertions(+), 52 deletions(-)

diff --git a/tools/tracing/rtla/src/cli_p.h b/tools/tracing/rtla/src/cli_p.h
index b7688559ca53d..661240d44ad9a 100644
--- a/tools/tracing/rtla/src/cli_p.h
+++ b/tools/tracing/rtla/src/cli_p.h
@@ -5,6 +5,7 @@
 #error "Private header file included outside of cli.c module"
 #endif
 
+#include <errno.h>
 #include <limits.h>
 #include <linux/kernel.h>
 #include <subcmd/parse-options.h>
@@ -239,6 +240,41 @@ static int check_int_range(const struct option *opt, int value)
 #define RTLA_OPT_DEBUG OPT_BOOLEAN('D', "debug", &config_debug, \
 	"print debug info")
 
+/*
+ * Helper functions for parsing numeric option arguments.
+ */
+static void opt_err(const struct option *opt, const char *arg, const char *msg)
+{
+	fprintf(stderr, " Error: --%s: '%s' %s\n", opt->long_name, arg, msg);
+}
+
+static int strtoll_safe(const struct option *opt, const char *arg, long long *value)
+{
+	long long tmp;
+	char *end;
+
+	errno = 0;
+	tmp = strtoll(arg, &end, 10);
+	if (errno || *end || end == arg) {
+		opt_err(opt, arg, "is not a valid number");
+		return -1;
+	}
+	*value = tmp;
+	return 0;
+}
+
+static int strtoi_safe(const struct option *opt, const char *arg, int *value)
+{
+	int tmp;
+
+	if (strtoi(arg, &tmp)) {
+		opt_err(opt, arg, "is not a valid number");
+		return -1;
+	}
+	*value = tmp;
+	return 0;
+}
+
 /*
  * Common callback functions for command line options
  */
@@ -255,7 +291,8 @@ static int opt_llong_callback(const struct option *opt, const char *arg, int uns
 	if (!arg)
 		return -1;
 
-	*value = get_llong_from_str((char *)arg);
+	if (strtoll_safe(opt, arg, value))
+		return -1;
 	if (check_llong_range(opt, *value))
 		return -1;
 	return 0;
@@ -273,7 +310,7 @@ static int opt_int_callback(const struct option *opt, const char *arg, int unset
 	if (!arg)
 		return -1;
 
-	if (strtoi(arg, value))
+	if (strtoi_safe(opt, arg, value))
 		return -1;
 	if (check_int_range(opt, *value))
 		return -1;
@@ -296,8 +333,10 @@ static int opt_cpus_cb(const struct option *opt, const char *arg, int unset)
 		return -1;
 
 	retval = parse_cpu_set((char *)arg, &params->monitored_cpus);
-	if (retval)
-		fatal("Invalid -c cpu list");
+	if (retval) {
+		opt_err(opt, arg, "is not a valid cpu set");
+		return -1;
+	}
 	params->cpus = (char *)arg;
 
 	return 0;
@@ -335,8 +374,10 @@ static int opt_duration_cb(const struct option *opt, const char *arg, int unset)
 		return -1;
 
 	params->duration = parse_seconds_duration((char *)arg);
-	if (!params->duration)
-		fatal("Invalid -d duration");
+	if (!params->duration) {
+		opt_err(opt, arg, "is not a valid duration");
+		return -1;
+	}
 
 	return 0;
 }
@@ -376,8 +417,10 @@ static int opt_housekeeping_cb(const struct option *opt, const char *arg, int un
 
 	params->hk_cpus = 1;
 	retval = parse_cpu_set((char *)arg, &params->hk_cpu_set);
-	if (retval)
-		fatal("Error parsing house keeping CPUs");
+	if (retval) {
+		opt_err(opt, arg, "is not a valid cpu set");
+		return -1;
+	}
 
 	return 0;
 }
@@ -397,8 +440,10 @@ static int opt_priority_cb(const struct option *opt, const char *arg, int unset)
 		return -1;
 
 	retval = parse_prio((char *)arg, &params->sched_param);
-	if (retval == -1)
-		fatal("Invalid -P priority");
+	if (retval == -1) {
+		opt_err(opt, arg, "is not a valid priority");
+		return -1;
+	}
 	params->set_sched = 1;
 
 	return 0;
@@ -411,8 +456,10 @@ static int opt_trigger_cb(const struct option *opt, const char *arg, int unset)
 	if (unset || !arg)
 		return -1;
 
-	if (!*events)
-		fatal("--trigger requires a previous -e");
+	if (!*events) {
+		opt_err(opt, arg, "has no previous event to apply to");
+		return -1;
+	}
 
 	trace_event_add_trigger(*events, (char *)arg);
 
@@ -426,8 +473,10 @@ static int opt_filter_cb(const struct option *opt, const char *arg, int unset)
 	if (unset || !arg)
 		return -1;
 
-	if (!*events)
-		fatal("--filter requires a previous -e");
+	if (!*events) {
+		opt_err(opt, arg, "has no previous event to apply to");
+		return -1;
+	}
 
 	trace_event_add_filter(*events, (char *)arg);
 
@@ -468,7 +517,8 @@ static int opt_osnoise_auto_cb(const struct option *opt, const char *arg, int un
 	if (!arg)
 		return -1;
 
-	auto_thresh = get_llong_from_str((char *)arg);
+	if (strtoll_safe(opt, arg, &auto_thresh))
+		return -1;
 	params->common.stop_us = auto_thresh;
 	params->threshold = 1;
 
@@ -508,8 +558,10 @@ static int opt_osnoise_on_threshold_cb(const struct option *opt, const char *arg
 		return -1;
 
 	retval = actions_parse(actions, (char *)arg, "osnoise_trace.txt");
-	if (retval)
-		fatal("Invalid action %s", arg);
+	if (retval) {
+		opt_err(opt, arg, "is not a valid action");
+		return -1;
+	}
 
 	return 0;
 }
@@ -523,8 +575,10 @@ static int opt_osnoise_on_end_cb(const struct option *opt, const char *arg, int
 		return -1;
 
 	retval = actions_parse(actions, (char *)arg, "osnoise_trace.txt");
-	if (retval)
-		fatal("Invalid action %s", arg);
+	if (retval) {
+		opt_err(opt, arg, "is not a valid action");
+		return -1;
+	}
 
 	return 0;
 }
@@ -596,7 +650,8 @@ static int opt_timerlat_auto_cb(const struct option *opt, const char *arg, int u
 	if (!arg)
 		return -1;
 
-	auto_thresh = get_llong_from_str((char *)arg);
+	if (strtoll_safe(opt, arg, &auto_thresh))
+		return -1;
 	params->common.stop_total_us = auto_thresh;
 	params->common.stop_us = auto_thresh;
 	params->print_stack = auto_thresh;
@@ -623,7 +678,8 @@ static int opt_aa_only_cb(const struct option *opt, const char *arg, int unset)
 	if (!arg)
 		return -1;
 
-	auto_thresh = get_llong_from_str((char *)arg);
+	if (strtoll_safe(opt, arg, &auto_thresh))
+		return -1;
 	params->common.stop_total_us = auto_thresh;
 	params->common.stop_us = auto_thresh;
 	params->print_stack = auto_thresh;
@@ -662,8 +718,10 @@ static int opt_timerlat_on_threshold_cb(const struct option *opt, const char *ar
 		return -1;
 
 	retval = actions_parse(actions, (char *)arg, "timerlat_trace.txt");
-	if (retval)
-		fatal("Invalid action %s", arg);
+	if (retval) {
+		opt_err(opt, arg, "is not a valid action");
+		return -1;
+	}
 
 	return 0;
 }
@@ -677,8 +735,10 @@ static int opt_timerlat_on_end_cb(const struct option *opt, const char *arg, int
 		return -1;
 
 	retval = actions_parse(actions, (char *)arg, "timerlat_trace.txt");
-	if (retval)
-		fatal("Invalid action %s", arg);
+	if (retval) {
+		opt_err(opt, arg, "is not a valid action");
+		return -1;
+	}
 
 	return 0;
 }
@@ -727,8 +787,10 @@ static int opt_stack_format_cb(const struct option *opt, const char *arg, int un
 
 	*format = parse_stack_format((char *)arg);
 
-	if (*format == -1)
-		fatal("Invalid --stack-format option");
+	if (*format == -1) {
+		opt_err(opt, arg, "is not a valid stack format");
+		return -1;
+	}
 
 	return 0;
 }
@@ -736,6 +798,7 @@ static int opt_stack_format_cb(const struct option *opt, const char *arg, int un
 static int opt_timerlat_align_cb(const struct option *opt, const char *arg, int unset)
 {
 	struct timerlat_params *params = opt->value;
+	long long val;
 
 	if (unset) {
 		params->timerlat_align = false;
@@ -746,10 +809,13 @@ static int opt_timerlat_align_cb(const struct option *opt, const char *arg, int
 	if (!arg)
 		return -1;
 
-	params->timerlat_align = true;
-	params->timerlat_align_us = get_llong_from_str((char *)arg);
-	if (check_llong_range(opt, params->timerlat_align_us))
+	if (strtoll_safe(opt, arg, &val))
 		return -1;
+	if (check_llong_range(opt, val))
+		return -1;
+
+	params->timerlat_align = true;
+	params->timerlat_align_us = val;
 
 	return 0;
 }
diff --git a/tools/tracing/rtla/tests/unit/cli_opt_callback.c b/tools/tracing/rtla/tests/unit/cli_opt_callback.c
index 8439fb5c6f0b3..70e2576b1336f 100644
--- a/tools/tracing/rtla/tests/unit/cli_opt_callback.c
+++ b/tools/tracing/rtla/tests/unit/cli_opt_callback.c
@@ -46,6 +46,28 @@ START_TEST(test_opt_llong_callback_min)
 }
 END_TEST
 
+START_TEST(test_opt_llong_callback_non_numeric)
+{
+	long long test_value = 0;
+	const struct option opt = TEST_CALLBACK(&test_value, opt_llong_callback);
+
+	assert(freopen("/dev/null", "w", stderr));
+	ck_assert_int_eq(opt_llong_callback(&opt, "abc", 0), -1);
+	ck_assert_int_eq(test_value, 0);
+}
+END_TEST
+
+START_TEST(test_opt_llong_callback_non_numeric_suffix)
+{
+	long long test_value = 0;
+	const struct option opt = TEST_CALLBACK(&test_value, opt_llong_callback);
+
+	assert(freopen("/dev/null", "w", stderr));
+	ck_assert_int_eq(opt_llong_callback(&opt, "1234567890abc", 0), -1);
+	ck_assert_int_eq(test_value, 0);
+}
+END_TEST
+
 START_TEST(test_opt_llong_callback_unset)
 {
 	long long test_value = 0;
@@ -105,6 +127,7 @@ START_TEST(test_opt_int_callback_non_numeric)
 	int test_value = 0;
 	const struct option opt = TEST_CALLBACK(&test_value, opt_int_callback);
 
+	assert(freopen("/dev/null", "w", stderr));
 	ck_assert_int_eq(opt_int_callback(&opt, "abc", 0), -1);
 	ck_assert_int_eq(test_value, 0);
 }
@@ -115,6 +138,7 @@ START_TEST(test_opt_int_callback_non_numeric_suffix)
 	int test_value = 0;
 	const struct option opt = TEST_CALLBACK(&test_value, opt_int_callback);
 
+	assert(freopen("/dev/null", "w", stderr));
 	ck_assert_int_eq(opt_int_callback(&opt, "1234567890abc", 0), -1);
 	ck_assert_int_eq(test_value, 0);
 }
@@ -245,7 +269,7 @@ START_TEST(test_opt_cpus_cb_invalid)
 
 	nr_cpus = 4;
 	assert(freopen("/dev/null", "w", stderr));
-	opt_cpus_cb(&opt, "0-3,5", 0);
+	ck_assert_int_eq(opt_cpus_cb(&opt, "0-3,5", 0), -1);
 }
 END_TEST
 
@@ -299,7 +323,7 @@ START_TEST(test_opt_duration_cb_invalid)
 	const struct option opt = TEST_CALLBACK(&params, opt_duration_cb);
 
 	assert(freopen("/dev/null", "w", stderr));
-	opt_duration_cb(&opt, "abc", 0);
+	ck_assert_int_eq(opt_duration_cb(&opt, "abc", 0), -1);
 }
 END_TEST
 
@@ -361,7 +385,7 @@ START_TEST(test_opt_housekeeping_cb_invalid)
 
 	nr_cpus = 4;
 	assert(freopen("/dev/null", "w", stderr));
-	opt_housekeeping_cb(&opt, "0-3,5", 0);
+	ck_assert_int_eq(opt_housekeeping_cb(&opt, "0-3,5", 0), -1);
 }
 END_TEST
 
@@ -395,7 +419,7 @@ START_TEST(test_opt_priority_cb_invalid)
 	const struct option opt = TEST_CALLBACK(&params, opt_priority_cb);
 
 	assert(freopen("/dev/null", "w", stderr));
-	opt_priority_cb(&opt, "abc", 0);
+	ck_assert_int_eq(opt_priority_cb(&opt, "abc", 0), -1);
 }
 END_TEST
 
@@ -427,7 +451,7 @@ START_TEST(test_opt_trigger_cb_no_event)
 	const struct option opt = TEST_CALLBACK(&events, opt_trigger_cb);
 
 	assert(freopen("/dev/null", "w", stderr));
-	opt_trigger_cb(&opt, "stacktrace", 0);
+	ck_assert_int_eq(opt_trigger_cb(&opt, "stacktrace", 0), -1);
 }
 END_TEST
 
@@ -447,7 +471,7 @@ START_TEST(test_opt_filter_cb_no_event)
 	const struct option opt = TEST_CALLBACK(&events, opt_filter_cb);
 
 	assert(freopen("/dev/null", "w", stderr));
-	opt_filter_cb(&opt, "comm ~ \"rtla\"", 0);
+	ck_assert_int_eq(opt_filter_cb(&opt, "comm ~ \"rtla\"", 0), -1);
 }
 END_TEST
 
@@ -528,7 +552,7 @@ START_TEST(test_opt_osnoise_on_threshold_cb_invalid)
 	const struct option opt = TEST_CALLBACK(&actions, opt_osnoise_on_threshold_cb);
 
 	assert(freopen("/dev/null", "w", stderr));
-	opt_osnoise_on_threshold_cb(&opt, "abc", 0);
+	ck_assert_int_eq(opt_osnoise_on_threshold_cb(&opt, "abc", 0), -1);
 }
 END_TEST
 
@@ -550,7 +574,7 @@ START_TEST(test_opt_osnoise_on_end_cb_invalid)
 	const struct option opt = TEST_CALLBACK(&actions, opt_osnoise_on_end_cb);
 
 	assert(freopen("/dev/null", "w", stderr));
-	opt_osnoise_on_end_cb(&opt, "abc", 0);
+	ck_assert_int_eq(opt_osnoise_on_end_cb(&opt, "abc", 0), -1);
 }
 END_TEST
 
@@ -660,7 +684,7 @@ START_TEST(test_opt_timerlat_on_threshold_cb_invalid)
 	const struct option opt = TEST_CALLBACK(&actions, opt_timerlat_on_threshold_cb);
 
 	assert(freopen("/dev/null", "w", stderr));
-	opt_timerlat_on_threshold_cb(&opt, "abc", 0);
+	ck_assert_int_eq(opt_timerlat_on_threshold_cb(&opt, "abc", 0), -1);
 }
 END_TEST
 
@@ -682,7 +706,7 @@ START_TEST(test_opt_timerlat_on_end_cb_invalid)
 	const struct option opt = TEST_CALLBACK(&actions, opt_timerlat_on_end_cb);
 
 	assert(freopen("/dev/null", "w", stderr));
-	opt_timerlat_on_end_cb(&opt, "abc", 0);
+	ck_assert_int_eq(opt_timerlat_on_end_cb(&opt, "abc", 0), -1);
 }
 END_TEST
 
@@ -782,7 +806,7 @@ START_TEST(test_opt_stack_format_cb_invalid)
 	const struct option opt = TEST_CALLBACK(&stack_format, opt_stack_format_cb);
 
 	assert(freopen("/dev/null", "w", stderr));
-	opt_stack_format_cb(&opt, "abc", 0);
+	ck_assert_int_eq(opt_stack_format_cb(&opt, "abc", 0), -1);
 }
 END_TEST
 
@@ -807,6 +831,8 @@ Suite *cli_opt_callback_suite(void)
 	tcase_add_test(tc, test_opt_llong_callback_simple);
 	tcase_add_test(tc, test_opt_llong_callback_max);
 	tcase_add_test(tc, test_opt_llong_callback_min);
+	tcase_add_test(tc, test_opt_llong_callback_non_numeric);
+	tcase_add_test(tc, test_opt_llong_callback_non_numeric_suffix);
 	tcase_add_test(tc, test_opt_llong_callback_unset);
 	tcase_add_test(tc, test_opt_llong_callback_unset_defval);
 	tcase_add_test(tc, test_opt_llong_callback_range_in);
@@ -825,25 +851,25 @@ Suite *cli_opt_callback_suite(void)
 	tcase_add_test(tc, test_opt_int_callback_range_above);
 	tcase_add_test(tc, test_opt_int_callback_range_boundary);
 	tcase_add_test(tc, test_opt_cpus_cb);
-	tcase_add_exit_test(tc, test_opt_cpus_cb_invalid, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_cpus_cb_invalid);
 	tcase_add_test(tc, test_opt_cgroup_cb);
 	tcase_add_test(tc, test_opt_cgroup_cb_equals);
 	tcase_add_test(tc, test_opt_cgroup_cb_unset);
 	tcase_add_test(tc, test_opt_duration_cb);
 	tcase_add_test(tc, test_opt_duration_cb_unset);
-	tcase_add_exit_test(tc, test_opt_duration_cb_invalid, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_duration_cb_invalid);
 	tcase_add_test(tc, test_opt_event_cb);
 	tcase_add_test(tc, test_opt_event_cb_multiple);
 	tcase_add_test(tc, test_opt_housekeeping_cb);
-	tcase_add_exit_test(tc, test_opt_housekeeping_cb_invalid, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_housekeeping_cb_invalid);
 	tcase_add_test(tc, test_opt_housekeeping_cb_unset);
 	tcase_add_test(tc, test_opt_priority_cb);
-	tcase_add_exit_test(tc, test_opt_priority_cb_invalid, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_priority_cb_invalid);
 	tcase_add_test(tc, test_opt_priority_cb_unset);
 	tcase_add_test(tc, test_opt_trigger_cb);
-	tcase_add_exit_test(tc, test_opt_trigger_cb_no_event, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_trigger_cb_no_event);
 	tcase_add_test(tc, test_opt_filter_cb);
-	tcase_add_exit_test(tc, test_opt_filter_cb_no_event, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_filter_cb_no_event);
 	suite_add_tcase(s, tc);
 
 	tc = tcase_create("osnoise");
@@ -853,9 +879,9 @@ Suite *cli_opt_callback_suite(void)
 	tcase_add_test(tc, test_opt_osnoise_trace_output_cb_noarg);
 	tcase_add_test(tc, test_opt_osnoise_trace_output_cb_unset);
 	tcase_add_test(tc, test_opt_osnoise_on_threshold_cb);
-	tcase_add_exit_test(tc, test_opt_osnoise_on_threshold_cb_invalid, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_osnoise_on_threshold_cb_invalid);
 	tcase_add_test(tc, test_opt_osnoise_on_end_cb);
-	tcase_add_exit_test(tc, test_opt_osnoise_on_end_cb_invalid, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_osnoise_on_end_cb_invalid);
 	suite_add_tcase(s, tc);
 
 	tc = tcase_create("timerlat");
@@ -867,15 +893,15 @@ Suite *cli_opt_callback_suite(void)
 	tcase_add_test(tc, test_opt_timerlat_trace_output_cb_noarg);
 	tcase_add_test(tc, test_opt_timerlat_trace_output_cb_unset);
 	tcase_add_test(tc, test_opt_timerlat_on_threshold_cb);
-	tcase_add_exit_test(tc, test_opt_timerlat_on_threshold_cb_invalid, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_timerlat_on_threshold_cb_invalid);
 	tcase_add_test(tc, test_opt_timerlat_on_end_cb);
-	tcase_add_exit_test(tc, test_opt_timerlat_on_end_cb_invalid, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_timerlat_on_end_cb_invalid);
 	tcase_add_test(tc, test_opt_user_threads_cb);
 	tcase_add_test(tc, test_opt_user_threads_cb_unset);
 	tcase_add_test(tc, test_opt_nano_cb);
 	tcase_add_test(tc, test_opt_nano_cb_unset);
 	tcase_add_test(tc, test_opt_stack_format_cb);
-	tcase_add_exit_test(tc, test_opt_stack_format_cb_invalid, EXIT_FAILURE);
+	tcase_add_test(tc, test_opt_stack_format_cb_invalid);
 	tcase_add_test(tc, test_opt_stack_format_cb_unset);
 	tcase_add_test(tc, test_opt_timerlat_align_cb);
 	tcase_add_test(tc, test_opt_timerlat_align_cb_invalid);
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-07-16 16:21 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-16 14:49 [PATCH] rtla/cli: Unify and improve handling of invalid option arguments Tomas Glozar
2026-07-16 16:20 ` Wander Lairson Costa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox