Re: [PATCH 3/5] perf dso: Use stored fd error instead of stale errno in file_read() and file_size()

On Thu, Aug 13, 2026 at 11:25:54AM +0900, Namhyung Kim wrote:
> On Wed, Aug 12, 2026 at 04:12:52PM -0700, Ian Rogers wrote:
> > On Tue, Aug 11, 2026 at 10:52 AM Arnaldo Carvalho de Melo
> > <[email protected]> wrote:
> > >
> > > From: Arnaldo Carvalho de Melo <[email protected]>
> > >
> > > file_read() and file_size() use ret = -errno when
> > > dso__data(dso)->fd is negative after try_to_open_dso() fails.  By this
> > > point errno has been through mutex_lock(), nsinfo__mountns_enter(), and
> > > multiple open() attempts inside try_to_open_dso() — it no longer
> > > reflects the actual open failure.  If errno happens to be 0, ret = 0
> > > looks like EOF rather than an error, and file_size() callers like
> > > dso__data_size() would then report a zero-sized file instead of
> > > failing.
> > >
> > > dso__data(dso)->fd is always negative on failure — -errno from
> > > __open_dso() when no filename could be built (e.g. -EINVAL, -ENOENT),
> > > or -1 when do_open() itself failed — and never 0, so use it directly
> > > instead of reading the stale global errno.
> > >
> > > Fixes: 33bdedcea2d7 ("perf tools: Protect dso cache fd with a mutex")
> > > Reported-by: sashiko-bot <[email protected]>
> > > Cc: Namhyung Kim <[email protected]>
> > > Assisted-by: Claude:claude-opus-4.6
> > > Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> > > ---
> > >  tools/perf/util/dso.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> > > index 124193453675ca18..32ae5c78cdb906e6 100644
> > > --- a/tools/perf/util/dso.c
> > > +++ b/tools/perf/util/dso.c
> > > @@ -1029,7 +1029,8 @@ static ssize_t file_read(struct dso *dso, struct machine *machine,
> > >
> > >         if (dso__data(dso)->fd < 0) {
> > >                 dso__data(dso)->status = DSO_DATA_STATUS_ERROR;
> > > -               ret = -errno;
> > > +               /* fd is always negative here: -errno or -1, never 0 */
> > > +               ret = dso__data(dso)->fd;
> > 
> > nit: we can "assert(ret < 0);" to verify the comment.
> 
> It's already checked in the above if statement.  I'm not sure the
> comment or assert is necessary.

You're right, I'm dropping both the comment and the assert, my bad.

- Arnaldo