Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752970AbdGFWZi (ORCPT ); Thu, 6 Jul 2017 18:25:38 -0400 Received: from mail.kernel.org ([198.145.29.99]:42498 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752939AbdGFWZf (ORCPT ); Thu, 6 Jul 2017 18:25:35 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1C4212133C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=goodmis.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=rostedt@goodmis.org Date: Thu, 6 Jul 2017 18:25:32 -0400 From: Steven Rostedt To: Federico Vaga Cc: LKML Subject: Re: [PATCH 1/2] trace-cmd: use asprintf when possible Message-ID: <20170706182532.212cba4f@gandalf.local.home> In-Reply-To: <20170605093118.29962-2-federico.vaga@vaga.pv.it> References: <20170605093118.29962-1-federico.vaga@vaga.pv.it> <20170605093118.29962-2-federico.vaga@vaga.pv.it> X-Mailer: Claws Mail 3.14.0 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12827 Lines: 432 On Mon, 5 Jun 2017 11:31:17 +0200 Federico Vaga wrote: Hi Federico, I finally got around to looking at these. Sorry for the really slow reply, but I had a bunch of kernel work I needed to get done before digging again into trace-cmd. > It makes the code clearer and less error prone. > > 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 I like this! > > 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. Yes, if we decide not to use GNU specific code, then we can implement our own version. > > Signed-off-by: Federico Vaga > --- > event-plugin.c | 24 +++++++++------------- > parse-filter.c | 11 ++++------ > trace-list.c | 8 ++++---- > trace-output.c | 6 +++--- > trace-record.c | 56 +++++++++++++++++++++------------------------------ > trace-recorder.c | 11 +++++----- > trace-show.c | 8 ++++---- > trace-split.c | 7 ++++--- > trace-stat.c | 7 ++++--- > trace-util.c | 61 +++++++++++++++++++++++--------------------------------- > 10 files changed, 85 insertions(+), 114 deletions(-) [...] > @@ -2237,11 +2233,10 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol > for (p = path + strlen(path) - 1; p > path; p--) > if (*p == '/') > break; > - *p = '\0'; > - p = malloc(strlen(path) + strlen("/enable") + 1); > - if (!p) > + *p = '\0'; /* FIXME is it ok ? */ I'm going to remove the comment. If you look at the code above it, You will see that 'p' is used to find the last instance of '/' in path. Then the *p = '\0' is used to remove it. > + ret = asprintf(&p, "%s/enable", path); > + if (ret < 0) > die("Failed to allocate enable path for %s", path); > - sprintf(p, "%s/enable", path); > ret = stat(p, &st); > if (ret >= 0) > event->enable_file = p; > @@ -2249,10 +2244,9 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol > free(p); > > if (event->trigger) { > - p = malloc(strlen(path) + strlen("/trigger") + 1); > - if (!p) > + ret = asprintf(&p, "%s/trigger", path); > + if (ret < 0) > die("Failed to allocate trigger path for %s", path); > - sprintf(p, "%s/trigger", path); > ret = stat(p, &st); > if (ret > 0) > die("trigger specified but not supported by this kernel"); > @@ -2268,17 +2262,16 @@ static void make_sched_event(struct buffer_instance *instance, > { > char *path; > char *p; > + int ret; > > /* Do nothing if the event already exists */ > if (*event) > return; > > - path = malloc(strlen(sched->filter_file) + strlen(sched_path) + 2); > - if (!path) > + ret = asprintf(&path, "%s", sched->filter_file); Now this part is not correct. It is actually buggy. If you noticed the malloc, it allocates strlen(sched->filter_file) + strlen(sched_path) + 2. Which is probably a little more than needed. > + if (ret < 0) > die("Failed to allocate path for %s", sched_path); > > - sprintf(path, "%s", sched->filter_file); > - Here I copy in the sched->filter_file which is the path I want, but I don't want the "/filter" part. So it is cut off below and the sched_path is copied in. Hmm, what would be better is to: asprintf(path, "%s/%s", dirname(sched->filter_file), sched_path); And remove all this open coded stuff. The same can probably be done above where you had the "fixme". > /* Remove the /filter from filter file */ > p = path + strlen(path) - strlen("filter"); > sprintf(p, "%s/filter", sched_path); > @@ -2310,10 +2303,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); > > @@ -3956,6 +3948,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") + > @@ -3963,10 +3957,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); > } > } > > @@ -4357,7 +4350,7 @@ void trace_record (int argc, char **argv) > usage(argv); > > for (;;) { > - int option_index = 0; > + int option_index = 0, ret; > const char *opts; > static struct option long_options[] = { > {"date", no_argument, NULL, OPT_date}, > @@ -4420,12 +4413,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..778c199 100644 > --- a/trace-stat.c > +++ b/trace-stat.c > @@ -70,15 +70,16 @@ char *strstrip(char *str) > return s; > } > > +/* FIXME repeated */ What do you mean by that? > 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 fbf8cea..29a7079 100644 > --- a/trace-util.c > +++ b/trace-util.c > @@ -88,14 +88,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); > @@ -617,14 +618,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", > @@ -710,7 +707,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; > @@ -752,16 +749,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; > } > @@ -780,13 +777,11 @@ const char *tracecmd_get_tracing_dir(void) > 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; > } > > /** > @@ -1392,7 +1387,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; > @@ -1415,14 +1411,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); > @@ -1509,15 +1501,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", > @@ -1606,6 +1595,7 @@ char *tracecmd_get_tracing_file(const char *name) > { > static const char *tracing; > char *file; > + int ret; > > if (!tracing) { > tracing = tracecmd_find_tracing_dir(); > @@ -1613,11 +1603,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; > } > The rest looks good. Do you want to send an updated patch, or do you want me to fix the above? -- Steve