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 <federico.v...@vaga.pv.it> 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 <federico.v...@vaga.pv.it>
> > > ---
> > >
> > > 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/
