Date:   Wed, 2 Aug 2017 12:51:42 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Federico Vaga <federico.vaga@...a.pv.it>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 1/2] trace-cmd: use asprintf when possible

On Wed,  2 Aug 2017 18:22:43 +0200
Federico Vaga <federico.vaga@...a.pv.it> wrote:


> @@ -2215,14 +2212,28 @@ 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;
>  	int ret;
>  
> +	path_dirname = dirname(path);
> +

Can you break this up into two patches. One that adds the function
comment and dirname(), and then the other that converts asprintf(). The
asprintf() should be a simple conversion patch, and not something that
modifies the code flow.

>  	event = malloc(sizeof(*event));
>  	if (!event)
>  		die("Failed to allocate event");
> @@ -2234,14 +2245,10 @@ 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);
> -	if (!p)
> -		die("Failed to allocate enable path for %s", path);
> -	sprintf(p, "%s/enable", path);
> +
> +	ret = asprintf(&p, "%s/enable", path_dirname);
> +	if (ret < 0)
> +		die("Failed to allocate enable path for %s", path_dirname);
>  	ret = stat(p, &st);
>  	if (ret >= 0)
>  		event->enable_file = p;
> @@ -2249,10 +2256,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)
> -			die("Failed to allocate trigger path for %s", path);
> -		sprintf(p, "%s/trigger", path);
> +		ret = asprintf(&p, "%s/trigger", path_dirname);
> +		if (ret < 0)
> +			die("Failed to allocate trigger path for %s", path_dirname);
>  		ret = stat(p, &st);
>  		if (ret > 0)
>  			die("trigger specified but not supported by this kernel");
> @@ -2266,22 +2272,24 @@ 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, *sched_filter_file_tmp;
> +	int ret;
>  
>  	/* Do nothing if the event already exists */
>  	if (*event)
>  		return;
>  
> -	path = malloc(strlen(sched->filter_file) + strlen(sched_path) + 2);
> -	if (!path)
> +	/* 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);
>  
> -	sprintf(path, "%s", sched->filter_file);
> -
> -	/* Remove the /filter from filter file */
> -	p = path + strlen(path) - strlen("filter");
> -	sprintf(p, "%s/filter", sched_path);

Add the use of dirname() here too for the first patch.

> +	ret = asprintf(&path, "%s/%s/filter",
> +		       dirname(sched_filter_file_tmp),
> +		       sched_path);
> +	free(sched_filter_file_tmp);
> +	if (ret < 0)
> +		die("Failed to allocate path for %s", sched_path);
>  
>  	*event = create_event(instance, path, sched);
>  	free(path);
> @@ -2310,10 +2318,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);
>  
> @@ -3969,6 +3976,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") +
> @@ -3976,10 +3985,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);
>  	}
>  }
>  
> @@ -4373,7 +4381,7 @@ void trace_record (int argc, char **argv)
>  		usage(argv);
>  
>  	for (;;) {
> -		int option_index = 0;
> +		int option_index = 0, ret;

Oh, and just add "int ret", to me it looks better when there's an
assignment.

-- Steve

>  		const char *opts;
>  		static struct option long_options[] = {
>  			{"date", no_argument, NULL, OPT_date},
> @@ -4438,12 +4446,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;
>  

Powered by blists - more mailing lists