On 26 November 2014 at 22:28, Tuukka Tikkanen
<tuukka.tikka...@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.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
