2017-08-02 22:19:37

by Federico Vaga

[permalink] [raw]
Subject: [PATCH V3 1/2] use direname instead of custom code

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

/* 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);
--
2.13.3


2017-08-02 22:20:15

by Federico Vaga

[permalink] [raw]
Subject: [PATCH V3 2/2] It makes the code clearer and less error prone.

clearer:
- less code
- the code is now using the same format to create strings dynamically

less error prone:
- no magic number +2 +9 +5 to compute the size
- no copy&paste of the strings to compute the size and to concatenate

The function `asprintf` is not POSIX standard but the program
was already using it. Later it can be decided to use only POSIX
functions, then we can easly replace all the `asprintf(3)` with a local
implementation of that function.

Signed-off-by: Federico Vaga <[email protected]>
---
event-plugin.c | 24 ++++++++--------------
parse-filter.c | 11 ++++------
trace-list.c | 8 ++++----
trace-output.c | 6 +++---
trace-record.c | 62 ++++++++++++++++++++++++--------------------------------
trace-recorder.c | 11 +++++-----
trace-show.c | 8 ++++----
trace-split.c | 7 ++++---
trace-stat.c | 7 ++++---
trace-util.c | 62 ++++++++++++++++++++++++--------------------------------
10 files changed, 89 insertions(+), 117 deletions(-)

diff --git a/event-plugin.c b/event-plugin.c
index a16756a..d542cb6 100644
--- a/event-plugin.c
+++ b/event-plugin.c
@@ -120,12 +120,12 @@ char **traceevent_plugin_list_options(void)
for (op = reg->options; op->name; op++) {
char *alias = op->plugin_alias ? op->plugin_alias : op->file;
char **temp = list;
+ int ret;

- name = malloc(strlen(op->name) + strlen(alias) + 2);
- if (!name)
+ ret = asprintf(&name, "%s:%s", alias, op->name);
+ if (ret < 0)
goto err;

- sprintf(name, "%s:%s", alias, op->name);
list = realloc(list, count + 2);
if (!list) {
list = temp;
@@ -290,17 +290,14 @@ load_plugin(struct pevent *pevent, const char *path,
const char *alias;
char *plugin;
void *handle;
+ int ret;

- plugin = malloc(strlen(path) + strlen(file) + 2);
- if (!plugin) {
+ ret = asprintf(&plugin, "%s/%s", path, file);
+ if (ret < 0) {
warning("could not allocate plugin memory\n");
return;
}

- strcpy(plugin, path);
- strcat(plugin, "/");
- strcat(plugin, file);
-
handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
if (!handle) {
warning("could not load plugin '%s'\n%s\n",
@@ -391,6 +388,7 @@ load_plugins(struct pevent *pevent, const char *suffix,
char *home;
char *path;
char *envdir;
+ int ret;

if (pevent->flags & PEVENT_DISABLE_PLUGINS)
return;
@@ -421,16 +419,12 @@ load_plugins(struct pevent *pevent, const char *suffix,
if (!home)
return;

- path = malloc(strlen(home) + strlen(LOCAL_PLUGIN_DIR) + 2);
- if (!path) {
+ ret = asprintf(&path, "%s/%s", home, LOCAL_PLUGIN_DIR);
+ if (ret < 0) {
warning("could not allocate plugin memory\n");
return;
}

- strcpy(path, home);
- strcat(path, "/");
- strcat(path, LOCAL_PLUGIN_DIR);
-
load_plugins_dir(pevent, suffix, path, load_plugin, data);

free(path);
diff --git a/parse-filter.c b/parse-filter.c
index 8e302c4..ebf2a95 100644
--- a/parse-filter.c
+++ b/parse-filter.c
@@ -287,12 +287,10 @@ find_event(struct pevent *pevent, struct event_list **events,
sys_name = NULL;
}

- reg = malloc(strlen(event_name) + 3);
- if (reg == NULL)
+ ret = asprintf(&reg, "^%s$", event_name);
+ if (ret < 0)
return PEVENT_ERRNO__MEM_ALLOC_FAILED;

- sprintf(reg, "^%s$", event_name);
-
ret = regcomp(&ereg, reg, REG_ICASE|REG_NOSUB);
free(reg);

@@ -300,13 +298,12 @@ find_event(struct pevent *pevent, struct event_list **events,
return PEVENT_ERRNO__INVALID_EVENT_NAME;

if (sys_name) {
- reg = malloc(strlen(sys_name) + 3);
- if (reg == NULL) {
+ ret = asprintf(&reg, "^%s$", sys_name);
+ if (ret < 0) {
regfree(&ereg);
return PEVENT_ERRNO__MEM_ALLOC_FAILED;
}

- sprintf(reg, "^%s$", sys_name);
ret = regcomp(&sreg, reg, REG_ICASE|REG_NOSUB);
free(reg);
if (ret) {
diff --git a/trace-list.c b/trace-list.c
index 293635f..32b19bc 100644
--- a/trace-list.c
+++ b/trace-list.c
@@ -133,6 +133,7 @@ static char *get_event_file(const char *type, char *buf, int len)
char *event;
char *path;
char *file;
+ int ret;

if (buf[len-1] == '\n')
buf[len-1] = '\0';
@@ -146,11 +147,10 @@ static char *get_event_file(const char *type, char *buf, int len)
die("no event found in %s\n", buf);

path = tracecmd_get_tracing_file("events");
- file = malloc(strlen(path) + strlen(system) + strlen(event) +
- strlen(type) + strlen("///") + 1);
- if (!file)
+ ret = asprintf(&file, "%s/%s/%s/%s", path, system, event, type);
+ if (ret < 0)
die("Failed to allocate event file %s %s", system, event);
- sprintf(file, "%s/%s/%s/%s", path, system, event, type);
+
tracecmd_put_tracing_file(path);

return file;
diff --git a/trace-output.c b/trace-output.c
index bfe6331..26ed560 100644
--- a/trace-output.c
+++ b/trace-output.c
@@ -234,16 +234,16 @@ static char *get_tracing_file(struct tracecmd_output *handle, const char *name)
{
const char *tracing;
char *file;
+ int ret;

tracing = find_tracing_dir(handle);
if (!tracing)
return NULL;

- file = malloc(strlen(tracing) + strlen(name) + 2);
- if (!file)
+ ret = asprintf(&file, "%s/%s", tracing, name);
+ if (ret < 0)
return NULL;

- sprintf(file, "%s/%s", tracing, name);
return file;
}

diff --git a/trace-record.c b/trace-record.c
index 79e4fe4..b1993c7 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -724,14 +724,12 @@ get_instance_file(struct buffer_instance *instance, const char *file)
{
char *buf;
char *path;
+ int ret;

if (instance->name) {
- buf = malloc(strlen(instance->name) +
- strlen(file) + strlen("instances//") + 1);
- if (!buf)
+ ret = asprintf(&buf, "instances/%s/%s", instance->name, file);
+ if (ret < 0)
die("Failed to allocate name for %s/%s", instance->name, file);
- sprintf(buf, "instances/%s/%s", instance->name, file);
-
path = tracecmd_get_tracing_file(buf);
free(buf);
} else
@@ -745,17 +743,15 @@ get_instance_dir(struct buffer_instance *instance)
{
char *buf;
char *path;
+ int ret;

/* only works for instances */
if (!instance->name)
return NULL;

