Re: [RFC PATCH v4 1/5] perf/trace-dat: Add trace.dat export infrastructure

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Large trace events encode their payload length incorrectly, causing alignment errors and event truncation.
- [Medium] Querying the local host system's `tracefs` for the `trace_clock` during file conversion leaks host environment state into the converted trace.
--

--- Patch [1]: [RFC PATCH v4 1/5] perf/trace-dat: Add trace.dat export infrastructure ---
commit 162f3240dcf1a2672aa00440674197558e8f2cec
Author: Tanushree Shah <[email protected]>

perf/trace-dat: Add trace.dat export infrastructure

Add new utility files util/trace-dat.c and util/trace-dat.h implementing
the infrastructure for exporting perf.data tracepoints to trace.dat format
compatible with trace-cmd and KernelShark.

> diff --git a/tools/perf/util/trace-dat.c b/tools/perf/util/trace-dat.c
> index 0000000000000..55a7bd982c6f8
> --- /dev/null
> +++ b/tools/perf/util/trace-dat.c
[ ... ]
> +static int trace_dat__write_cpu_dat(FILE *fp, int cpu, unsigned long long *file_offset_out)
> +{
[ ... ]
> +		memcpy(data_rec, &hdr_word, TRACE_DAT_WORD_SIZE);
> +
> +		/* Large events: write actual byte length after header */
> +		if (type_len == 0)
> +			memcpy(data_rec + TRACE_DAT_WORD_SIZE, &data_len, TRACE_DAT_WORD_SIZE);

[Severity: Medium]
When writing large events (where type_len == 0), is `data_len` the correct
value to write here? Ftrace ring buffer parsers typically expect this field
to contain the size of the payload plus the 4-byte size field itself,
properly padded to 4-byte alignment, rather than just the raw event size.
Could this lead to event truncation or alignment errors when parsing?

[ ... ]
> +int trace_dat__write_options_section1(void)
> +{
[ ... ]
> +	/* TRACECLOCK option */
> +	opt_id = TRACE_DAT_OPTION_TRACECLOCK;
> +
> +	path = get_tracing_file("trace_clock");
> +	if (path) {
> +		clock_file = fopen(path, "r");
> +		put_tracing_file(path);
> +	} else {
> +		clock_file = NULL;
> +	}

[Severity: Medium]
Does querying the host system's tracefs mount here break cross-platform
analysis? Since this is an offline conversion of a perf.data file, the host
machine's current trace clock might not match the clock used when the trace
was originally recorded. Should this extract the clock from the perf_env
instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1