2010-01-07 16:40:58

by Alexander Beregalov

[permalink] [raw]
Subject: [PATCH 1/2] perf: add close()/closedir() calls when needed

Close file and directory descriptors before exiting or dying.

Signed-off-by: Alexander Beregalov <[email protected]>
---
tools/perf/builtin-kmem.c | 2 ++
tools/perf/builtin-trace.c | 6 +++++-
tools/perf/util/parse-events.c | 1 +
tools/perf/util/symbol.c | 1 +
tools/perf/util/trace-event-info.c | 20 +++++++++++++++++---
5 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index 7ceb741..81735d2 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -112,7 +112,9 @@ static void setup_cpunode_map(void)
continue;
cpunode_map[cpu] = mem;
}
+ closedir(dir2);
}
+ closedir(dir1);
}

static void insert_alloc_stat(unsigned long call_site, unsigned long ptr,
diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 574a215..5ad93e2 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -507,7 +507,9 @@ static char *get_script_path(const char *script_root, const char *suffix)
}
free(__script_root);
}
+ closedir(lang_dir);
}
+ closedir(scripts_dir);

return path;
}
@@ -615,7 +617,8 @@ int cmd_trace(int argc, const char **argv, const char *prefix __used)
scripting_ops = script_spec__lookup(generate_script_lang);
if (!scripting_ops) {
fprintf(stderr, "invalid language specifier");
- return -1;
+ err = -1;
+ goto out;
}

perf_header__read(&session->header, input);
@@ -634,5 +637,6 @@ int cmd_trace(int argc, const char **argv, const char *prefix __used)
perf_session__delete(session);
cleanup_scripting();
out:
+ close(input);
return err;
}
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index e5bc0fb..7739a37 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -483,6 +483,7 @@ parse_subsystem_tracepoint_event(char *sys_name, char *flags)
if (parse_events(NULL, event_opt, 0))
return EVT_FAILED;
}
+ closedir(evt_dir);

return EVT_HANDLED_ALL;
}
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index ab92763..953e654 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1364,6 +1364,7 @@ static int perf_session__set_modules_path_dir(struct perf_session *self, char *d
}
}

+ closedir(dir);
return 0;
failure:
closedir(dir);
diff --git a/tools/perf/util/trace-event-info.c b/tools/perf/util/trace-event-info.c
index cace355..e191126 100644
--- a/tools/perf/util/trace-event-info.c
+++ b/tools/perf/util/trace-event-info.c
@@ -275,6 +275,7 @@ static void read_header_files(void)
die("wrong size for '%s' size=%lld read=%lld",
path, size, check_size);
put_tracing_file(path);
+ close(fd);

path = get_tracing_file("events/header_event");
fd = open(path, O_RDONLY);
@@ -289,6 +290,7 @@ static void read_header_files(void)
if (size != check_size)
die("wrong size for '%s'", path);
put_tracing_file(path);
+ close(fd);
}

static bool name_in_tp_list(char *sys, struct tracepoint_path *tps)
@@ -321,7 +323,11 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
strcmp(dent->d_name, "..") == 0 ||
!name_in_tp_list(dent->d_name, tps))
continue;
- format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
+ format = malloc(strlen(sys) + strlen(dent->d_name) + 10);
+ if (!format) {
+ closedir(dir);
+ die("malloc");
+ }
sprintf(format, "%s/%s/format", sys, dent->d_name);
ret = stat(format, &st);
free(format);
@@ -338,7 +344,11 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
strcmp(dent->d_name, "..") == 0 ||
!name_in_tp_list(dent->d_name, tps))
continue;
- format = malloc_or_die(strlen(sys) + strlen(dent->d_name) + 10);
+ format = malloc(strlen(sys) + strlen(dent->d_name) + 10);
+ if (!format) {
+ closedir(dir);
+ die("malloc");
+ }
sprintf(format, "%s/%s/format", sys, dent->d_name);
ret = stat(format, &st);

@@ -347,12 +357,15 @@ static void copy_event_system(const char *sys, struct tracepoint_path *tps)
size = get_size(format);
write_or_die(&size, 8);
check_size = copy_file(format);
- if (size != check_size)
+ if (size != check_size) {
+ closedir(dir);
die("error in size of file '%s'", format);
+ }
}

