Re: [RFC PATCH v3 3/5] perf data-convert: Add perf.data to trace.dat conversion backend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing inline fallback stub for `trace_convert__perf2dat` when `HAVE_LIBTRACEEVENT` is undefined.
- [Medium] Missing POSIX header `<stdio.h>` for file I/O operations.
- [Medium] Resource leak: orphaned file left on the filesystem if `fdopen()` fails.
--

--- Patch [3]: [RFC PATCH v3 3/5] perf data-convert: Add perf.data to trace.dat conversion backend ---
commit 25ab95b73fc4726a00bca4e0429ad24d8f04129d
Author: Tanushree Shah <tshah@linux.ibm.com>

perf data-convert: Add perf.data to trace.dat conversion backend

Add data-convert-trace.c implementing trace_convert__perf2dat() to
convert perf.data tracepoint events to trace.dat format.

> diff --git a/tools/perf/util/data-convert-trace.c b/tools/perf/util/data-convert-trace.c
> new file mode 100644
> index 0000000000000..8dbdb2c9caa4f
> --- /dev/null
> +++ b/tools/perf/util/data-convert-trace.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
[ ... ]
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>

[Severity: Medium]
Does this file also need to include <stdio.h>? 

The code heavily relies on FILE pointers and stdio routines like fopen(), 
fdopen(), and fclose(). Without the explicit include, builds might fail on 
musl libc due to relying on implicit inclusions.

> +#include <linux/compiler.h>
> +#include <linux/err.h>
[ ... ]
> +int trace_convert__perf2dat(const char *input, const char *to_trace,
> +			   struct perf_data_convert_opts *opts)
> +{
[ ... ]
> +	if (!opts->force) {
> +		int fd = open(to_trace, O_WRONLY | O_CREAT | O_EXCL, 0644);
> +
> +		if (fd < 0) {
[ ... ]
> +			return -errno;
> +		}
> +		trace_dat_fp = fdopen(fd, "wb");
> +		if (!trace_dat_fp) {
> +			int err = errno;
> +
> +			close(fd);
> +			pr_err("Failed to open output file '%s': %s\n",
> +			       to_trace, strerror(err));
> +			return -err;

[Severity: Medium]
Can this error path leave an orphaned file on the filesystem? 

By returning directly instead of jumping to the out_close label where
unlink(to_trace) is called, the newly created 0-byte file isn't cleaned up. 
This might break subsequent runs without --force since O_EXCL would fail 
with EEXIST.

> +		}
[ ... ]
> diff --git a/tools/perf/util/data-convert.h b/tools/perf/util/data-convert.h
> index a96240f156712..f041c23252263 100644
> --- a/tools/perf/util/data-convert.h
> +++ b/tools/perf/util/data-convert.h
> @@ -19,4 +19,8 @@ int bt_convert__perf2ctf(const char *input_name, const char *to_ctf,
>  int bt_convert__perf2json(const char *input_name, const char *to_ctf,
>  			 struct perf_data_convert_opts *opts);
>  
> +#ifdef HAVE_LIBTRACEEVENT
> +int trace_convert__perf2dat(const char *input, const char *to_trace,
> +			   struct perf_data_convert_opts *opts);
> +#endif /* HAVE_LIBTRACEEVENT */

[Severity: Medium]
Should this declaration include a corresponding #else block with a dummy
inline fallback? 

When libtraceevent is disabled, omitting the stub could cause compilation 
failures if callers do not wrap their usage in #ifdef blocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803145958.299956-1-tshah@linux.ibm.com?part=3