Replace asserts with displaying error messages and/or terminating program in a controlled manner. Also refine some error handling paths.
Signed-off-by: Pi-Cheng Chen pi-cheng.chen@linaro.org --- energy_model.c | 3 --- idlestat.c | 62 +++++++++++++++++++++++++++++++++++++++++++--------------- topology.c | 12 ++++++------ 3 files changed, 52 insertions(+), 25 deletions(-)
diff --git a/energy_model.c b/energy_model.c index 87ea3a8..b564ec6 100644 --- a/energy_model.c +++ b/energy_model.c @@ -3,7 +3,6 @@ #include <string.h> #include <stdbool.h> #include <errno.h> -#include <assert.h>
#include "energy_model.h" #include "idlestat.h" @@ -92,8 +91,6 @@ int parse_energy_model(struct program_options *options) char *path = options->energy_model_filename; int ret;
- assert(path != NULL); - f = fopen(path, "r"); if (!f) { if (errno == ENOENT) { diff --git a/idlestat.c b/idlestat.c index eff04e5..5bd2a14 100644 --- a/idlestat.c +++ b/idlestat.c @@ -40,7 +40,6 @@ #include <sys/types.h> #include <sys/resource.h> #include <sys/wait.h> -#include <assert.h>
#include "idlestat.h" #include "utils.h" @@ -647,7 +646,8 @@ static int alloc_pstate(struct cpufreq_pstates *pstates, unsigned int freq)
tmp = realloc(pstate, sizeof(*pstate) * (nrfreq + 1)); if (!tmp) { - perror("realloc pstate"); + fprintf(stderr, "%s: Failed ot realloc memory for pstate. " + "Exiting.\n", __func__); return -1; } pstate = tmp; @@ -864,7 +864,8 @@ static void cpu_change_pstate(struct cpuidle_datas *datas, int cpu, next = freq_to_pstate_index(ps, freq); if (next < 0) next = alloc_pstate(ps, freq); - assert(next >= 0); + if (next < 0) + exit(1);
switch (cur) { case 1: @@ -1090,15 +1091,24 @@ static int get_wakeup_irq(struct cpuidle_datas *datas, char *buffer, int count) char irqname[NAMELEN+1];
if (strstr(buffer, "irq_handler_entry")) { - assert(sscanf(buffer, TRACE_IRQ_FORMAT, &cpu, &irqid, - irqname) == 3); + if (sscanf(buffer, TRACE_IRQ_FORMAT, &cpu, &irqid, + irqname) != 3) { + fprintf(stderr, "warning: Unrecognized" + "irq_handler_entry record. Skip it.\n"); + return -1; + }
store_irq(cpu, irqid, irqname, datas); return 0; }
if (strstr(buffer, "ipi_entry")) { - assert(sscanf(buffer, TRACE_IPIIRQ_FORMAT, &cpu, irqname) == 2); + if (sscanf(buffer, TRACE_IPIIRQ_FORMAT, &cpu, irqname) != 2) { + fprintf(stderr, "warning: Unrecognized ipi_entry" + "record. Skip it.\n"); + return -1; + } + irqname[strlen(irqname) - 1] = '\0'; store_irq(cpu, -1, irqname, datas); return 0; @@ -1128,20 +1138,23 @@ struct cpuidle_datas *idlestat_load(struct program_options *options) if (strstr(buffer, "idlestat")) { options->format = IDLESTAT_HEADER; fgets(buffer, BUFSIZE, f); - assert(sscanf(buffer, "cpus=%u", &nrcpus) == 1); + if (sscanf(buffer, "cpus=%u", &nrcpus) != 1) + nrcpus = 0; fgets(buffer, BUFSIZE, f); } else if (strstr(buffer, "# tracer")) { options->format = TRACE_CMD_HEADER; while(!feof(f)) { if (buffer[0] != '#') break; - if (strstr(buffer, "#P:")) - assert(sscanf(buffer, "#%*[^#]#P:%u", &nrcpus) == 1); + if (strstr(buffer, "#P:") && + sscanf(buffer, "#%*[^#]#P:%u", &nrcpus) != 1) + nrcpus = 0; fgets(buffer, BUFSIZE, f); } } else { fprintf(stderr, "%s: unrecognized import format in '%s'\n", __func__, options->filename); + fclose(f); return NULL; }
@@ -1185,8 +1198,13 @@ struct cpuidle_datas *idlestat_load(struct program_options *options)
do { if (strstr(buffer, "cpu_idle")) { - assert(sscanf(buffer, TRACE_FORMAT, &time, &state, - &cpu) == 3); + if (sscanf(buffer, TRACE_FORMAT, &time, &state, &cpu) + != 3) { + fprintf(stderr, "warning: Unrecognized cpuidle" + "record. The result of analysis might " + "be wrong.\n"); + continue; + }
if (start) { begin = time; @@ -1198,8 +1216,13 @@ struct cpuidle_datas *idlestat_load(struct program_options *options) count++; continue; } else if (strstr(buffer, "cpu_frequency")) { - assert(sscanf(buffer, TRACE_FORMAT, &time, &freq, - &cpu) == 3); + if (sscanf(buffer, TRACE_FORMAT, &time, &freq, &cpu) + != 3) { + fprintf(stderr, "warning: Unrecognized cpufreq" + "record. The result of analysis might " + "be wrong.\n"); + continue; + } cpu_change_pstate(datas, cpu, freq, time); count++; continue; @@ -1524,8 +1547,11 @@ static int idlestat_store(const char *path, double start_ts, double end_ts, if (ret < 0) return -1;
- if (initp) - assert(ret == initp->nrcpus); + if (initp && ret != initp->nrcpus) { + fprintf(stderr, "%s: The number of CPUs is inconsistency\n", + __func__); + return -1; + }
f = fopen(path, "w+");
@@ -1716,7 +1742,11 @@ int main(int argc, char *argv[], char *const envp[]) if ((options.mode == TRACE) || args < argc) {
/* Read cpu topology info from sysfs */ - read_sysfs_cpu_topo(); + if (read_sysfs_cpu_topo()) { + fprintf(stderr, "Failed to read CPU topology info from" + " sysfs.\n"); + return 1; + }
/* Stop tracing (just in case) */ if (idlestat_trace_enable(false)) { diff --git a/topology.c b/topology.c index 92521e1..eddd479 100644 --- a/topology.c +++ b/topology.c @@ -34,7 +34,6 @@ #include <dirent.h> #include <ctype.h> #include <sys/stat.h> -#include <assert.h>
#include "list.h" #include "utils.h" @@ -313,8 +312,11 @@ static int topo_folder_scan(char *path, folder_filter_t filter) closedir(dir_topology);
read_topology_cb(newpath, &cpu_info); - assert(sscanf(direntp->d_name, "cpu%d", - &cpu_info.cpu_id) == 1); + if (sscanf(direntp->d_name, "cpu%d", + &cpu_info.cpu_id) != 1) { + ret = -1; + goto out_free_newpath; + } add_topo_info(&g_cpu_topo_list, &cpu_info); }
@@ -341,9 +343,7 @@ int init_cpu_topo_info(void)
int read_sysfs_cpu_topo(void) { - topo_folder_scan("/sys/devices/system/cpu", cpu_filter_cb); - - return 0; + return topo_folder_scan("/sys/devices/system/cpu", cpu_filter_cb); }
int read_cpu_topo_info(FILE *f, char *buf)
Hi,
committing amended (see below).
On Wed, Nov 26, 2014 at 10:15 AM, pi-cheng.chen pi-cheng.chen@linaro.org wrote:
Replace asserts with displaying error messages and/or terminating program in a controlled manner. Also refine some error handling paths.
This description doesn't really tell what is wrong in the original code. Replaced the commit message with the card description:
"Some parts of the code use asserts as a way to terminate execution in case input data processing fails. This leaves the user in the black regarding the reason why the tool terminated (and possibly dumped a core file). Replace the abnormal termination with displaying error message and terminating the application in a controlled fashion."
Signed-off-by: Pi-Cheng Chen pi-cheng.chen@linaro.org
energy_model.c | 3 --- idlestat.c | 62 +++++++++++++++++++++++++++++++++++++++++++--------------- topology.c | 12 ++++++------ 3 files changed, 52 insertions(+), 25 deletions(-)
diff --git a/energy_model.c b/energy_model.c index 87ea3a8..b564ec6 100644 --- a/energy_model.c +++ b/energy_model.c @@ -3,7 +3,6 @@ #include <string.h> #include <stdbool.h> #include <errno.h> -#include <assert.h>
#include "energy_model.h" #include "idlestat.h" @@ -92,8 +91,6 @@ int parse_energy_model(struct program_options *options) char *path = options->energy_model_filename; int ret;
assert(path != NULL);
This is a valid assertation (the function should never be called with a NULL energy model filename and the caller should enforce it and this condition does not depend directly on user input). Removed removing it.
f = fopen(path, "r"); if (!f) { if (errno == ENOENT) {diff --git a/idlestat.c b/idlestat.c index eff04e5..5bd2a14 100644 --- a/idlestat.c +++ b/idlestat.c @@ -40,7 +40,6 @@ #include <sys/types.h> #include <sys/resource.h> #include <sys/wait.h> -#include <assert.h>
#include "idlestat.h" #include "utils.h" @@ -647,7 +646,8 @@ static int alloc_pstate(struct cpufreq_pstates *pstates, unsigned int freq)
tmp = realloc(pstate, sizeof(*pstate) * (nrfreq + 1)); if (!tmp) {
perror("realloc pstate");
fprintf(stderr, "%s: Failed ot realloc memory for pstate. ""Exiting.\n", __func__); return -1;
Massaged the message and replaced return with exit(1), as we don't gain anything by propagating this error.
} pstate = tmp;@@ -864,7 +864,8 @@ static void cpu_change_pstate(struct cpuidle_datas *datas, int cpu, next = freq_to_pstate_index(ps, freq); if (next < 0) next = alloc_pstate(ps, freq);
assert(next >= 0);
if (next < 0)exit(1);
Especially given the above change, this assert is valid and should not be removed.
switch (cur) { case 1:@@ -1090,15 +1091,24 @@ static int get_wakeup_irq(struct cpuidle_datas *datas, char *buffer, int count) char irqname[NAMELEN+1];
if (strstr(buffer, "irq_handler_entry")) {
assert(sscanf(buffer, TRACE_IRQ_FORMAT, &cpu, &irqid,irqname) == 3);
if (sscanf(buffer, TRACE_IRQ_FORMAT, &cpu, &irqid,irqname) != 3) {fprintf(stderr, "warning: Unrecognized"
Added missing space.
"irq_handler_entry record. Skip it.\n");return -1;} store_irq(cpu, irqid, irqname, datas); return 0; } if (strstr(buffer, "ipi_entry")) {
assert(sscanf(buffer, TRACE_IPIIRQ_FORMAT, &cpu, irqname) == 2);
if (sscanf(buffer, TRACE_IPIIRQ_FORMAT, &cpu, irqname) != 2) {fprintf(stderr, "warning: Unrecognized ipi_entry"
Added missing space.
"record. Skip it.\n");return -1;}irqname[strlen(irqname) - 1] = '\0'; store_irq(cpu, -1, irqname, datas); return 0;@@ -1128,20 +1138,23 @@ struct cpuidle_datas *idlestat_load(struct program_options *options) if (strstr(buffer, "idlestat")) { options->format = IDLESTAT_HEADER; fgets(buffer, BUFSIZE, f);
assert(sscanf(buffer, "cpus=%u", &nrcpus) == 1);
if (sscanf(buffer, "cpus=%u", &nrcpus) != 1)nrcpus = 0; fgets(buffer, BUFSIZE, f); } else if (strstr(buffer, "# tracer")) { options->format = TRACE_CMD_HEADER; while(!feof(f)) { if (buffer[0] != '#') break;
if (strstr(buffer, "#P:"))assert(sscanf(buffer, "#%*[^#]#P:%u", &nrcpus) == 1);
if (strstr(buffer, "#P:") &&sscanf(buffer, "#%*[^#]#P:%u", &nrcpus) != 1)nrcpus = 0; fgets(buffer, BUFSIZE, f); } } else { fprintf(stderr, "%s: unrecognized import format in '%s'\n", __func__, options->filename);fclose(f);
Good catch.
return NULL; }@@ -1185,8 +1198,13 @@ struct cpuidle_datas *idlestat_load(struct program_options *options)
do { if (strstr(buffer, "cpu_idle")) {
assert(sscanf(buffer, TRACE_FORMAT, &time, &state,&cpu) == 3);
if (sscanf(buffer, TRACE_FORMAT, &time, &state, &cpu)!= 3) {fprintf(stderr, "warning: Unrecognized cpuidle"
Added missing space.
"record. The result of analysis might ""be wrong.\n");continue;} if (start) { begin = time;@@ -1198,8 +1216,13 @@ struct cpuidle_datas *idlestat_load(struct program_options *options) count++; continue; } else if (strstr(buffer, "cpu_frequency")) {
assert(sscanf(buffer, TRACE_FORMAT, &time, &freq,&cpu) == 3);
if (sscanf(buffer, TRACE_FORMAT, &time, &freq, &cpu)!= 3) {fprintf(stderr, "warning: Unrecognized cpufreq"
Added missing space.
"record. The result of analysis might ""be wrong.\n");continue;} cpu_change_pstate(datas, cpu, freq, time); count++; continue;@@ -1524,8 +1547,11 @@ static int idlestat_store(const char *path, double start_ts, double end_ts, if (ret < 0) return -1;
if (initp)assert(ret == initp->nrcpus);
if (initp && ret != initp->nrcpus) {fprintf(stderr, "%s: The number of CPUs is inconsistency\n",__func__);return -1;}
This assertation is valid and should not be removed.
f = fopen(path, "w+");@@ -1716,7 +1742,11 @@ int main(int argc, char *argv[], char *const envp[]) if ((options.mode == TRACE) || args < argc) {
/* Read cpu topology info from sysfs */
read_sysfs_cpu_topo();
if (read_sysfs_cpu_topo()) {fprintf(stderr, "Failed to read CPU topology info from"" sysfs.\n");return 1;} /* Stop tracing (just in case) */ if (idlestat_trace_enable(false)) {diff --git a/topology.c b/topology.c index 92521e1..eddd479 100644 --- a/topology.c +++ b/topology.c @@ -34,7 +34,6 @@ #include <dirent.h> #include <ctype.h> #include <sys/stat.h> -#include <assert.h>
#include "list.h" #include "utils.h" @@ -313,8 +312,11 @@ static int topo_folder_scan(char *path, folder_filter_t filter) closedir(dir_topology);
read_topology_cb(newpath, &cpu_info);
assert(sscanf(direntp->d_name, "cpu%d",&cpu_info.cpu_id) == 1);
if (sscanf(direntp->d_name, "cpu%d",&cpu_info.cpu_id) != 1) {ret = -1;
Added an error message.
goto out_free_newpath;} add_topo_info(&g_cpu_topo_list, &cpu_info); }@@ -341,9 +343,7 @@ int init_cpu_topo_info(void)
int read_sysfs_cpu_topo(void) {
topo_folder_scan("/sys/devices/system/cpu", cpu_filter_cb);return 0;
return topo_folder_scan("/sys/devices/system/cpu", cpu_filter_cb);}
int read_cpu_topo_info(FILE *f, char *buf)
1.9.1
Tuukka
On 26 November 2014 at 22:28, Tuukka Tikkanen tuukka.tikkanen@linaro.org wrote:
Hi,
committing amended (see below).
Hi Tuukka,
thanks for reviewing and fixing.
Pi-Cheng
On Wed, Nov 26, 2014 at 10:15 AM, pi-cheng.chen pi-cheng.chen@linaro.org wrote:
Replace asserts with displaying error messages and/or terminating program in a controlled manner. Also refine some error handling paths.
This description doesn't really tell what is wrong in the original code. Replaced the commit message with the card description:
"Some parts of the code use asserts as a way to terminate execution in case input data processing fails. This leaves the user in the black regarding the reason why the tool terminated (and possibly dumped a core file). Replace the abnormal termination with displaying error message and terminating the application in a controlled fashion."
Signed-off-by: Pi-Cheng Chen pi-cheng.chen@linaro.org
energy_model.c | 3 --- idlestat.c | 62 +++++++++++++++++++++++++++++++++++++++++++--------------- topology.c | 12 ++++++------ 3 files changed, 52 insertions(+), 25 deletions(-)
diff --git a/energy_model.c b/energy_model.c index 87ea3a8..b564ec6 100644 --- a/energy_model.c +++ b/energy_model.c @@ -3,7 +3,6 @@ #include <string.h> #include <stdbool.h> #include <errno.h> -#include <assert.h>
#include "energy_model.h" #include "idlestat.h" @@ -92,8 +91,6 @@ int parse_energy_model(struct program_options *options) char *path = options->energy_model_filename; int ret;
assert(path != NULL);This is a valid assertation (the function should never be called with a NULL energy model filename and the caller should enforce it and this condition does not depend directly on user input). Removed removing it.
f = fopen(path, "r"); if (!f) { if (errno == ENOENT) {diff --git a/idlestat.c b/idlestat.c index eff04e5..5bd2a14 100644 --- a/idlestat.c +++ b/idlestat.c @@ -40,7 +40,6 @@ #include <sys/types.h> #include <sys/resource.h> #include <sys/wait.h> -#include <assert.h>
#include "idlestat.h" #include "utils.h" @@ -647,7 +646,8 @@ static int alloc_pstate(struct cpufreq_pstates *pstates, unsigned int freq)
tmp = realloc(pstate, sizeof(*pstate) * (nrfreq + 1)); if (!tmp) {
perror("realloc pstate");
fprintf(stderr, "%s: Failed ot realloc memory for pstate. ""Exiting.\n", __func__); return -1;Massaged the message and replaced return with exit(1), as we don't gain anything by propagating this error.
} pstate = tmp;@@ -864,7 +864,8 @@ static void cpu_change_pstate(struct cpuidle_datas *datas, int cpu, next = freq_to_pstate_index(ps, freq); if (next < 0) next = alloc_pstate(ps, freq);
assert(next >= 0);
if (next < 0)exit(1);Especially given the above change, this assert is valid and should not be removed.
switch (cur) { case 1:@@ -1090,15 +1091,24 @@ static int get_wakeup_irq(struct cpuidle_datas *datas, char *buffer, int count) char irqname[NAMELEN+1];
if (strstr(buffer, "irq_handler_entry")) {
assert(sscanf(buffer, TRACE_IRQ_FORMAT, &cpu, &irqid,irqname) == 3);
if (sscanf(buffer, TRACE_IRQ_FORMAT, &cpu, &irqid,irqname) != 3) {fprintf(stderr, "warning: Unrecognized"Added missing space.
"irq_handler_entry record. Skip it.\n");return -1;} store_irq(cpu, irqid, irqname, datas); return 0; } if (strstr(buffer, "ipi_entry")) {
assert(sscanf(buffer, TRACE_IPIIRQ_FORMAT, &cpu, irqname) == 2);
if (sscanf(buffer, TRACE_IPIIRQ_FORMAT, &cpu, irqname) != 2) {fprintf(stderr, "warning: Unrecognized ipi_entry"Added missing space.
"record. Skip it.\n");return -1;}irqname[strlen(irqname) - 1] = '\0'; store_irq(cpu, -1, irqname, datas); return 0;@@ -1128,20 +1138,23 @@ struct cpuidle_datas *idlestat_load(struct program_options *options) if (strstr(buffer, "idlestat")) { options->format = IDLESTAT_HEADER; fgets(buffer, BUFSIZE, f);
assert(sscanf(buffer, "cpus=%u", &nrcpus) == 1);
if (sscanf(buffer, "cpus=%u", &nrcpus) != 1)nrcpus = 0; fgets(buffer, BUFSIZE, f); } else if (strstr(buffer, "# tracer")) { options->format = TRACE_CMD_HEADER; while(!feof(f)) { if (buffer[0] != '#') break;
if (strstr(buffer, "#P:"))assert(sscanf(buffer, "#%*[^#]#P:%u", &nrcpus) == 1);
if (strstr(buffer, "#P:") &&sscanf(buffer, "#%*[^#]#P:%u", &nrcpus) != 1)nrcpus = 0; fgets(buffer, BUFSIZE, f); } } else { fprintf(stderr, "%s: unrecognized import format in '%s'\n", __func__, options->filename);fclose(f);Good catch.
return NULL; }@@ -1185,8 +1198,13 @@ struct cpuidle_datas *idlestat_load(struct program_options *options)
do { if (strstr(buffer, "cpu_idle")) {
assert(sscanf(buffer, TRACE_FORMAT, &time, &state,&cpu) == 3);
if (sscanf(buffer, TRACE_FORMAT, &time, &state, &cpu)!= 3) {fprintf(stderr, "warning: Unrecognized cpuidle"Added missing space.
"record. The result of analysis might ""be wrong.\n");continue;} if (start) { begin = time;@@ -1198,8 +1216,13 @@ struct cpuidle_datas *idlestat_load(struct program_options *options) count++; continue; } else if (strstr(buffer, "cpu_frequency")) {
assert(sscanf(buffer, TRACE_FORMAT, &time, &freq,&cpu) == 3);
if (sscanf(buffer, TRACE_FORMAT, &time, &freq, &cpu)!= 3) {fprintf(stderr, "warning: Unrecognized cpufreq"Added missing space.
"record. The result of analysis might ""be wrong.\n");continue;} cpu_change_pstate(datas, cpu, freq, time); count++; continue;@@ -1524,8 +1547,11 @@ static int idlestat_store(const char *path, double start_ts, double end_ts, if (ret < 0) return -1;
if (initp)assert(ret == initp->nrcpus);
if (initp && ret != initp->nrcpus) {fprintf(stderr, "%s: The number of CPUs is inconsistency\n",__func__);return -1;}This assertation is valid and should not be removed.
f = fopen(path, "w+");@@ -1716,7 +1742,11 @@ int main(int argc, char *argv[], char *const envp[]) if ((options.mode == TRACE) || args < argc) {
/* Read cpu topology info from sysfs */
read_sysfs_cpu_topo();
if (read_sysfs_cpu_topo()) {fprintf(stderr, "Failed to read CPU topology info from"" sysfs.\n");return 1;} /* Stop tracing (just in case) */ if (idlestat_trace_enable(false)) {diff --git a/topology.c b/topology.c index 92521e1..eddd479 100644 --- a/topology.c +++ b/topology.c @@ -34,7 +34,6 @@ #include <dirent.h> #include <ctype.h> #include <sys/stat.h> -#include <assert.h>
#include "list.h" #include "utils.h" @@ -313,8 +312,11 @@ static int topo_folder_scan(char *path, folder_filter_t filter) closedir(dir_topology);
read_topology_cb(newpath, &cpu_info);
assert(sscanf(direntp->d_name, "cpu%d",&cpu_info.cpu_id) == 1);
if (sscanf(direntp->d_name, "cpu%d",&cpu_info.cpu_id) != 1) {ret = -1;Added an error message.
goto out_free_newpath;} add_topo_info(&g_cpu_topo_list, &cpu_info); }@@ -341,9 +343,7 @@ int init_cpu_topo_info(void)
int read_sysfs_cpu_topo(void) {
topo_folder_scan("/sys/devices/system/cpu", cpu_filter_cb);return 0;
return topo_folder_scan("/sys/devices/system/cpu", cpu_filter_cb);}
int read_cpu_topo_info(FILE *f, char *buf)
1.9.1
Tuukka
-
Pi-Cheng Chen -
pi-cheng.chen
-
Tuukka Tikkanen