Hi Zoran,
Colin King sent me some patches to fixup memory leaks and some nits in the error code path.
Please, pull these changes in your tree.
The following changes since commit da6a8c94a8f8124711db0ae84a3ef4e0e186b388:
Fixing improperly initialized cstate_max per-CPU. (2014-01-29 15:47:47 -0800)
are available in the git repository at:
git://git.linaro.org/people/daniel.lezcano/idlestat.git master
for you to fetch changes up to 41505eca08f1bd0f2d35733cd631be5967e38724:
Use the correct format specifier for size_t type (2014-02-18 08:51:30 +0100)
---------------------------------------------------------------- Colin Ian King (6): Write open failure message to stderr Free memory on realloc failure. Fix memory leak, result is being leaked on a realloc failure check s_cpu rather than s_core on memory allocation Fix file and memory resource leak on error exit in idlestat_load Use the correct format specifier for size_t type
idlestat.c | 36 ++++++++++++++++++++++++++---------- topology.c | 2 +- 2 files changed, 27 insertions(+), 11 deletions(-)
From: Colin Ian King colin.king@canonical.com
Error message should be written to stderr rather than the failed to open file.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/idlestat.c b/idlestat.c index 3abfaaf..fce6112 100644 --- a/idlestat.c +++ b/idlestat.c @@ -659,7 +659,7 @@ static int idlestat_file_for_each_line(const char *path, void *data, f = fopen(path, "r");
if (!f) { - fprintf(f, "failed to open '%s': %m\n", path); + fprintf(stderr, "failed to open '%s': %m\n", path); return -1; }
From: Colin Ian King colin.king@canonical.com
Fix a common coding error where realloc fails and the original allocated data is not free'd.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/idlestat.c b/idlestat.c index fce6112..41ed3ab 100644 --- a/idlestat.c +++ b/idlestat.c @@ -188,6 +188,7 @@ static struct cpuidle_cstate *inter(struct cpuidle_cstate *c1, for (i = 0, index = 0; i < c1->nrdata; i++) {
for (j = index; j < c2->nrdata; j++) { + struct cpuidle_data *tmp;
/* intervals are ordered, no need to go further */ if (c1->data[i].end < c2->data[j].begin) @@ -216,10 +217,13 @@ static struct cpuidle_cstate *inter(struct cpuidle_cstate *c1,
result->nrdata++;
- data = realloc(data, sizeof(*data) * + tmp = realloc(data, sizeof(*data) * (result->nrdata + 1)); - if (!data) + if (!tmp) { + free(data); return NULL; + } + data = tmp;
result->data = data; result->data[result->nrdata - 1] = *interval; @@ -236,7 +240,7 @@ static int store_data(double time, int state, int cpu, { struct cpuidle_cstates *cstates = &datas->cstates[cpu]; struct cpuidle_cstate *cstate; - struct cpuidle_data *data; + struct cpuidle_data *data, *tmp; int nrdata, last_cstate = cstates->last_cstate;
/* ignore when we got a "closing" state first */ @@ -280,9 +284,12 @@ static int store_data(double time, int state, int cpu, return 0; }
- data = realloc(data, sizeof(*data) * (nrdata + 1)); - if (!data) + tmp = realloc(data, sizeof(*data) * (nrdata + 1)); + if (!tmp) { + free(data); return error("realloc data");; + } + data = tmp;
data[nrdata].begin = time;
From: Colin Ian King colin.king@canonical.com
A realloc failure leaks result on the error return path.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/idlestat.c b/idlestat.c index 41ed3ab..739f6dc 100644 --- a/idlestat.c +++ b/idlestat.c @@ -221,6 +221,7 @@ static struct cpuidle_cstate *inter(struct cpuidle_cstate *c1, (result->nrdata + 1)); if (!tmp) { free(data); + free(result); return NULL; } data = tmp;
From: Colin Ian King colin.king@canonical.com
s_cpu is not being checked for a memory allocation failure, instead s_core is being checked. Fix this cut-n-paste error.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/topology.c b/topology.c index 052ddb9..9de1354 100644 --- a/topology.c +++ b/topology.c @@ -128,7 +128,7 @@ int add_topo_info(struct cpu_topology *topo_list, struct topology_info *info) ptr = check_exist_from_head(&s_core->cpu_head, info->cpu_id); if (!ptr) { s_cpu = calloc(sizeof(struct cpu_cpu), 1); - if (!s_core) + if (!s_cpu) return -1;
s_cpu->cpu_id = info->cpu_id;
From: Colin Ian King colin.king@canonical.com
The error returns in idlestat_load fail to close an open file and free allocated memory on the heap. This patch addresses this.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/idlestat.c b/idlestat.c index 739f6dc..0961463 100644 --- a/idlestat.c +++ b/idlestat.c @@ -396,16 +396,24 @@ static struct cpuidle_datas *idlestat_load(const char *path) sscanf(buffer, "cpus=%u", &nrcpus); }
- if (!nrcpus) + if (!nrcpus) { + fclose(f); return ptrerror("read error for 'cpus=' in trace file"); + }
datas = malloc(sizeof(*datas)); - if (!datas) + if (!datas) { + fclose(f); return ptrerror("malloc datas"); + }
datas->cstates = calloc(sizeof(*datas->cstates), nrcpus); - if (!datas->cstates) + if (!datas->cstates) { + free(datas); + fclose(f); return ptrerror("calloc cstate"); + } + /* initialize cstate_max for each cpu */ for (cpu = 0; cpu < nrcpus; cpu++) datas->cstates[cpu].cstate_max = -1;
From: Colin Ian King colin.king@canonical.com
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/idlestat.c b/idlestat.c index 0961463..3f6076e 100644 --- a/idlestat.c +++ b/idlestat.c @@ -447,7 +447,7 @@ static struct cpuidle_datas *idlestat_load(const char *path)
fclose(f);
- fprintf(stderr, "Log is %lf secs long with %d events\n", + fprintf(stderr, "Log is %lf secs long with %zd events\n", end - begin, (int)count);
return datas;
From: Colin Ian King colin.king@canonical.com
Error message should be written to stderr rather than the failed to open file.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/idlestat.c b/idlestat.c index 3abfaaf..fce6112 100644 --- a/idlestat.c +++ b/idlestat.c @@ -659,7 +659,7 @@ static int idlestat_file_for_each_line(const char *path, void *data, f = fopen(path, "r");
if (!f) { - fprintf(f, "failed to open '%s': %m\n", path); + fprintf(stderr, "failed to open '%s': %m\n", path); return -1; }
From: Colin Ian King colin.king@canonical.com
Fix a common coding error where realloc fails and the original allocated data is not free'd.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/idlestat.c b/idlestat.c index fce6112..41ed3ab 100644 --- a/idlestat.c +++ b/idlestat.c @@ -188,6 +188,7 @@ static struct cpuidle_cstate *inter(struct cpuidle_cstate *c1, for (i = 0, index = 0; i < c1->nrdata; i++) {
for (j = index; j < c2->nrdata; j++) { + struct cpuidle_data *tmp;
/* intervals are ordered, no need to go further */ if (c1->data[i].end < c2->data[j].begin) @@ -216,10 +217,13 @@ static struct cpuidle_cstate *inter(struct cpuidle_cstate *c1,
result->nrdata++;
- data = realloc(data, sizeof(*data) * + tmp = realloc(data, sizeof(*data) * (result->nrdata + 1)); - if (!data) + if (!tmp) { + free(data); return NULL; + } + data = tmp;
result->data = data; result->data[result->nrdata - 1] = *interval; @@ -236,7 +240,7 @@ static int store_data(double time, int state, int cpu, { struct cpuidle_cstates *cstates = &datas->cstates[cpu]; struct cpuidle_cstate *cstate; - struct cpuidle_data *data; + struct cpuidle_data *data, *tmp; int nrdata, last_cstate = cstates->last_cstate;
/* ignore when we got a "closing" state first */ @@ -280,9 +284,12 @@ static int store_data(double time, int state, int cpu, return 0; }
- data = realloc(data, sizeof(*data) * (nrdata + 1)); - if (!data) + tmp = realloc(data, sizeof(*data) * (nrdata + 1)); + if (!tmp) { + free(data); return error("realloc data");; + } + data = tmp;
data[nrdata].begin = time;
From: Colin Ian King colin.king@canonical.com
A realloc failure leaks result on the error return path.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/idlestat.c b/idlestat.c index 41ed3ab..739f6dc 100644 --- a/idlestat.c +++ b/idlestat.c @@ -221,6 +221,7 @@ static struct cpuidle_cstate *inter(struct cpuidle_cstate *c1, (result->nrdata + 1)); if (!tmp) { free(data); + free(result); return NULL; } data = tmp;
From: Colin Ian King colin.king@canonical.com
s_cpu is not being checked for a memory allocation failure, instead s_core is being checked. Fix this cut-n-paste error.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/topology.c b/topology.c index 052ddb9..9de1354 100644 --- a/topology.c +++ b/topology.c @@ -128,7 +128,7 @@ int add_topo_info(struct cpu_topology *topo_list, struct topology_info *info) ptr = check_exist_from_head(&s_core->cpu_head, info->cpu_id); if (!ptr) { s_cpu = calloc(sizeof(struct cpu_cpu), 1); - if (!s_core) + if (!s_cpu) return -1;
s_cpu->cpu_id = info->cpu_id;
From: Colin Ian King colin.king@canonical.com
The error returns in idlestat_load fail to close an open file and free allocated memory on the heap. This patch addresses this.
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/idlestat.c b/idlestat.c index 739f6dc..0961463 100644 --- a/idlestat.c +++ b/idlestat.c @@ -396,16 +396,24 @@ static struct cpuidle_datas *idlestat_load(const char *path) sscanf(buffer, "cpus=%u", &nrcpus); }
- if (!nrcpus) + if (!nrcpus) { + fclose(f); return ptrerror("read error for 'cpus=' in trace file"); + }
datas = malloc(sizeof(*datas)); - if (!datas) + if (!datas) { + fclose(f); return ptrerror("malloc datas"); + }
datas->cstates = calloc(sizeof(*datas->cstates), nrcpus); - if (!datas->cstates) + if (!datas->cstates) { + free(datas); + fclose(f); return ptrerror("calloc cstate"); + } + /* initialize cstate_max for each cpu */ for (cpu = 0; cpu < nrcpus; cpu++) datas->cstates[cpu].cstate_max = -1;
From: Colin Ian King colin.king@canonical.com
Signed-off-by: Colin Ian King colin.king@canonical.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- idlestat.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/idlestat.c b/idlestat.c index 0961463..3f6076e 100644 --- a/idlestat.c +++ b/idlestat.c @@ -447,7 +447,7 @@ static struct cpuidle_datas *idlestat_load(const char *path)
fclose(f);
- fprintf(stderr, "Log is %lf secs long with %d events\n", + fprintf(stderr, "Log is %lf secs long with %zd events\n", end - begin, (int)count);
return datas;
Pulled, thanks! Zoran
On 17 February 2014 23:56, Daniel Lezcano daniel.lezcano@linaro.org wrote:
Hi Zoran,
Colin King sent me some patches to fixup memory leaks and some nits in the error code path.
Please, pull these changes in your tree.
The following changes since commit da6a8c94a8f8124711db0ae84a3ef4e0e186b388:
Fixing improperly initialized cstate_max per-CPU. (2014-01-29 15:47:47 -0800)
are available in the git repository at:
git://git.linaro.org/people/daniel.lezcano/idlestat.git master
for you to fetch changes up to 41505eca08f1bd0f2d35733cd631be5967e38724:
Use the correct format specifier for size_t type (2014-02-18 08:51:30 +0100)
Colin Ian King (6): Write open failure message to stderr Free memory on realloc failure. Fix memory leak, result is being leaked on a realloc failure check s_cpu rather than s_core on memory allocation Fix file and memory resource leak on error exit in idlestat_load Use the correct format specifier for size_t type
idlestat.c | 36 ++++++++++++++++++++++++++---------- topology.c | 2 +- 2 files changed, 27 insertions(+), 11 deletions(-)
-- http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro Facebook | http://twitter.com/#!/linaroorg Twitter | http://www.linaro.org/linaro-blog/ Blog
-
"Daniel Lezcano -
Daniel Lezcano
-
Zoran Markovic