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

[Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>]

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@kernel.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