- buf = malloc(strlen(instance->name) +
- strlen("instances/") + 1);
- if (!buf)
+ ret = asprintf(&buf, "instances/%s", instance->name);
+ if (ret < 0)
die("Failed to allocate for instance %s", instance->name);
- sprintf(buf, "instances/%s", instance->name);
-
path = tracecmd_get_tracing_file(buf);
free(buf);

@@ -2250,11 +2246,9 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol

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_dirname);
-
+ 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;
@@ -2262,11 +2256,9 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
free(p);

if (event->trigger) {
- p = malloc(strlen(path_dirname) + strlen("/trigger") + 1);
- if (!p)
- die("Failed to allocate trigger path for %s", path);
- sprintf(p, "%s/trigger", path_dirname);
-
+ 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");
@@ -2281,22 +2273,22 @@ static void make_sched_event(struct buffer_instance *instance,
const char *sched_path)
{
char *path, *path_dirname, *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)
- die("Failed to allocate path for %s", sched_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);
path_dirname = dirname(sched_filter_file_tmp);

- sprintf(path, "%s/%s/filter", path_dirname, sched_path);
+ ret = asprintf(&path, "%s/%s/filter", path_dirname, 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);
@@ -2325,10 +2317,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);

@@ -3984,6 +3975,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") +
@@ -3991,10 +3984,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);
}
}

