linux-kernel - Re: [PATCH 10/20] perf data: Add directory support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190225150722.GQ31136@kernel.org>
Date:   Mon, 25 Feb 2019 12:07:22 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Stephane Eranian <eranian@...gle.com>,
        Alexey Budankov <alexey.budankov@...ux.intel.com>
Subject: Re: [PATCH 10/20] perf data: Add directory support

Em Mon, Feb 25, 2019 at 02:56:51PM +0100, Jiri Olsa escreveu:
> On Mon, Feb 25, 2019 at 10:45:48AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Feb 24, 2019 at 08:06:46PM +0100, Jiri Olsa escreveu:
> > > Adding support to have directory as perf.data.
> > > 
> > > The caller needs to set 'struct perf_data::is_dir flag
> > > and the path will be treated as directory.
> > > 
> > > The 'struct perf_data::file' is initialized and open
> > > as 'path/header' file.
> > > 
> > > Adding check to direcory interface functions to check
> > > on is_dir flag.
> > > 
> > > Link: http://lkml.kernel.org/n/tip-pvot1aywiem9epgqpfi1agaj@git.kernel.org
> > > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > > ---
> > >  tools/perf/util/data.c    | 41 ++++++++++++++++++++++++++++++++++++++-
> > >  tools/perf/util/data.h    |  6 ++++++
> > >  tools/perf/util/session.c |  4 ++++
> > >  3 files changed, 50 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> > > index 7bd5ddeb7a41..72ac4dbb5c69 100644
> > > --- a/tools/perf/util/data.c
> > > +++ b/tools/perf/util/data.c
> > > @@ -34,6 +34,9 @@ int perf_data__create_dir(struct perf_data *data, int nr)
> > >  	struct perf_data_file *files = NULL;
> > >  	int i, ret = -1;
> > >  
> > > +	if (WARN_ON(!data->is_dir))
> > > +		return -EINVAL;
> > > +
> > >  	files = zalloc(nr * sizeof(*files));
> > >  	if (!files)
> > >  		return -ENOMEM;
> > > @@ -69,6 +72,9 @@ int perf_data__open_dir(struct perf_data *data)
> > >  	DIR *dir;
> > >  	int nr = 0;
> > >  
> > > +	if (WARN_ON(!data->is_dir))
> > > +		return -EINVAL;
> > > +
> > >  	dir = opendir(data->path);
> > >  	if (!dir)
> > >  		return -EINVAL;
> > > @@ -173,6 +179,16 @@ static int check_backup(struct perf_data *data)
> > >  	return 0;
> > >  }
> > >  
> > > +static bool is_dir(struct perf_data *data)
> > > +{
> > > +	struct stat st;
> > > +
> > > +	if (stat(data->path, &st))
> > > +		return false;
> > > +
> > > +	return (st.st_mode & S_IFMT) == S_IFDIR;
> > > +}
> > > +
> > >  static int open_file_read(struct perf_data *data)
> > >  {
> > >  	struct stat st;
> > > @@ -254,6 +270,22 @@ static int open_file_dup(struct perf_data *data)
> > >  	return open_file(data);
> > >  }
> > >  
> > > +static int open_dir(struct perf_data *data)
> > > +{
> > > +	if (perf_data__is_write(data) &&
> > > +	    mkdir(data->path, S_IRWXU) < 0)
> > > +		return -1;
> > > +
> > > +	/*
> > > +	 * So far we open only the header, so we
> > > +	 * can read the data version and layout.
> > > +	 */
> > > +	if (asprintf(&data->file.path, "%s/header", data->path) < 0)
> > > +		return -ENOMEM;
> > 
> > so, if this fails, then we should unwind the mkdir, if it was
> > performed, so that we leave things as they were before calling
> > open_dir(), right?
> 
> I think we need to some global solution on this,

If we don't add more, that would be a good thing :-)

The other parts also need to be investigated to see what is best in that
case, but here, undoing the mkdir() if the asprintf() fails is the right
thing to do :-)

- Arnaldo

> currently we also leve halfway screwed perf.data
> if we fail in the middle
> 
> I think maybe we need to add something on upper (session)
> layer to cleanup if we screw up

B