Re: [LTP] ftrace: Add common library for C implementation

[linuxtestproject.agent@gmail.com]

Hi Praveen,

On Mon, 8 Jun 2026 18:55:22 +0530, Praveen K Pandey wrote:
> ftrace: Add common library for C implementation

--- [PATCH 1/5] ---

> +// Made with Bob

What is this? This is a C++ style comment (kernel style requires
/* ... */) and seems like a leftover that should be removed.

> +int ftrace_set_tracer(const char *tracer)
> +{
> +	char *path;
> +	int ret;

`ret` is declared but never used in this function since
SAFE_FILE_PRINTF() is used for the write. Should be removed.

> +	if (saved_state.trace_options) {
> +		char *options = strdup(saved_state.trace_options);
> +		char *token = strtok(options, "\n");

If strdup() returns NULL (OOM), passing NULL as the first argument
to strtok() on the first call is undefined behavior. The other
strdup() call in ftrace_initialize() checks for NULL, so this one
should too.

--- [PATCH 2/5] ---

> ftrace: Convert ftrace_regression01.sh to C
>
> Convert ftrace_regression01.sh to C using the new LTP test API.
> This test validates basic ftrace functionality by testing all
> available tracers.

The commit body does not match the test. The code (and the doc
block) test for a kernel panic triggered by userstacktrace with
page fault events, not "basic ftrace functionality by testing all
available tracers."

> - Sets .min_kver = "2.6.29" for kernel version requirement

The code does not set .min_kver at all. The commit body should
match the actual struct tst_test definition.

--- [PATCH 3/5] ---

> ftrace: Convert ftrace_regression02.sh to C
>
> - Sets .min_kver = "2.6.29" for kernel version requirement

The code sets .min_kver = "3.2", not "2.6.29".

--- [PATCH 4/5] ---

> +static volatile int stop_testing = 0;

> +static void stop_stress_tests(void)
> +{
> +	int i;
> +
> +	stop_testing = 1;
> +
> +	for (i = 0; i < thread_count; i++) {
> +		pthread_join(threads[i], NULL);
> +	}

> +static void run_test(void)
> +{
> +	...
> +	stop_stress_tests();

> +static void cleanup(void)
> +{
> +	if (thread_count > 0) {
> +		stop_testing = 1;
> +		stop_stress_tests();
> +	}

On the normal path, run_test() calls stop_stress_tests() which
joins all threads, then cleanup() is called and
stop_stress_tests() runs again. Calling pthread_join() on
already-joined threads is undefined behavior.

One fix: reset thread_count to 0 at the end of
stop_stress_tests().

Also, stop_testing is never reset to 0 between iterations, so
running with -i N would cause iterations 2+ to spawn threads
that exit immediately (all threads check
`while (!stop_testing)`). Both stop_testing and thread_count
should be re-initialized in run_test() or setup().

--- [PATCH 5/5] ---

> diff --git a/testcases/kernel/tracing/ftrace_test/Makefile
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> +
> +# C test binaries with shared library dependencies
> +ftrace_regression01:	ftrace_lib.o
> +ftrace_regression02:	ftrace_lib.o
> +ftrace_stress_test:	ftrace_lib.o

The Makefile changes (FILTER_OUT_MAKE_TARGETS, library
dependencies, -lpthread) are deferred to patch 5, but the C
source files are added in patches 1-4. This means the build
breaks at every intermediate commit: patch 1 adds ftrace_lib.c
(which defines TST_NO_DEFAULT_MAIN and has no main()), but the
old Makefile's auto-detection tries to build it as a standalone
binary and the link fails. Patches 2-4 likewise cannot link
without the ftrace_lib.o dependency.

Each patch in the series must compile on its own. The Makefile
changes should be moved earlier, or folded into the respective
patches that add the C files.

Verdict: Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp