Re: [PATCH v3] perf data: Clean up use_stdio and structures

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] The newly introduced `perf_data_file__seek` API silently truncates 64-bit file offsets on 32-bit platforms.
- [Medium] Missed API conversion in `record__finish_output` bypasses the new stdio abstraction.
--

commit f45ea505995a4b5ea83270dc73763efa7e6f535d
Author: Ian Rogers <irogers@google.com>

perf data: Clean up use_stdio and structures

This commit reorganizes perf_data_file to better abstract use_stdio,
adding new accessors like perf_data_file__seek() to ensure stdio buffering
is respected when a file pointer is used instead of a file descriptor.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 60d7640683024..88c2e4ea5c2d7 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
[ ... ]
> @@ -1847,8 +1847,10 @@ record__finish_output(struct record *rec)
>  	rec->session->header.data_size += rec->bytes_written;
>  	data->file.size = lseek(perf_data__fd(data), 0, SEEK_CUR);

Should this lseek() also be converted to use perf_data__seek()?

If use_stdio is enabled here, perf_data__fd() will extract the raw file
descriptor and lseek() will bypass the stdio buffering. This seems to
miss the abstraction introduced in this patch.

>  	if (record__threads_enabled(rec)) {
> -		for (i = 0; i < data->dir.nr; i++)
> -			data->dir.files[i].size = lseek(data->dir.files[i].fd, 0, SEEK_CUR);
> +		for (i = 0; i < data->dir.nr; i++) {
> +			data->dir.files[i].size =
> +				perf_data_file__seek(&data->dir.files[i], 0, SEEK_CUR);
> +		}
>  	}

[ ... ]

> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index 90df41da1a32b..a1914d71e3331 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
[ ... ]
>  ssize_t perf_data__write(struct perf_data *data,
>  			 void *buf, size_t size)
>  {
> -	if (data->use_stdio) {
> -		if (fwrite(buf, size, 1, data->file.fptr) == 1)
> -			return size;
> -		return -1;
> -	}
>  	return perf_data_file__write(&data->file, buf, size);
>  }
>  
> +off_t perf_data_file__seek(struct perf_data_file *file, off_t offset, int whence)
> +{
> +	if (file->use_stdio) {
> +		off_t res = fseek(file->fptr, offset, whence);
> +
> +		return res < 0 ? -1 : ftell(file->fptr);

Does this silently truncate 64-bit file offsets on 32-bit platforms?

Both fseek() and ftell() operate on the long data type, which is 32 bits on
32-bit architectures. Since the offset parameter and return type are off_t
(which is 64-bit when _FILE_OFFSET_BITS=64 is used), shouldn't fseeko() and
ftello() be used here instead to correctly support large files?

> +	}
> +	return lseek(file->fd, offset, whence);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260408064721.138338-1-irogers@google.com?part=1