From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752005AbdHBWUP (ORCPT ); Wed, 2 Aug 2017 18:20:15 -0400 Received: from mx.kolabnow.com ([95.128.36.41]:11642 "EHLO mx.kolabnow.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751959AbdHBWUB (ORCPT ); Wed, 2 Aug 2017 18:20:01 -0400 From: Federico Vaga To: Steven Rostedt Cc: LKML , Federico Vaga Subject: [PATCH V3 2/2] It makes the code clearer and less error prone. Date: Thu, 3 Aug 2017 00:15:58 +0200 Message-Id: <20170802221558.9684-2-federico.vaga@vaga.pv.it> In-Reply-To: <20170802221558.9684-1-federico.vaga@vaga.pv.it> References: <20170802221558.9684-1-federico.vaga@vaga.pv.it> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 --- 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