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

[linuxtestproject.agent@gmail.com]

Hi Praveen,

On Sat, 06 Jun 2026, Praveen K Pandey wrote:
> ftrace: Add common library for C implementation

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

> +#include "tst_safe_macros.h"
> +#include "tst_safe_stdio.h"

ftrace_lib.h does not include tst_test.h. The SAFE_* macros expand to
code that calls tst_brk_(), which is declared only in tst_test.h.
Without it, ftrace_lib.c and every test file that includes ftrace_lib.h
will fail to compile with an implicit declaration error for tst_brk_().

Add #include "tst_test.h" to ftrace_lib.h.

> +       debugfs_path = tst_tmpdir_mkpath("debugfs");

tst_tmpdir_mkpath() does not exist in LTP. A grep of the entire tree
finds no declaration or definition outside this file. This will produce
a linker error. The intended API is likely tst_get_tmpdir() combined
with a path construction step.

> +// Made with Bob

This line at the end of ftrace_lib.h looks like a leftover development
artifact. It must be removed before the patch is merged.

> +    Fixes: #1282

The Fixes: tag must reference a valid git commit hash (e.g.
"Fixes: abc1234 (tracing: ...)"). A GitHub issue number is not valid
here. The same issue applies to all four commits that carry this tag
(patches 1-4).

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

> + * [Description]

The [Description] header in the doc comment is deprecated. Remove it
and let the description text follow the opening /*\ line directly.
See rule 18 in the coding style checklist.

> +	/* Small delay to let events trigger */
> +	usleep(10000);

This is sleep-based synchronization (waiting for kernel trace events to
land in the buffer), which violates Ground Rule 2. The correct approach
is to poll the trace file until the expected content appears, using an
exponential-backoff loop.

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

> +	/* Small delay to ensure trace is written */
> +	usleep(100000);

Same issue as in patch 2: sleep used to synchronize with the kernel
writing trace data. Replace with polling on the trace content.

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

> +ftrace_regression01:	ftrace_lib.o
> +ftrace_regression02:	ftrace_lib.o
> +ftrace_stress_test:	ftrace_lib.o
> +
> +LDLIBS			+= -lpthread
> +
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk

Three issues with the Makefile:

1. The shared helper object rules must appear AFTER the
   generic_leaf_target.mk include, not before. MAKE_TARGETS is not
   populated until that include is processed. Move the three
   dependency lines after the include.

2. ftrace_lib.c is in the directory so the build framework will try to
   build a standalone ftrace_lib binary. Add:

     FILTER_OUT_MAKE_TARGETS := ftrace_lib

   before the generic_leaf_target.mk include.

3. Only ftrace_stress_test.c uses pthreads. The global assignment
   links -lpthread into ftrace_regression01 and ftrace_regression02
   unnecessarily. Change to a per-target assignment:

     ftrace_stress_test: LDLIBS += -lpthread

The new C binaries (ftrace_regression01, ftrace_regression02,
ftrace_stress_test) are not added to runtest/tracing. The existing
entries still point to the .sh versions, so the new C tests are built
but never executed by the LTP runner. Update runtest/tracing to include
entries for the new binaries.

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

The commit is missing a Signed-off-by: tag, which is mandatory.

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