lkml.org 
[lkml]   [2017]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] trace-cmd: use asprintf when possible
    On Mon,  5 Jun 2017 11:31:17 +0200
    Federico Vaga <federico.vaga@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.vaga@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.

    > + 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

    \
     
     \ /
      Last update: 2017-07-07 00:26    [W:4.207 / U:0.224 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site