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