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

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 <[email protected]>

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>?=20

The code heavily relies on FILE pointers and stdio routines like fopen(),=20
fdopen(), and fclose(). Without the explicit include, builds might fail on=
=20
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 =3D open(to_trace, O_WRONLY | O_CREAT | O_EXCL, 0644);
> +
> +		if (fd < 0) {
[ ... ]
> +			return -errno;
> +		}
> +		trace_dat_fp =3D fdopen(fd, "wb");
> +		if (!trace_dat_fp) {
> +			int err =3D 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?=20

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.=
=20
This might break subsequent runs without --force since O_EXCL would fail=20
with EEXIST.

> +		}
[ ... ]
> diff --git a/tools/perf/util/data-convert.h b/tools/perf/util/data-conver=
t.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);
> =20
> +#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?=20

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

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803145958.2999=
[email protected]?part=3D3