[PATCH V3 2/2] It makes the code clearer and less error prone.
From: Federico Vaga
Date: Wed Aug 02 2017 - 18:20:18 EST
clearer:
- less code
- the code is now using the same format to create strings dynamically
less error prone:
- no magic number +2 +9 +5 to compute the size
- no copy&paste of the strings to compute the size and to concatenate
The function `asprintf` is not POSIX standard but the program
was already using it. Later it can be decided to use only POSIX
functions, then we can easly replace all the `asprintf(3)` with a local
implementation of that function.
Signed-off-by: Federico Vaga <federico.vaga@xxxxxxxxxx>
---
event-plugin.c | 24 ++++++++--------------
parse-filter.c | 11 ++++------
trace-list.c | 8 ++++----
trace-output.c | 6 +++---
trace-record.c | 62 ++++++++++++++++++++++++--------------------------------
trace-recorder.c | 11 +++++-----
trace-show.c | 8 ++++----
trace-split.c | 7 ++++---
trace-stat.c | 7 ++++---
trace-util.c | 62 ++++++++++++++++++++++++--------------------------------
10 files changed, 89 insertions(+), 117 deletions(-)
diff --git a/event-plugin.c b/event-plugin.c
index a16756a..d542cb6 100644
--- a/event-plugin.c
+++ b/event-plugin.c
@@ -120,12 +120,12 @@ char **traceevent_plugin_list_options(void)
for (op = reg->options; op->name; op++) {
char *alias = op->plugin_alias ? op->plugin_alias : op->file;
char **temp = list;
+ int ret;
- name = malloc(strlen(op->name) + strlen(alias) + 2);
- if (!name)
+ ret = asprintf(&name, "%s:%s", alias, op->name);
+ if (ret < 0)
goto err;
- sprintf(name, "%s:%s", alias, op->name);
list = realloc(list, count + 2);
if (!list) {
list = temp;
@@ -290,17 +290,14 @@ load_plugin(struct pevent *pevent, const char *path,
const char *alias;
char *plugin;
void *handle;
+ int ret;
- plugin = malloc(strlen(path) + strlen(file) + 2);
- if (!plugin) {
+ ret = asprintf(&plugin, "%s/%s", path, file);
+ if (ret < 0) {
warning("could not allocate plugin memory\n");
return;
}
- strcpy(plugin, path);
- strcat(plugin, "/");
- strcat(plugin, file);
-
handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
if (!handle) {
warning("could not load plugin '%s'\n%s\n",
@@ -391,6 +388,7 @@ load_plugins(struct pevent *pevent, const char *suffix,
char *home;
char *path;
char *envdir;
+ int ret;
if (pevent->flags & PEVENT_DISABLE_PLUGINS)
return;
@@ -421,16 +419,12 @@ load_plugins(struct pevent *pevent, const char *suffix,
if (!home)
return;
- path = malloc(strlen(home) + strlen(LOCAL_PLUGIN_DIR) + 2);
- if (!path) {
+ ret = asprintf(&path, "%s/%s", home, LOCAL_PLUGIN_DIR);
+ if (ret < 0) {
warning("could not allocate plugin memory\n");
return;
}
- strcpy(path, home);
- strcat(path, "/");
- strcat(path, LOCAL_PLUGIN_DIR);
-
load_plugins_dir(pevent, suffix, path, load_plugin, data);
free(path);
diff --git a/parse-filter.c b/parse-filter.c
index 8e302c4..ebf2a95 100644
--- a/parse-filter.c
+++ b/parse-filter.c
@@ -287,12 +287,10 @@ find_event(struct pevent *pevent, struct event_list **events,
sys_name = NULL;
}
- reg = malloc(strlen(event_name) + 3);
- if (reg == NULL)
+ ret = asprintf(®, "^%s$", event_name);
+ if (ret < 0)
return PEVENT_ERRNO__MEM_ALLOC_FAILED;
- sprintf(reg, "^%s$", event_name);
-
ret = regcomp(&ereg, reg, REG_ICASE|REG_NOSUB);
free(reg);
@@ -300,13 +298,12 @@ find_event(struct pevent *pevent, struct event_list **events,
return PEVENT_ERRNO__INVALID_EVENT_NAME;
if (sys_name) {
- reg = malloc(strlen(sys_name) + 3);
- if (reg == NULL) {
+ ret = asprintf(®, "^%s$", sys_name);
+ if (ret < 0) {
regfree(&ereg);
return PEVENT_ERRNO__MEM_ALLOC_FAILED;
}
- sprintf(reg, "^%s$", sys_name);
ret = regcomp(&sreg, reg, REG_ICASE|REG_NOSUB);
free(reg);
if (ret) {
diff --git a/trace-list.c b/trace-list.c
index 293635f..32b19bc 100644
--- a/trace-list.c
+++ b/trace-list.c
@@ -133,6 +133,7 @@ static char *get_event_file(const char *type, char *buf, int len)
char *event;
char *path;
char *file;
+ int ret;
if (buf[len-1] == '\n')
buf[len-1] = '\0';
@@ -146,11 +147,10 @@ static char *get_event_file(const char *type, char *buf, int len)
die("no event found in %s\n", buf);
path = tracecmd_get_tracing_file("events");
- file = malloc(strlen(path) + strlen(system) + strlen(event) +
- strlen(type) + strlen("///") + 1);
- if (!file)
+ ret = asprintf(&file, "%s/%s/%s/%s", path, system, event, type);
+ if (ret < 0)
die("Failed to allocate event file %s %s", system, event);
- sprintf(file, "%s/%s/%s/%s", path, system, event, type);
+
tracecmd_put_tracing_file(path);
return file;
diff --git a/trace-output.c b/trace-output.c
index bfe6331..26ed560 100644
--- a/trace-output.c
+++ b/trace-output.c
@@ -234,16 +234,16 @@ static char *get_tracing_file(struct tracecmd_output *handle, const char *name)
{
const char *tracing;
char *file;
+ int ret;
tracing = find_tracing_dir(handle);
if (!tracing)
return NULL;
- file = malloc(strlen(tracing) + strlen(name) + 2);
- if (!file)
+ ret = asprintf(&file, "%s/%s", tracing, name);
+ if (ret < 0)
return NULL;
- sprintf(file, "%s/%s", tracing, name);
return file;
}
diff --git a/trace-record.c b/trace-record.c
index 79e4fe4..b1993c7 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -724,14 +724,12 @@ get_instance_file(struct buffer_instance *instance, const char *file)
{
char *buf;
char *path;
+ int ret;
if (instance->name) {
- buf = malloc(strlen(instance->name) +
- strlen(file) + strlen("instances//") + 1);
- if (!buf)
+ ret = asprintf(&buf, "instances/%s/%s", instance->name, file);
+ if (ret < 0)
die("Failed to allocate name for %s/%s", instance->name, file);
- sprintf(buf, "instances/%s/%s", instance->name, file);
-
path = tracecmd_get_tracing_file(buf);
free(buf);
} else
@@ -745,17 +743,15 @@ get_instance_dir(struct buffer_instance *instance)
{
char *buf;
char *path;
+ int ret;
/* only works for instances */
if (!instance->name)
return NULL;
- buf = malloc(strlen(instance->name) +
- strlen("instances/") + 1);
- if (!buf)
+ ret = asprintf(&buf, "instances/%s", instance->name);
+ if (ret < 0)
die("Failed to allocate for instance %s", instance->name);
- sprintf(buf, "instances/%s", instance->name);
-
path = tracecmd_get_tracing_file(buf);
free(buf);
@@ -2250,11 +2246,9 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
path_dirname = dirname(path);
- p = malloc(strlen(path_dirname) + strlen("/enable") + 1);
- if (!p)
- die("Failed to allocate enable path for %s", path);
- sprintf(p, "%s/enable", path_dirname);
-
+ ret = asprintf(&p, "%s/enable", path_dirname);
+ if (ret < 0)
+ die("Failed to allocate enable path for %s", path_dirname);
ret = stat(p, &st);
if (ret >= 0)
event->enable_file = p;
@@ -2262,11 +2256,9 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
free(p);
if (event->trigger) {
- p = malloc(strlen(path_dirname) + strlen("/trigger") + 1);
- if (!p)
- die("Failed to allocate trigger path for %s", path);
- sprintf(p, "%s/trigger", path_dirname);
-
+ ret = asprintf(&p, "%s/trigger", path_dirname);
+ if (ret < 0)
+ die("Failed to allocate trigger path for %s", path_dirname);
ret = stat(p, &st);
if (ret > 0)
die("trigger specified but not supported by this kernel");
@@ -2281,22 +2273,22 @@ static void make_sched_event(struct buffer_instance *instance,
const char *sched_path)
{
char *path, *path_dirname, *sched_filter_file_tmp;
+ int ret;
/* Do nothing if the event already exists */
if (*event)
return;
- path = malloc(strlen(sched->filter_file) + strlen(sched_path) + 2);
- if (!path)
- die("Failed to allocate path for %s", sched_path);
-
/* we do not want to corrupt sched->filter_file when using dirname() */
sched_filter_file_tmp = strdup(sched->filter_file);
if (!sched_filter_file_tmp)
die("Failed to allocate path for %s", sched_path);
path_dirname = dirname(sched_filter_file_tmp);
- sprintf(path, "%s/%s/filter", path_dirname, sched_path);
+ ret = asprintf(&path, "%s/%s/filter", path_dirname, sched_path);
+ free(sched_filter_file_tmp);
+ if (ret < 0)
+ die("Failed to allocate path for %s", sched_path);
*event = create_event(instance, path, sched);
free(path);
@@ -2325,10 +2317,9 @@ static int expand_event_files(struct buffer_instance *instance,
int ret;
int i;
- p = malloc(strlen(file) + strlen("events//filter") + 1);
- if (!p)
+ ret = asprintf(&p, "events/%s/filter", file);
+ if (ret < 0)
die("Failed to allocate event filter path for %s", file);
- sprintf(p, "events/%s/filter", file);
path = get_instance_file(instance, p);
@@ -3984,6 +3975,8 @@ static int recording_all_events(void)
static void add_trigger(struct event_list *event, const char *trigger)
{
+ int ret;
+
if (event->trigger) {
event->trigger = realloc(event->trigger,
strlen(event->trigger) + strlen("\n") +
@@ -3991,10 +3984,9 @@ static void add_trigger(struct event_list *event, const char *trigger)
strcat(event->trigger, "\n");
strcat(event->trigger, trigger);
} else {
- event->trigger = malloc(strlen(trigger) + 1);
- if (!event->trigger)
+ ret = asprintf(&event->trigger, "%s", trigger);
+ if (ret < 0)
die("Failed to allocate event trigger");
- sprintf(event->trigger, "%s", trigger);
}
}
@@ -4389,6 +4381,7 @@ void trace_record (int argc, char **argv)
for (;;) {
int option_index = 0;
+ int ret;
const char *opts;
static struct option long_options[] = {
{"date", no_argument, NULL, OPT_date},
@@ -4453,12 +4446,9 @@ void trace_record (int argc, char **argv)
strcat(last_event->filter, optarg);
strcat(last_event->filter, ")");
} else {
- last_event->filter =
- malloc(strlen(optarg) +
- strlen("()") + 1);
- if (!last_event->filter)
+ ret = asprintf(&last_event->filter, "(%s)", optarg);
+ if (ret < 0)
die("Failed to allocate filter %s", optarg);
- sprintf(last_event->filter, "(%s)", optarg);
}
break;
diff --git a/trace-recorder.c b/trace-recorder.c
index 1b9d364..85150fd 100644
--- a/trace-recorder.c
+++ b/trace-recorder.c
@@ -156,14 +156,13 @@ tracecmd_create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags,
recorder->fd1 = fd;
recorder->fd2 = fd2;
- path = malloc(strlen(buffer) + 40);
- if (!path)
- goto out_free;
-
if (flags & TRACECMD_RECORD_SNAPSHOT)
- sprintf(path, "%s/per_cpu/cpu%d/snapshot_raw", buffer, cpu);
+ ret = asprintf(&path, "%s/per_cpu/cpu%d/snapshot_raw", buffer, cpu);
else
- sprintf(path, "%s/per_cpu/cpu%d/trace_pipe_raw", buffer, cpu);
+ ret = asprintf(&path, "%s/per_cpu/cpu%d/trace_pipe_raw", buffer, cpu);
+ if (ret < 0)
+ goto out_free;
+
recorder->trace_fd = open(path, O_RDONLY);
if (recorder->trace_fd < 0)
goto out_free;
diff --git a/trace-show.c b/trace-show.c
index 14b786c..f13db31 100644
--- a/trace-show.c
+++ b/trace-show.c
@@ -154,11 +154,11 @@ void trace_show(int argc, char **argv)
}
if (buffer) {
- path = malloc(strlen(buffer) + strlen("instances//") +
- strlen(file) + 1);
- if (!path)
+ int ret;
+
+ ret = asprintf(&path, "instances/%s/%s", buffer, file);
+ if (ret < 0)
die("Failed to allocate instance path %s", file);
- sprintf(path, "instances/%s/%s", buffer, file);
file = path;
}
diff --git a/trace-split.c b/trace-split.c
index 87d43ad..5e4ac68 100644
--- a/trace-split.c
+++ b/trace-split.c
@@ -369,10 +369,11 @@ static double parse_file(struct tracecmd_input *handle,
die("Failed to allocate cpu_data for %d cpus", cpus);
for (cpu = 0; cpu < cpus; cpu++) {
- file = malloc(strlen(output_file) + 50);
- if (!file)
+ int ret;
+
+ ret = asprintf(&file, "%s/.tmp.%s.%d", dir, base, cpu);
+ if (ret < 0)
die("Failed to allocate file for %s %s %d", dir, base, cpu);
- sprintf(file, "%s/.tmp.%s.%d", dir, base, cpu);
fd = open(file, O_WRONLY | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
cpu_data[cpu].cpu = cpu;
cpu_data[cpu].fd = fd;
diff --git a/trace-stat.c b/trace-stat.c
index adbf3c3..fd16354 100644
--- a/trace-stat.c
+++ b/trace-stat.c
@@ -70,15 +70,16 @@ char *strstrip(char *str)
return s;
}
+/* FIXME: append_file() is duplicated and could be consolidated */
char *append_file(const char *dir, const char *name)
{
char *file;
+ int ret;
- file = malloc(strlen(dir) + strlen(name) + 2);
- if (!file)
+ ret = asprintf(&file, "%s/%s", dir, name);
+ if (ret < 0)
die("Failed to allocate %s/%s", dir, name);
- sprintf(file, "%s/%s", dir, name);
return file;
}
diff --git a/trace-util.c b/trace-util.c
index d464a61..45fa95a 100644
--- a/trace-util.c
+++ b/trace-util.c
@@ -85,14 +85,15 @@ char **trace_util_list_plugin_options(void)
for (reg = registered_options; reg; reg = reg->next) {
for (op = reg->options; op->name; op++) {
char *alias = op->plugin_alias ? op->plugin_alias : op->file;
+ int ret;
- name = malloc(strlen(op->name) + strlen(alias) + 2);
- if (!name) {
+ ret = asprintf(&name, "%s:%s", alias, op->name);
+ if (ret < 0) {
warning("Failed to allocate plugin option %s:%s",
alias, op->name);
break;
}
- sprintf(name, "%s:%s", alias, op->name);
+
list = realloc(list, count + 2);
if (!list) {
warning("Failed to allocate plugin list for %s", name);
@@ -614,14 +615,10 @@ static int load_plugin(struct pevent *pevent, const char *path,
void *handle;
int ret;
- plugin = malloc(strlen(path) + strlen(file) + 2);
- if (!plugin)
+ ret = asprintf(&plugin, "%s/%s", path, file);
+ if (ret < 0)
return -ENOMEM;
- strcpy(plugin, path);
- strcat(plugin, "/");
- strcat(plugin, file);
-
handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
if (!handle) {
warning("cound not load plugin '%s'\n%s\n",
@@ -707,7 +704,7 @@ char *tracecmd_find_tracing_dir(void)
char type[100];
int use_debug = 0;
FILE *fp;
-
+
if ((fp = fopen("/proc/mounts","r")) == NULL) {
warning("Can't open /proc/mounts for read");
return NULL;
@@ -749,16 +746,16 @@ char *tracecmd_find_tracing_dir(void)
free(debug_str);
if (use_debug) {
- tracing_dir = malloc(strlen(fspath) + 9);
- if (!tracing_dir)
- return NULL;
+ int ret;
- sprintf(tracing_dir, "%s/tracing", fspath);
+ ret = asprintf(&tracing_dir, "%s/tracing", fspath);
+ if (ret < 0)
+ return NULL;
} else {
tracing_dir = strdup(fspath);
if (!tracing_dir)
return NULL;
- }
+ }
return tracing_dir;
}
@@ -774,16 +771,15 @@ const char *tracecmd_get_tracing_dir(void)
return tracing_dir;
}
+/* FIXME: append_file() is duplicated and could be consolidated */
static char *append_file(const char *dir, const char *name)
{
char *file;
+ int ret;
- file = malloc(strlen(dir) + strlen(name) + 2);
- if (!file)
- return NULL;
+ ret = asprintf(&file, "%s/%s", dir, name);
- sprintf(file, "%s/%s", dir, name);
- return file;
+ return ret < 0 ? NULL : file;
}
/**
@@ -1389,7 +1385,8 @@ int trace_util_load_plugins(struct pevent *pevent, const char *suffix,
{
char *home;
char *path;
- char *envdir;
+ char *envdir;
+ int ret;
if (tracecmd_disable_plugins)
return -EBUSY;
@@ -1412,14 +1409,10 @@ int trace_util_load_plugins(struct pevent *pevent, const char *suffix,
if (!home)
return -EINVAL;
- path = malloc(strlen(home) + strlen(LOCAL_PLUGIN_DIR) + 2);
- if (!path)
+ ret = asprintf(&path, "%s/%s", home, LOCAL_PLUGIN_DIR);
+ if (ret < 0)
return -ENOMEM;
- strcpy(path, home);
- strcat(path, "/");
- strcat(path, LOCAL_PLUGIN_DIR);
-
trace_util_load_plugins_dir(pevent, suffix, path, load_plugin, data);
free(path);
@@ -1506,15 +1499,12 @@ static int read_options(struct pevent *pevent, const char *path,
int unload = 0;
char *plugin;
void *handle;
+ int ret;
- plugin = malloc(strlen(path) + strlen(file) + 2);
- if (!plugin)
+ ret = asprintf(&plugin, "%s/%s", path, file);
+ if (ret < 0)
return -ENOMEM;
- strcpy(plugin, path);
- strcat(plugin, "/");
- strcat(plugin, file);
-
handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
if (!handle) {
warning("cound not load plugin '%s'\n%s\n",
@@ -1603,6 +1593,7 @@ char *tracecmd_get_tracing_file(const char *name)
{
static const char *tracing;
char *file;
+ int ret;
if (!tracing) {
tracing = tracecmd_find_tracing_dir();
@@ -1610,11 +1601,10 @@ char *tracecmd_get_tracing_file(const char *name)
return NULL;
}
- file = malloc(strlen(tracing) + strlen(name) + 2);
- if (!file)
+ ret = asprintf(&file, "%s/%s", tracing, name);
+ if (ret < 0)
return NULL;
- sprintf(file, "%s/%s", tracing, name);
return file;
}
--
2.13.3