Hi,
committing amended (see below).
On Wed, Nov 26, 2014 at 10:15 AM, pi-cheng.chen
<pi-cheng.c...@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.c...@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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
