[PATCH core] perf data: Adding error message if perf_data__create_dir fails

[PATCH core] perf data: Adding error message if perf_data__create_dir fails

[Alexey Bayduraev]

There is no notification about data directory creation failure. Add it.

Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
---
 tools/perf/builtin-record.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 0bc6529814b2..0306d5911de2 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec,
 
 	if (record__threads_enabled(rec)) {
 		ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps);
-		if (ret)
+		if (ret) {
+			pr_err("Failed to create data directory: %s\n", strerror(errno));
 			return ret;
+		}
 		for (i = 0; i < evlist->core.nr_mmaps; i++) {
 			if (evlist->mmap)
 				evlist->mmap[i].file = &rec->data.dir.files[i];
-- 
2.19.0

[PATCH urgent] perf data: Fix double free in perf_session__delete

[Alexey Bayduraev]

When perf_data__create_dir fails, it calls close_dir, but
perf_session__delete also calls close_dir and since dir.version and
dir.nr was initialized by perf_data__create_dir, a double free occurs.
This patch moves the initialization of dir.version and dir.nr after
successful initialization of dir.files, that prevents double freeing.
This behavior is already implemented in perf_data__open_dir.

Fixes: 145520631130bd64 ("perf data: Add perf_data__(create_dir|close_dir) functions")
Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
---
 tools/perf/util/data.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index f5d260b1df4d..15a4547d608e 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -44,10 +44,6 @@ int perf_data__create_dir(struct perf_data *data, int nr)
 	if (!files)
 		return -ENOMEM;
 
-	data->dir.version = PERF_DIR_VERSION;
-	data->dir.files   = files;
-	data->dir.nr      = nr;
-
 	for (i = 0; i < nr; i++) {
 		struct perf_data_file *file = &files[i];
 
@@ -62,6 +58,9 @@ int perf_data__create_dir(struct perf_data *data, int nr)
 		file->fd = ret;
 	}
 
+	data->dir.version = PERF_DIR_VERSION;
+	data->dir.files   = files;
+	data->dir.nr      = nr;
 	return 0;
 
 out_err:
-- 
2.19.0

Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails

[Jiri Olsa]

On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote:
> There is no notification about data directory creation failure. Add it.
> 
> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
> ---
>  tools/perf/builtin-record.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 0bc6529814b2..0306d5911de2 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec,
>  
>  	if (record__threads_enabled(rec)) {
>  		ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps);
> -		if (ret)
> +		if (ret) {
> +			pr_err("Failed to create data directory: %s\n", strerror(errno));

errno will be misleading in here, because perf_data__create_dir
calls other syscalls on error path

jirka

>  			return ret;
> +		}
>  		for (i = 0; i < evlist->core.nr_mmaps; i++) {
>  			if (evlist->mmap)
>  				evlist->mmap[i].file = &rec->data.dir.files[i];
> -- 
> 2.19.0
> 

Re: [PATCH urgent] perf data: Fix double free in perf_session__delete

[Jiri Olsa]

On Fri, Feb 18, 2022 at 06:23:41PM +0300, Alexey Bayduraev wrote:
> When perf_data__create_dir fails, it calls close_dir, but
> perf_session__delete also calls close_dir and since dir.version and
> dir.nr was initialized by perf_data__create_dir, a double free occurs.
> This patch moves the initialization of dir.version and dir.nr after
> successful initialization of dir.files, that prevents double freeing.
> This behavior is already implemented in perf_data__open_dir.
> 
> Fixes: 145520631130bd64 ("perf data: Add perf_data__(create_dir|close_dir) functions")
> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/util/data.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index f5d260b1df4d..15a4547d608e 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
> @@ -44,10 +44,6 @@ int perf_data__create_dir(struct perf_data *data, int nr)
>  	if (!files)
>  		return -ENOMEM;
>  
> -	data->dir.version = PERF_DIR_VERSION;
> -	data->dir.files   = files;
> -	data->dir.nr      = nr;
> -
>  	for (i = 0; i < nr; i++) {
>  		struct perf_data_file *file = &files[i];
>  
> @@ -62,6 +58,9 @@ int perf_data__create_dir(struct perf_data *data, int nr)
>  		file->fd = ret;
>  	}
>  
> +	data->dir.version = PERF_DIR_VERSION;
> +	data->dir.files   = files;
> +	data->dir.nr      = nr;
>  	return 0;
>  
>  out_err:
> -- 
> 2.19.0
> 

Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails

[Bayduraev, Alexey V]

On 21.02.2022 1:43, Jiri Olsa wrote:
> On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote:
>> There is no notification about data directory creation failure. Add it.
>>
>> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
>> ---
>>  tools/perf/builtin-record.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index 0bc6529814b2..0306d5911de2 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec,
>>  
>>  	if (record__threads_enabled(rec)) {
>>  		ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps);
>> -		if (ret)
>> +		if (ret) {
>> +			pr_err("Failed to create data directory: %s\n", strerror(errno));
> 
> errno will be misleading in here, because perf_data__create_dir
> calls other syscalls on error path

Mostly I want to output something like:

  Failed to create data dir: Too many open files

This will trigger the user to increase the open files limit.
Would it be better to place such message to perf_data__create_dir after 
open() syscall?

Regards,
Alexey

> 
> jirka
> 
>>  			return ret;
>> +		}
>>  		for (i = 0; i < evlist->core.nr_mmaps; i++) {
>>  			if (evlist->mmap)
>>  				evlist->mmap[i].file = &rec->data.dir.files[i];
>> -- 
>> 2.19.0
>>

Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails

[Jiri Olsa]

On Mon, Feb 21, 2022 at 04:24:28PM +0300, Bayduraev, Alexey V wrote:
> On 21.02.2022 1:43, Jiri Olsa wrote:
> > On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote:
> >> There is no notification about data directory creation failure. Add it.
> >>
> >> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
> >> ---
> >>  tools/perf/builtin-record.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> >> index 0bc6529814b2..0306d5911de2 100644
> >> --- a/tools/perf/builtin-record.c
> >> +++ b/tools/perf/builtin-record.c
> >> @@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec,
> >>  
> >>  	if (record__threads_enabled(rec)) {
> >>  		ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps);
> >> -		if (ret)
> >> +		if (ret) {
> >> +			pr_err("Failed to create data directory: %s\n", strerror(errno));
> > 
> > errno will be misleading in here, because perf_data__create_dir
> > calls other syscalls on error path
> 
> Mostly I want to output something like:
> 
>   Failed to create data dir: Too many open files
> 
> This will trigger the user to increase the open files limit.
> Would it be better to place such message to perf_data__create_dir after 
> open() syscall?

how about something like below (with your change)

jirka


---
diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 15a4547d608e..d3382098d6f9 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -52,8 +52,10 @@ int perf_data__create_dir(struct perf_data *data, int nr)
 			goto out_err;
 
 		ret = open(file->path, O_RDWR|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR);
-		if (ret < 0)
+		if (ret < 0) {
+			ret = -errno;
 			goto out_err;
+		}
 
 		file->fd = ret;
 	}

Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails

[Bayduraev, Alexey V]

On 21.02.2022 21:24, Jiri Olsa wrote:
> On Mon, Feb 21, 2022 at 04:24:28PM +0300, Bayduraev, Alexey V wrote:
>> On 21.02.2022 1:43, Jiri Olsa wrote:
>>> On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote:
>>>> There is no notification about data directory creation failure. Add it.
>>>>
>>>> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
>>>> ---
>>>>  tools/perf/builtin-record.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>>>> index 0bc6529814b2..0306d5911de2 100644
>>>> --- a/tools/perf/builtin-record.c
>>>> +++ b/tools/perf/builtin-record.c
>>>> @@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec,
>>>>  
>>>>  	if (record__threads_enabled(rec)) {
>>>>  		ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps);
>>>> -		if (ret)
>>>> +		if (ret) {
>>>> +			pr_err("Failed to create data directory: %s\n", strerror(errno));
>>>
>>> errno will be misleading in here, because perf_data__create_dir
>>> calls other syscalls on error path
>>
>> Mostly I want to output something like:
>>
>>   Failed to create data dir: Too many open files
>>
>> This will trigger the user to increase the open files limit.
>> Would it be better to place such message to perf_data__create_dir after 
>> open() syscall?
> 
> how about something like below (with your change)

Looks better, I will apply this to my patch.

Thanks,
Alexey

> 
> jirka
> 
> 
> ---
> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index 15a4547d608e..d3382098d6f9 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
> @@ -52,8 +52,10 @@ int perf_data__create_dir(struct perf_data *data, int nr)
>  			goto out_err;
>  
>  		ret = open(file->path, O_RDWR|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			ret = -errno;
>  			goto out_err;
> +		}
>  
>  		file->fd = ret;
>  	}

Re: [PATCH core] perf data: Adding error message if perf_data__create_dir fails

[Arnaldo Carvalho de Melo]

On February 21, 2022 3:45:55 PM GMT-03:00, "Bayduraev, Alexey V" <alexey.v.bayduraev@linux.intel.com> wrote:
>
>On 21.02.2022 21:24, Jiri Olsa wrote:
>> On Mon, Feb 21, 2022 at 04:24:28PM +0300, Bayduraev, Alexey V wrote:
>>> On 21.02.2022 1:43, Jiri Olsa wrote:
>>>> On Fri, Feb 18, 2022 at 06:23:40PM +0300, Alexey Bayduraev wrote:
>>>>> There is no notification about data directory creation failure. Add it.
>>>>>
>>>>> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
>>>>> ---
>>>>>  tools/perf/builtin-record.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>>>>> index 0bc6529814b2..0306d5911de2 100644
>>>>> --- a/tools/perf/builtin-record.c
>>>>> +++ b/tools/perf/builtin-record.c
>>>>> @@ -1186,8 +1186,10 @@ static int record__mmap_evlist(struct record *rec,
>>>>>  
>>>>>  	if (record__threads_enabled(rec)) {
>>>>>  		ret = perf_data__create_dir(&rec->data, evlist->core.nr_mmaps);
>>>>> -		if (ret)
>>>>> +		if (ret) {
>>>>> +			pr_err("Failed to create data directory: %s\n", strerror(errno));
>>>>
>>>> errno will be misleading in here, because perf_data__create_dir
>>>> calls other syscalls on error path
>>>
>>> Mostly I want to output something like:
>>>
>>>   Failed to create data dir: Too many open files
>>>
>>> This will trigger the user to increase the open files limit.
>>> Would it be better to place such message to perf_data__create_dir after 
>>> open() syscall?
>> 
>> how about something like below (with your change)
>
>Looks better, I will apply this to my patch.


Thanks a lot, I'll check when you post it.

Thanks for splitting that original patch, it was the correct thing to do in the first place, and now one of the patches got reviews I even missed :-)

- Arnaldo

>
>Thanks,
>Alexey
>
>> 
>> jirka
>> 
>> 
>> ---
>> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
>> index 15a4547d608e..d3382098d6f9 100644
>> --- a/tools/perf/util/data.c
>> +++ b/tools/perf/util/data.c
>> @@ -52,8 +52,10 @@ int perf_data__create_dir(struct perf_data *data, int nr)
>>  			goto out_err;
>>  
>>  		ret = open(file->path, O_RDWR|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR);
>> -		if (ret < 0)
>> +		if (ret < 0) {
>> +			ret = -errno;
>>  			goto out_err;
>> +		}
>>  
>>  		file->fd = ret;
>>  	}