free(format);
}
+ closedir(dir);
}

static void read_ftrace_files(struct tracepoint_path *tps)
@@ -429,6 +442,7 @@ static void read_event_files(struct tracepoint_path *tps)
}
free(sys);
}
+ closedir(dir);

put_tracing_file(path);
}
--
1.6.6


2010-01-07 16:41:07

by Alexander Beregalov

[permalink] [raw]
Subject: [PATCH 2/2] perf: fix memory leak: counterwidth

Signed-off-by: Alexander Beregalov <[email protected]>
---
tools/perf/util/values.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/values.c b/tools/perf/util/values.c
index 1c15e39..cfa55d6 100644
--- a/tools/perf/util/values.c
+++ b/tools/perf/util/values.c
@@ -169,6 +169,7 @@ static void perf_read_values__display_pretty(FILE *fp,
counterwidth[j], values->value[i][j]);
fprintf(fp, "\n");
}
+ free(counterwidth);
}

static void perf_read_values__display_raw(FILE *fp,
--
1.6.6

2010-01-07 21:29:22

by Paul Mackerras

[permalink] [raw]
Subject: Re: [PATCH 1/2] perf: add close()/closedir() calls when needed

On Thu, Jan 07, 2010 at 07:40:46PM +0300, Alexander Beregalov wrote:

> Close file and directory descriptors before exiting or dying.

Your patch description doesn't say why that's a good thing to do, and
it's not obvious to me at least. Please explain the motivation in
your patch description.

Paul.

2010-01-07 22:45:07

by Alexander Beregalov

[permalink] [raw]
Subject: Re: [PATCH 1/2] perf: add close()/closedir() calls when needed

2010/1/8 Paul Mackerras <[email protected]>:
> On Thu, Jan 07, 2010 at 07:40:46PM +0300, Alexander Beregalov wrote:
>
>> Close file and directory descriptors before exiting or dying.
>
> Your patch description doesn't say why that's a good thing to do, and
> it's not obvious to me at least.  Please explain the motivation in
> your patch description.

Isn't it a good style to close files after use and before exit?

2010-01-07 23:54:47

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH 1/2] perf: add close()/closedir() calls when needed

On 01/07/2010 02:44 PM, Alexander Beregalov wrote:
> 2010/1/8 Paul Mackerras <[email protected]>:
>> On Thu, Jan 07, 2010 at 07:40:46PM +0300, Alexander Beregalov wrote:
>>
>>> Close file and directory descriptors before exiting or dying.
>>
>> Your patch description doesn't say why that's a good thing to do, and
>> it's not obvious to me at least. Please explain the motivation in
>> your patch description.
>
> Isn't it a good style to close files after use and before exit?

exit() closes all files, so it's redundant.

It's "good style" in the sense that it can help librarization, though.

-hpa

2010-01-13 10:36:38

by Alexander Beregalov

[permalink] [raw]
Subject: [tip:perf/core] perf: Fix memory leak: counterwidth

Commit-ID: 8d9e503928638fc95317be42c416fb7907322aff
Gitweb: http://git.kernel.org/tip/8d9e503928638fc95317be42c416fb7907322aff
Author: Alexander Beregalov <[email protected]>
AuthorDate: Thu, 7 Jan 2010 19:40:47 +0300
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 13 Jan 2010 10:09:15 +0100

perf: Fix memory leak: counterwidth

Signed-off-by: Alexander Beregalov <[email protected]>
Cc: [email protected]
Cc: [email protected]
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
tools/perf/util/values.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/tools/perf/util/values.c b/tools/perf/util/values.c
index 1c15e39..cfa55d6 100644
--- a/tools/perf/util/values.c
+++ b/tools/perf/util/values.c
@@ -169,6 +169,7 @@ static void perf_read_values__display_pretty(FILE *fp,
counterwidth[j], values->value[i][j]);
fprintf(fp, "\n");
}
+ free(counterwidth);
}

static void perf_read_values__display_raw(FILE *fp,