@@ -4389,6 +4381,7 @@ void trace_record (int argc, char **argv)

for (;;) {
int option_index = 0;
+ int ret;
const char *opts;
static struct option long_options[] = {
{"date", no_argument, NULL, OPT_date},
@@ -4453,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;

diff --git a/trace-recorder.c b/trace-recorder.c
index 1b9d364..85150fd 100644
--- a/trace-recorder.c
+++ b/trace-recorder.c
@@ -156,14 +156,13 @@ tracecmd_create_buffer_recorder_fd2(int fd, int fd2, int cpu, unsigned flags,
recorder->fd1 = fd;
recorder->fd2 = fd2;

- path = malloc(strlen(buffer) + 40);
- if (!path)
- goto out_free;
-
if (flags & TRACECMD_RECORD_SNAPSHOT)
- sprintf(path, "%s/per_cpu/cpu%d/snapshot_raw", buffer, cpu);
+ ret = asprintf(&path, "%s/per_cpu/cpu%d/snapshot_raw", buffer, cpu);
else
- sprintf(path, "%s/per_cpu/cpu%d/trace_pipe_raw", buffer, cpu);
+ ret = asprintf(&path, "%s/per_cpu/cpu%d/trace_pipe_raw", buffer, cpu);
+ if (ret < 0)
+ goto out_free;
+
recorder->trace_fd = open(path, O_RDONLY);
if (recorder->trace_fd < 0)
goto out_free;
diff --git a/trace-show.c b/trace-show.c
index 14b786c..f13db31 100644
--- a/trace-show.c
+++ b/trace-show.c
@@ -154,11 +154,11 @@ void trace_show(int argc, char **argv)
}

if (buffer) {
- path = malloc(strlen(buffer) + strlen("instances//") +
- strlen(file) + 1);
- if (!path)
+ int ret;
+
+ ret = asprintf(&path, "instances/%s/%s", buffer, file);
+ if (ret < 0)
die("Failed to allocate instance path %s", file);
- sprintf(path, "instances/%s/%s", buffer, file);
file = path;
}

diff --git a/trace-split.c b/trace-split.c
index 87d43ad..5e4ac68 100644
--- a/trace-split.c
+++ b/trace-split.c
@@ -369,10 +369,11 @@ static double parse_file(struct tracecmd_input *handle,
die("Failed to allocate cpu_data for %d cpus", cpus);

for (cpu = 0; cpu < cpus; cpu++) {
- file = malloc(strlen(output_file) + 50);
- if (!file)
+ int ret;
+
+ ret = asprintf(&file, "%s/.tmp.%s.%d", dir, base, cpu);
+ if (ret < 0)
die("Failed to allocate file for %s %s %d", dir, base, cpu);
- sprintf(file, "%s/.tmp.%s.%d", dir, base, cpu);
fd = open(file, O_WRONLY | O_CREAT | O_TRUNC | O_LARGEFILE, 0644);
cpu_data[cpu].cpu = cpu;
cpu_data[cpu].fd = fd;
diff --git a/trace-stat.c b/trace-stat.c
index adbf3c3..fd16354 100644
--- a/trace-stat.c
+++ b/trace-stat.c
@@ -70,15 +70,16 @@ char *strstrip(char *str)
return s;
}

+/* FIXME: append_file() is duplicated and could be consolidated */
char *append_file(const char *dir, const char *name)
{
char *file;
+ int ret;

- file = malloc(strlen(dir) + strlen(name) + 2);
- if (!file)
+ ret = asprintf(&file, "%s/%s", dir, name);
+ if (ret < 0)
die("Failed to allocate %s/%s", dir, name);

- sprintf(file, "%s/%s", dir, name);
return file;
}

diff --git a/trace-util.c b/trace-util.c
index d464a61..45fa95a 100644
--- a/trace-util.c
+++ b/trace-util.c
@@ -85,14 +85,15 @@ char **trace_util_list_plugin_options(void)
for (reg = registered_options; reg; reg = reg->next) {
for (op = reg->options; op->name; op++) {
char *alias = op->plugin_alias ? op->plugin_alias : op->file;
+ int ret;

- name = malloc(strlen(op->name) + strlen(alias) + 2);
- if (!name) {
+ ret = asprintf(&name, "%s:%s", alias, op->name);
+ if (ret < 0) {
warning("Failed to allocate plugin option %s:%s",
alias, op->name);
break;
}
- sprintf(name, "%s:%s", alias, op->name);
+
list = realloc(list, count + 2);
if (!list) {
warning("Failed to allocate plugin list for %s", name);
@@ -614,14 +615,10 @@ static int load_plugin(struct pevent *pevent, const char *path,
void *handle;
int ret;

- plugin = malloc(strlen(path) + strlen(file) + 2);
- if (!plugin)
+ ret = asprintf(&plugin, "%s/%s", path, file);
+ if (ret < 0)
return -ENOMEM;

- strcpy(plugin, path);
- strcat(plugin, "/");
- strcat(plugin, file);
-
handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
if (!handle) {
warning("cound not load plugin '%s'\n%s\n",
@@ -707,7 +704,7 @@ char *tracecmd_find_tracing_dir(void)
char type[100];
int use_debug = 0;
FILE *fp;
-
+
if ((fp = fopen("/proc/mounts","r")) == NULL) {
warning("Can't open /proc/mounts for read");
return NULL;
@@ -749,16 +746,16 @@ char *tracecmd_find_tracing_dir(void)
free(debug_str);

if (use_debug) {
- tracing_dir = malloc(strlen(fspath) + 9);
- if (!tracing_dir)
- return NULL;
+ int ret;

- sprintf(tracing_dir, "%s/tracing", fspath);
+ ret = asprintf(&tracing_dir, "%s/tracing", fspath);
+ if (ret < 0)
+ return NULL;
} else {
tracing_dir = strdup(fspath);
if (!tracing_dir)
return NULL;
- }
+ }

return tracing_dir;
}
@@ -774,16 +771,15 @@ const char *tracecmd_get_tracing_dir(void)
return tracing_dir;
}

+/* FIXME: append_file() is duplicated and could be consolidated */
static char *append_file(const char *dir, const char *name)
{
char *file;
+ int ret;

- file = malloc(strlen(dir) + strlen(name) + 2);
- if (!file)
- return NULL;
+ ret = asprintf(&file, "%s/%s", dir, name);

- sprintf(file, "%s/%s", dir, name);
- return file;
+ return ret < 0 ? NULL : file;
}

/**
@@ -1389,7 +1385,8 @@ int trace_util_load_plugins(struct pevent *pevent, const char *suffix,
{
char *home;
char *path;
- char *envdir;
+ char *envdir;
+ int ret;

if (tracecmd_disable_plugins)
return -EBUSY;
@@ -1412,14 +1409,10 @@ int trace_util_load_plugins(struct pevent *pevent, const char *suffix,
if (!home)
return -EINVAL;

- path = malloc(strlen(home) + strlen(LOCAL_PLUGIN_DIR) + 2);
- if (!path)
+ ret = asprintf(&path, "%s/%s", home, LOCAL_PLUGIN_DIR);
+ if (ret < 0)
return -ENOMEM;

- strcpy(path, home);
- strcat(path, "/");
- strcat(path, LOCAL_PLUGIN_DIR);
-
trace_util_load_plugins_dir(pevent, suffix, path, load_plugin, data);

free(path);
@@ -1506,15 +1499,12 @@ static int read_options(struct pevent *pevent, const char *path,
int unload = 0;
char *plugin;
void *handle;
+ int ret;

- plugin = malloc(strlen(path) + strlen(file) + 2);
- if (!plugin)
+ ret = asprintf(&plugin, "%s/%s", path, file);
+ if (ret < 0)
return -ENOMEM;

- strcpy(plugin, path);
- strcat(plugin, "/");
- strcat(plugin, file);
-
handle = dlopen(plugin, RTLD_NOW | RTLD_GLOBAL);
if (!handle) {
warning("cound not load plugin '%s'\n%s\n",
@@ -1603,6 +1593,7 @@ char *tracecmd_get_tracing_file(const char *name)
{
static const char *tracing;
char *file;
+ int ret;

if (!tracing) {
tracing = tracecmd_find_tracing_dir();
@@ -1610,11 +1601,10 @@ char *tracecmd_get_tracing_file(const char *name)
return NULL;
}

- file = malloc(strlen(tracing) + strlen(name) + 2);
- if (!file)
+ ret = asprintf(&file, "%s/%s", tracing, name);
+ if (ret < 0)
return NULL;

- sprintf(file, "%s/%s", tracing, name);
return file;
}

--
2.13.3

2017-08-02 23:48:05

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH V3 1/2] trace-cmd record: use direname instead of custom code

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]>

Thanks. I'll be traveling for a bit and wont get to these before I
leave. Just a note. When sending patches, can you add to the subject,
something like I just did "trace-cmd record:" to let those know that
this is a trace-cmd patch (especially when sent to LKML). It also makes
it easier for me when I look for trace-cmd patches, as I will filter on
"trace-cmd" in the subject.

Thanks,

-- Steve

2017-08-14 17:14:24

by Steven Rostedt

[permalink] [raw]
Subject: Re: [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 <[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);

2017-08-14 17:33:44

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH V3 2/2] It makes the code clearer and less error prone.

On Thu, 3 Aug 2017 00:15:58 +0200
Federico Vaga <[email protected]> wrote:

Why did you change the subject? The previous patch had a much better
one: "trace-cmd: Use asprintf when possible"

Or was it the tool you used to send the patches that chopped it off?

A quick scan of the patch looks good. I'll look a bit deeper at it, and
if I don't find anything, I'll apply it (and fix the subject).

Thanks!

-- Steve

> clearer:
> - less code
> - the code is now using the same format to create strings dynamically
>
> less error prone:
> - no magic number +2 +9 +5 to compute the size
> - no copy&paste of the strings to compute the size and to concatenate
>
> The function `asprintf` is not POSIX standard but the program
> was already using it. Later it can be decided to use only POSIX
> functions, then we can easly replace all the `asprintf(3)` with a local
> implementation of that function.
>
> Signed-off-by: Federico Vaga <[email protected]>
> ---
> event-plugin.c | 24 ++++++++--------------
>

2017-08-15 07:25:15

by Federico Vaga

[permalink] [raw]
Subject: Re: [PATCH V3 2/2] It makes the code clearer and less error prone.

On Monday, August 14, 2017 7:33:41 PM CEST Steven Rostedt wrote:
> On Thu, 3 Aug 2017 00:15:58 +0200
> Federico Vaga <[email protected]> wrote:
>
> Why did you change the subject? The previous patch had a much better
> one: "trace-cmd: Use asprintf when possible"
>
> Or was it the tool you used to send the patches that chopped it off?

It was not my intention.
I used git send-email, but probably I did something wrong by accident. Sorry

>
> A quick scan of the patch looks good. I'll look a bit deeper at it, and
> if I don't find anything, I'll apply it (and fix the subject).

Ok, thank you

--
Federico Vaga
http://www.federicovaga.it/

2017-11-08 18:51:43

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH V3 2/2] It makes the code clearer and less error prone.

On Tue, 15 Aug 2017 09:25:10 +0200
Federico Vaga <[email protected]> wrote:

> On Monday, August 14, 2017 7:33:41 PM CEST Steven Rostedt wrote:
> > On Thu, 3 Aug 2017 00:15:58 +0200
> > Federico Vaga <[email protected]> wrote:
> >
> > Why did you change the subject? The previous patch had a much better
> > one: "trace-cmd: Use asprintf when possible"
> >
> > Or was it the tool you used to send the patches that chopped it off?
>
> It was not my intention.
> I used git send-email, but probably I did something wrong by accident. Sorry
>
> >
> > A quick scan of the patch looks good. I'll look a bit deeper at it, and
> > if I don't find anything, I'll apply it (and fix the subject).
>
> Ok, thank you
>

Finally got around to applying your patches. Sorry for the long
delay. :-/ I've been traveling too much lately.

-- Steve

From 1575781077781751341@xxx Tue Aug 15 07:25:42 +0000 2017
X-GM-THRID: 1574659662152626612
X-Gmail-Labels: Inbox,Category Forums