There is no notification about data directory creation failure. Add it.
Signed-off-by: Alexey Bayduraev <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
Acked-by: Jiri Olsa <[email protected]>
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
>
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 <[email protected]>
>> ---
>> 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
>>
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 <[email protected]>
>>>> ---
>>>> 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;
> }
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 <[email protected]>
> >> ---
> >> 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;
}
On February 21, 2022 3:45:55 PM GMT-03:00, "Bayduraev, Alexey V" <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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;
>> }