On Wed, 2 Aug 2017 18:22:43 +0200
Federico Vaga <[email protected]> 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;
>
