Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752107AbdGYOVT (ORCPT ); Tue, 25 Jul 2017 10:21:19 -0400 Received: from mx.kolabnow.com ([95.128.36.42]:60256 "EHLO mx.kolabnow.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751812AbdGYOVS (ORCPT ); Tue, 25 Jul 2017 10:21:18 -0400 From: Federico Vaga To: Steven Rostedt Cc: LKML Subject: Re: [PATCH 1/2] trace-cmd: use asprintf when possible Date: Tue, 25 Jul 2017 16:21:11 +0200 Message-ID: <1967443.iUBOHzf2Bp@harkonnen> Organization: VAGA In-Reply-To: <2876336.80y3AYhZJ7@harkonnen> References: <20170605093118.29962-1-federico.vaga@vaga.pv.it> <20170706182532.212cba4f@gandalf.local.home> <2876336.80y3AYhZJ7@harkonnen> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 18463 Lines: 660 Hi Steven, I found some free time and unfortunately I can't enjoy the sun, so here I am on this patch. Before submitting the V2, one comment (inline) On Monday, July 10, 2017 2:08:41 AM CEST Federico Vaga wrote: > On Friday, July 7, 2017 12:25:32 AM CEST Steven Rostedt wrote: > > 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. > > At the beginning the logic was not clear to me, in particular "why is it > doing this?", then I understood by having a look at the usage of > `create_event()` but I forget to remove the FIXME. > > > But still, it is not immediately obvious why we need this without reading > how the function has been used. > > Answer to the question: > we need it because when we call `create_event()` we pass the path to the > filter file, and not the path to the event directory. > > > In my opinion we should pass the path to the event directory, and from this > we can build the event_list's paths. To me, it sounds more correct for a > function named `create_event()`, rather than: > - taking a path to a specific event file, > - deduce the event directory, > - build the path for the other event files, While I was fixing my patch according to what we said last time, I think I recall what was my true original meaning of "/* FIXME is it ok ? */". (What I wrote last time is still valid anyway) The questions comes by the fact that this line: *p = '\0'; /* FIXME is it ok ? */ changes the input parameter by cutting it (it does what dirname() does). So, "is it ok (to cut the input string)?". According to the internal usage, when a function uses `create_event()`, it passes a generated string that then is not used anymore. So, nobody cares if this string has been manipulated by create_event(). I think that this should not happen. So I will propose a patch V2 where I use `dirname()` as suggested but on local duplicate using `strdup()`. This guarantee (even if it is not necessary) that the input string does not change. > > > > + 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. > > Yes, you are right. > > > > + 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". > > yes > > > > /* 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? I forget to answer to this point last time. The function `append_file()` is implemented twice in trace-stat.c and trace- util.c I noticed that those two files are included in different binaries (trace-cmd and the libraries). I just put a note because instead of having multiple implementation we can have just one in a file that gets included where is needed. Of course, if it is just for such a simple function it does not make much sense right now. But if we can group all the internal helpers I believe is better. I will remove the fixme from the V2 patch > > > > > 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? > > I can send an updated patch, but I do not know when (weeks). It is a super > easy patch but I'm really busy and tired at the moment. Sorry :/ > > If you have time and you want to do it, please go ahead. Otherwise I will do > it when I will have free time :) -- Federico Vaga http://www.federicovaga.it/