DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DE06C23960
Date: Mon, 14 Aug 2017 13:14:19 -0400
From: Steven Rostedt <rostedt@goodmis.org>
To: Federico Vaga <federico.vaga@vaga.pv.it>
Cc: LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH V3 1/2] use direname instead of custom code
Message-ID: <20170814131419.4b0f58d9@gandalf.local.home>
In-Reply-To: <20170802221558.9684-1-federico.vaga@vaga.pv.it>
References: <20170802221558.9684-1-federico.vaga@vaga.pv.it>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4163
Lines: 127


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);