I'm back from vacation! On Thu, 3 Aug 2017 00:15:57 +0200 Federico Vaga <[email protected]> wrote:
> Prefer well known functions like `dirname(3)` instead of custom > implementation for the same functionality > > Signed-off-by: Federico Vaga <[email protected]> > --- > 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);
