lkml.org 
[lkml]   [2017]   [Aug]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V3 1/2] use direname instead of custom code

I'm back from vacation!

On Thu, 3 Aug 2017 00:15:57 +0200
Federico Vaga <federico.vaga@vaga.pv.it> wrote:

> Prefer well known functions like `dirname(3)` instead of custom
> implementation for the same functionality
>
> Signed-off-by: Federico Vaga <federico.vaga@vaga.pv.it>
> ---
> trace-record.c | 45 ++++++++++++++++++++++++++++++---------------
> 1 file changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/trace-record.c b/trace-record.c
> index 020a373..79e4fe4 100644
> --- a/trace-record.c
> +++ b/trace-record.c
> @@ -45,6 +45,7 @@
> #include <glob.h>
> #include <errno.h>
> #include <limits.h>
> +#include <libgen.h>
>
> #include "trace-local.h"
> #include "trace-msg.h"
> @@ -2215,12 +2216,24 @@ static void set_max_graph_depth(struct buffer_instance *instance, char *max_grap
> die("could not write to max_graph_depth");
> }
>
> +
> +/**
> + * create_event - create and event descriptor
> + * @instance: instance to use
> + * @path: path to event attribute
> + * @old_event: event descriptor to use as base
> + *
> + * NOTE: the function purpose is to create a data structure to describe
> + * an ftrace event. During the process it becomes handy to change the
> + * string `path`. So, do not rely on the content of `path` after you
> + * invoke this function.
> + */
> static struct event_list *
> create_event(struct buffer_instance *instance, char *path, struct event_list *old_event)
> {
> struct event_list *event;
> struct stat st;
> - char *p;
> + char *p, *path_dirname;

Just a personal preference, but I prefer to have each variable on its
own line, with a few exceptions. Makes diffs better when updating them.

> int ret;
>
> event = malloc(sizeof(*event));
> @@ -2234,14 +2247,14 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
> if (!event->filter_file)
> die("malloc filter file");
> }
> - for (p = path + strlen(path) - 1; p > path; p--)
> - if (*p == '/')
> - break;
> - *p = '\0';
> - p = malloc(strlen(path) + strlen("/enable") + 1);
> +
> + path_dirname = dirname(path);
> +
> + p = malloc(strlen(path_dirname) + strlen("/enable") + 1);
> if (!p)
> die("Failed to allocate enable path for %s", path);
> - sprintf(p, "%s/enable", path);
> + sprintf(p, "%s/enable", path_dirname);
> +
> ret = stat(p, &st);
> if (ret >= 0)
> event->enable_file = p;
> @@ -2249,10 +2262,11 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
> free(p);
>
> if (event->trigger) {
> - p = malloc(strlen(path) + strlen("/trigger") + 1);
> + p = malloc(strlen(path_dirname) + strlen("/trigger") + 1);
> if (!p)
> die("Failed to allocate trigger path for %s", path);
> - sprintf(p, "%s/trigger", path);
> + sprintf(p, "%s/trigger", path_dirname);
> +
> ret = stat(p, &st);
> if (ret > 0)
> die("trigger specified but not supported by this kernel");
> @@ -2266,8 +2280,7 @@ static void make_sched_event(struct buffer_instance *instance,
> struct event_list **event, struct event_list *sched,
> const char *sched_path)
> {
> - char *path;
> - char *p;
> + char *path, *path_dirname, *sched_filter_file_tmp;

These should be broken up too. Also, "sched_filter_file_tmp" is a bit
too descriptive. Something like "tmp_file" should suffice.

Thanks!

-- Steve

>
> /* Do nothing if the event already exists */
> if (*event)
> @@ -2277,11 +2290,13 @@ static void make_sched_event(struct buffer_instance *instance,
> if (!path)
> die("Failed to allocate path for %s", sched_path);
>
> - sprintf(path, "%s", sched->filter_file);
> + /* we do not want to corrupt sched->filter_file when using dirname() */
> + sched_filter_file_tmp = strdup(sched->filter_file);
> + if (!sched_filter_file_tmp)
> + die("Failed to allocate path for %s", sched_path);
> + path_dirname = dirname(sched_filter_file_tmp);
>
> - /* Remove the /filter from filter file */
> - p = path + strlen(path) - strlen("filter");
> - sprintf(p, "%s/filter", sched_path);
> + sprintf(path, "%s/%s/filter", path_dirname, sched_path);
>
> *event = create_event(instance, path, sched);
> free(path);

\
 
 \ /
  Last update: 2017-08-14 19:15    [W:0.160 / U:1.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site