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

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.

Thanks,
Namhyung