Re: [RFC PATCH v3 4/5] perf data: Add --to-trace-dat option for converting perf.data tracepoint events into trace.dat format

Thank you for your contribution! Sashiko AI review found 4 potential issue(=
s) to consider:
- [Medium] Memory leak of page_records and page_rec_sizes arrays when exten=
d allocation fails.
- [Medium] The --to-trace-dat conversion silently ignores the --time filter=
ing option.
- [Low] Missing explicit POSIX header inclusions in trace-dat.h and data-co=
nvert-trace.c causes build failures on musl libc.
- [Low] Missing dummy inline stub for trace_convert__perf2dat when HAVE_LIB=
TRACEEVENT is absent.
--

--- Patch [4]: [RFC PATCH v3 4/5] perf data: Add --to-trace-dat option for =
converting perf.data tracepoint events into trace.dat format ---
commit 3f0a7b06f263871cd1c6bbd15fd1c2bd5d454bae
Author: Tanushree Shah <[email protected]>
perf data: Add --to-trace-dat option for converting perf.data tracepoint ev=
ents into trace.dat format

This commit adds a new command-line option to perf data convert for generat=
ing
trace.dat output files. It provides a trace_convert__perf2dat function to
handle tracepoint events and write them in a format compatible with trace-c=
md
and KernelShark.

> 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
> +/*
> + * Copyright 2026, IBM Corporation
> + * Author: Tanushree Shah <[email protected]>
> + *
> + * data-convert-trace.c
> + *
> + * Implements perf.data to trace.dat format conversion for tracepoint ev=
ents.
> + */
> +
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <linux/compiler.h>
> +#include <linux/err.h>
> +
> +#include "data-convert.h"

[ ... ]

> +/* Convert perf.data tracepoint events to trace.dat format */
> +int trace_convert__perf2dat(const char *input, const char *to_trace,
> +			   struct perf_data_convert_opts *opts)
> +{
> +	struct perf_session *session;

[Severity: Medium]
Does this function ignore the opts->time_str filtering option? In other for=
mat
conversions, the time parameter is parsed to filter samples within the spec=
ified
time ranges, but it appears to be unused here when converting to trace.dat.

> +	struct trace_convert tc =3D {
> +		.events_count =3D 0,
> +	};

[ ... ]

> +	if (!opts->force) {
> +		int fd =3D open(to_trace, O_WRONLY | O_CREAT | O_EXCL, 0644);
> +
> +		if (fd < 0) {
> +			if (errno =3D=3D EEXIST)
> +				pr_err("Output file '%s' already exists. Use --force to overwrite.\n=
",
> +				       to_trace);
> +			else
> +				pr_err("Failed to open output file '%s': %s\n",
> +				       to_trace, strerror(errno));
> +			return -errno;
> +		}
> +		trace_dat_fp =3D fdopen(fd, "wb");

[Severity: Low]
Should <stdio.h> be explicitly included at the top of this file? Standard I=
/O
functions like fdopen() and fopen() are used here, which could cause build
failures on musl libc without the direct header inclusion.

> 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: Low]
Is a dummy inline stub needed here for when HAVE_LIBTRACEEVENT is disabled?
Without an #else block providing a fallback stub, it could cause future bui=
ld
failures if new callers are added that aren't guarded by the feature macro.

> diff --git a/tools/perf/util/trace-dat.c b/tools/perf/util/trace-dat.c
> new file mode 100644
> index 0000000000000..5598496c9a61c
> --- /dev/null
> +++ b/tools/perf/util/trace-dat.c

[ ... ]

> +static int trace_dat__write_cpu_dat(FILE *fp, struct tep_handle *pevent,
> +		int cpu, unsigned long long *file_offset_out)
> +{
> +	struct cpu_events *cpu_events =3D &trace_cpu_data[cpu];

[ ... ]

> +	for (i =3D 0; i < cpu_events->count; i++) {
> +		struct cpu_event *event =3D &cpu_events->events[i];

[ ... ]

> +		/* Emit TIME_EXTEND when delta does not fit in 27 bits */
> +		if (time_delta > TRACE_DAT_RECORD_TIME_MASK) {
> +			unsigned int extend_hdr;
> +			unsigned int delta_upper;
> +
> +			extend_size =3D TRACE_DAT_RECORD_TIME_EXTEND_SIZE;
> +			extend =3D calloc(1, extend_size);
> +			if (!extend)
> +				return -ENOMEM;

[Severity: Medium]
Can this early return cause a memory leak? If the allocation for extend fai=
ls,
returning -ENOMEM directly skips the out_free label at the end of the funct=
ion,
which is responsible for freeing the dynamically resized page_records and
page_rec_sizes arrays.

> diff --git a/tools/perf/util/trace-dat.h b/tools/perf/util/trace-dat.h
> new file mode 100644
> index 0000000000000..b63d4e8c1e2d7
> --- /dev/null
> +++ b/tools/perf/util/trace-dat.h
> @@ -0,0 +1,113 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

[ ... ]

> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <event-parse.h>
> +#include <byteswap.h>
> +#include "util.h"
> +
> +/* trace.dat file format version */
> +#define TRACE_DAT_VERSION "7"

[ ... ]

> +static inline uint16_t to_file_u16(struct tep_handle *pevent, uint16_t v=
al)
> +{
> +	return tep_read_number(pevent, &val, 2);
> +}

[Severity: Low]
Does this file need to explicitly include <stdint.h> to ensure compatibility
when compiling with musl libc? Fixed-width integer types like uint16_t are =
used
here but the standard POSIX header for them is missing.

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