linux-kernel - Re: [PATCH v1] tools/rtla: Fix parse_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAP4=nvRUA2VvCVXO4J5zA3XMUWnG8L-1zhvWUYJ5atRQuPb4yQ@mail.gmail.com>
Date: Mon, 12 Jan 2026 15:50:17 +0100
From: Tomas Glozar <tglozar@...hat.com>
To: Costa Shulyupin <costa.shul@...hat.com>
Cc: Steven Rostedt <rostedt@...dmis.org>, Wander Lairson Costa <wander@...hat.com>, 
	Ivan Pravdin <ipravdin.official@...il.com>, Tiezhu Yang <yangtiezhu@...ngson.cn>, 
	linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH v1] tools/rtla: Fix parse_cpu_set() and add unit test

po 12. 1. 2026 v 14:22 odesílatel Costa Shulyupin
<costa.shul@...hat.com> napsal:
>
> The patch "Replace atoi() with a robust strtoi()" introduced a bug
> in parse_cpu_set(), which relies on partial parsing of the input
> string.
>
> Restore the original use of atoi() in parse_cpu_set().
>
> Add a unit test to prevent accidental regressions.
>
> Fixes: 7e9dfccf8f11 ("rtla: Replace atoi() with a robust strtoi()")
>
> Signed-off-by: Costa Shulyupin <costa.shul@...hat.com>

The convention is not to insert a blank line between Fixes and Signed-off-by.

> ---
>  tools/tracing/rtla/Makefile           |  3 ++
>  tools/tracing/rtla/src/utils.c        | 10 ++--
>  tools/tracing/rtla/tests/Makefile     | 12 +++++
>  tools/tracing/rtla/tests/test_utils.c | 74 +++++++++++++++++++++++++++
>  4 files changed, 93 insertions(+), 6 deletions(-)
>  create mode 100644 tools/tracing/rtla/tests/Makefile
>  create mode 100644 tools/tracing/rtla/tests/test_utils.c
>

Could you split this into three commits?

1. Fix the bug.
2. Add unit test framework.
3. Add the specific unit tests for utils.

IMHO, it would be easier to modify the test in tests/timerlat.t to
cover this case for now, and implement unit tests later in an entirely
separate patchset. (See comments for the unit tests below.)

> diff --git a/tools/tracing/rtla/Makefile b/tools/tracing/rtla/Makefile
> index 2701256abaf3..1805916c7dba 100644
> --- a/tools/tracing/rtla/Makefile
> +++ b/tools/tracing/rtla/Makefile
> @@ -109,7 +109,10 @@ clean: doc_clean fixdep-clean
>         $(Q)rm -f rtla rtla-static fixdep FEATURE-DUMP rtla-*
>         $(Q)rm -rf feature
>         $(Q)rm -f src/timerlat.bpf.o src/timerlat.skel.h example/timerlat_bpf_action.o
> +
> check: $(RTLA) tests/bpf/bpf_action_map.o
> +       make -C tests/ check
>        RTLA=$(RTLA) BPFTOOL=$(SYSTEM_BPFTOOL) prove -o -f -v tests/
> +
> examples: example/timerlat_bpf_action.o
> .PHONY: FORCE clean check

The unit tests should be a separate target, so that you can run unit
tests and end-to-end tests separately. Also, the directory structure
should reflect that there are two types of tests. For example, in
rteval, Test:Harness-based end-to-end tests (inspired by and very
similar to RTLA tests) are in tests/e2e while unit tests are in tests/
directly; it might be even better to have them in tests/unit.

> diff --git a/tools/tracing/rtla/tests/test_utils.c b/tools/tracing/rtla/tests/test_utils.c
> new file mode 100644
> index 000000000000..92ed49d60d33
> --- /dev/null
> +++ b/tools/tracing/rtla/tests/test_utils.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Unit tests for src/utils.c parsing functions
> + */
> +
> +#define _GNU_SOURCE
> +#include <check.h>
> +#include <unistd.h>
> +
> +#include "../src/utils.h"
> +

Here, you add an external dependency on libcheck. That has to be
documented in both the commit message and README, and it should also
be added to tools/build dependency detection (in a separate commit)
first before being added to RTLA, so that RTLA can check for the
dependency and disable unit tests if it is not present.

> diff --git a/tools/tracing/rtla/tests/Makefile b/tools/tracing/rtla/tests/Makefile
> new file mode 100644
> index 000000000000..fe187306a404
> --- /dev/null
> +++ b/tools/tracing/rtla/tests/Makefile
> @@ -0,0 +1,12 @@
> +LIBS := -lcheck
> +
> +test_utils: test_utils.c ../src/utils.c
> +       $(CC) $(CFLAGS) -o $@ $^ $(LIBS)
> +
> +check: test_utils
> +       ./test_utils
> +
> +clean:
> +       rm -f test_utils
> +
> +.PHONY: check clean

Is there a reason for doing a separate Makefile for the tests? That
will require explicitly passing variables to the child make during the
build. It appears to me that it would be more natural to just include
unit tests into the main Makefile (or a file included from it, e.g.
Makefile.test), which already covers end-to-end tests. That would also
allow it to use RTLA build system's dependency detection.

Thanks,
Tomas