Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 15 Jan 2021 16:48:25 -0300
From:   Arnaldo Carvalho de Melo <acme@kernel.org>
To:     Jiri Olsa <jolsa@redhat.com>
Cc:     Denis Nikitin <denik@chromium.org>,
        linux-perf-users@vger.kernel.org, peterz@infradead.org,
        mingo@redhat.com, mark.rutland@arm.com,
        alexander.shishkin@linux.intel.com, namhyung@kernel.org,
        al.grant@arm.com, al.grant@foss.arm.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf inject: correct event attribute sizes
Message-ID: <20210115194825.GL457607@kernel.org>
References: <20201124195818.30603-1-al.grant@arm.com>
 <20201216065816.1840893-1-denik@chromium.org>
 <20201223221105.GA236568@krava>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201223221105.GA236568@krava>
Precedence: bulk

Em Wed, Dec 23, 2020 at 11:11:05PM +0100, Jiri Olsa escreveu:
> On Tue, Dec 15, 2020 at 10:58:16PM -0800, Denis Nikitin wrote:
> > When perf inject reads a perf.data file from an older version of perf,
> > it writes event attributes into the output with the original size field,
> > but lays them out as if they had the size currently used. Readers see
> > a corrupt file. Update the size field to match the layout.
> > 
> > Signed-off-by: Al Grant <al.grant@foss.arm.com>
> > Signed-off-by: Denis Nikitin <denik@chromium.org>
> > ---
> >  tools/perf/util/header.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index be850e9f8852..0d95981df8dd 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -3331,6 +3331,14 @@ int perf_session__write_header(struct perf_session *session,
> >  	attr_offset = lseek(ff.fd, 0, SEEK_CUR);
> >  
> >  	evlist__for_each_entry(evlist, evsel) {
> > +		if (evsel->core.attr.size < sizeof(evsel->core.attr)) {
> > +			/*
> > +			 * We are likely in "perf inject" and have read
> > +			 * from an older file. Update attr size so that
> > +			 * reader gets the right offset to the ids.
> > +			 */
> > +			evsel->core.attr.size = sizeof(evsel->core.attr);
> > +		}
> 
> seems ok, just wondering if it would be better
> to fix it in perf_event__process_attr instead
> 
> where we know where the data is coming from,
> here we could cover for some unexpected case
> of having different attr.size?
> 
> anyway both would be probably equal, I'm ok
> with either way
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks, applied.

- Arnaldo