A couple of patches that try to make consistent the usage of `asprintf(3)`
and `show_instance_file()`.
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 | 56 +++++++++++++++++++++------------------------------
trace-recorder.c | 11 +++++-----
trace-show.c | 8 ++++----
trace-split.c | 7 ++++---
trace-stat.c | 7 ++++---
trace-util.c | 61 +++++++++++++++++++++++---------------------------------
10 files changed, 85 insertions(+), 114 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 c2fd26f..2fa601d 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(®, "^%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(®, "^%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 e2ab0db..4b8e813 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 1b55043..7155cfc 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -723,14 +723,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
@@ -744,17 +742,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);
@@ -2237,11 +2233,10 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
for (p = path + strlen(path) - 1; p > path; p--)
if (*p == '/')
break;
- *p = '\0';
- p = malloc(strlen(path) + strlen("/enable") + 1);
- if (!p)
+ *p = '\0'; /* FIXME is it ok ? */
+ ret = asprintf(&p, "%s/enable", path);
+ if (ret < 0)
die("Failed to allocate enable path for %s", path);
- sprintf(p, "%s/enable", path);
ret = stat(p, &st);
if (ret >= 0)
event->enable_file = p;
@@ -2249,10 +2244,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)
+ ret = asprintf(&p, "%s/trigger", path);
+ if (ret < 0)
die("Failed to allocate trigger path for %s", path);
- sprintf(p, "%s/trigger", path);
ret = stat(p, &st);
if (ret > 0)
die("trigger specified but not supported by this kernel");
@@ -2268,17 +2262,16 @@ static void make_sched_event(struct buffer_instance *instance,
{
char *path;
char *p;
+ int ret;
/* Do nothing if the event already exists */
if (*event)
return;
- path = malloc(strlen(sched->filter_file) + strlen(sched_path) + 2);
- if (!path)
+ ret = asprintf(&path, "%s", sched->filter_file);
+ if (ret < 0)
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);
@@ -2310,10 +2303,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);
@@ -3956,6 +3948,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") +
@@ -3963,10 +3957,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);
}
}
@@ -4357,7 +4350,7 @@ void trace_record (int argc, char **argv)
usage(argv);
for (;;) {
- int option_index = 0;
+ int option_index = 0, ret;
const char *opts;
static struct option long_options[] = {
{"date", no_argument, NULL, OPT_date},
@@ -4420,12 +4413,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..778c199 100644
--- a/trace-stat.c
+++ b/trace-stat.c
@@ -70,15 +70,16 @@ char *strstrip(char *str)
return s;
}
+/* FIXME repeated */
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 fbf8cea..29a7079 100644
--- a/trace-util.c
+++ b/trace-util.c
@@ -88,14 +88,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);
@@ -617,14 +618,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",
@@ -710,7 +707,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;
@@ -752,16 +749,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;
}
@@ -780,13 +777,11 @@ const char *tracecmd_get_tracing_dir(void)
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;
}
/**
@@ -1392,7 +1387,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;
@@ -1415,14 +1411,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);
@@ -1509,15 +1501,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",
@@ -1606,6 +1595,7 @@ char *tracecmd_get_tracing_file(const char *name)
{
static const char *tracing;
char *file;
+ int ret;
if (!tracing) {
tracing = tracecmd_find_tracing_dir();
@@ -1613,11 +1603,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.9.4
show_file(name) and show_instance_file(&top_instance, name) are
equivalent.
Remove the show_file() function in order to have a single function for
this task.
Signed-off-by: Federico Vaga <[email protected]>
---
trace-list.c | 21 ++++++---------------
trace-local.h | 2 --
trace-show.c | 18 ++----------------
trace-snapshot.c | 2 +-
4 files changed, 9 insertions(+), 34 deletions(-)
diff --git a/trace-list.c b/trace-list.c
index 32b19bc..933397f 100644
--- a/trace-list.c
+++ b/trace-list.c
@@ -59,15 +59,6 @@ enum {
};
-void show_file(const char *name)
-{
- char *path;
-
- path = tracecmd_get_tracing_file(name);
- dump_file_content(path);
- tracecmd_put_tracing_file(path);
-}
-
typedef int (*process_file_func)(char *buf, int len);
static void process_file_re(process_file_func func,
@@ -83,7 +74,7 @@ static void process_file_re(process_file_func func,
/* Just in case :-p */
if (!re || l == 0) {
- show_file(name);
+ show_instance_file( &top_instance, name);
return;
}
@@ -258,25 +249,25 @@ static void show_events(const char *eventre, int flags)
else
show_file_re("available_events", eventre);
} else
- show_file("available_events");
+ show_instance_file(&top_instance, "available_events");
}
static void show_tracers(void)
{
- show_file("available_tracers");
+ show_instance_file(&top_instance, "available_tracers");
}
static void show_options(void)
{
- show_file("trace_options");
+ show_instance_file(&top_instance, "trace_options");
}
static void show_clocks(void)
{
- show_file("trace_clock");
+ show_instance_file(&top_instance, "trace_clock");
}
@@ -285,7 +276,7 @@ static void show_functions(const char *funcre)
if (funcre)
show_file_re("available_filter_functions", funcre);
else
- show_file("available_filter_functions");
+ show_instance_file(&top_instance, "available_filter_functions");
}
diff --git a/trace-local.h b/trace-local.h
index fa987bc..0e4ce61 100644
--- a/trace-local.h
+++ b/trace-local.h
@@ -50,8 +50,6 @@ struct pid_record_data {
struct pevent_record *record;
};
-void show_file(const char *name);
-
struct tracecmd_input *read_trace_header(const char *file);
int read_trace_files(void);
diff --git a/trace-show.c b/trace-show.c
index f13db31..7d5a805 100644
--- a/trace-show.c
+++ b/trace-show.c
@@ -38,12 +38,10 @@ enum {
void trace_show(int argc, char **argv)
{
- const char *buffer = NULL;
const char *file = "trace";
const char *cpu = NULL;
struct buffer_instance *instance = &top_instance;
char cpu_path[128];
- char *path;
int snap = 0;
int pipe = 0;
int show_name = 0;
@@ -72,9 +70,8 @@ void trace_show(int argc, char **argv)
usage(argv);
break;
case 'B':
- if (buffer)
+ if (instance != &top_instance)
die("Can only show one buffer at a time");
- buffer = optarg;
instance = create_instance(optarg);
if (!instance)
die("Failed to create instance");
@@ -153,24 +150,13 @@ void trace_show(int argc, char **argv)
file = cpu_path;
}
- if (buffer) {
- int ret;
-
- ret = asprintf(&path, "instances/%s/%s", buffer, file);
- if (ret < 0)
- die("Failed to allocate instance path %s", file);
- file = path;
- }
-
if (show_name) {
char *name;
name = tracecmd_get_tracing_file(file);
printf("%s\n", name);
tracecmd_put_tracing_file(name);
}
- show_file(file);
- if (buffer)
- free(path);
+ show_instance_file(instance, file);
return;
}
diff --git a/trace-snapshot.c b/trace-snapshot.c
index 771b065..1e11438 100644
--- a/trace-snapshot.c
+++ b/trace-snapshot.c
@@ -113,7 +113,7 @@ void trace_snapshot (int argc, char **argv)
tracecmd_put_tracing_file(name);
if (!reset_snap && !take_snap && !free_snap) {
- show_file(file);
+ show_instance_file(&top_instance, file);
exit(0);
}
--
2.9.4
On Mon, 5 Jun 2017 11:31:17 +0200
Federico Vaga <[email protected]> wrote:
Hi Federico,
I finally got around to looking at these. Sorry for the really slow
reply, but I had a bunch of kernel work I needed to get done before
digging again into trace-cmd.
> 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
I like this!
>
> 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.
Yes, if we decide not to use GNU specific code, then we can implement
our own version.
>
> 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 | 56 +++++++++++++++++++++------------------------------
> trace-recorder.c | 11 +++++-----
> trace-show.c | 8 ++++----
> trace-split.c | 7 ++++---
> trace-stat.c | 7 ++++---
> trace-util.c | 61 +++++++++++++++++++++++---------------------------------
> 10 files changed, 85 insertions(+), 114 deletions(-)
[...]
> @@ -2237,11 +2233,10 @@ create_event(struct buffer_instance *instance, char *path, struct event_list *ol
> for (p = path + strlen(path) - 1; p > path; p--)
> if (*p == '/')
> break;
> - *p = '\0';
> - p = malloc(strlen(path) + strlen("/enable") + 1);
> - if (!p)
> + *p = '\0'; /* FIXME is it ok ? */
I'm going to remove the comment. If you look at the code above it, You
will see that 'p' is used to find the last instance of '/' in path.
Then the *p = '\0' is used to remove it.
> + ret = asprintf(&p, "%s/enable", path);
> + if (ret < 0)
> die("Failed to allocate enable path for %s", path);
> - sprintf(p, "%s/enable", path);
> ret = stat(p, &st);
> if (ret >= 0)
> event->enable_file = p;
> @@ -2249,10 +2244,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)
> + ret = asprintf(&p, "%s/trigger", path);
> + if (ret < 0)
> die("Failed to allocate trigger path for %s", path);
> - sprintf(p, "%s/trigger", path);
> ret = stat(p, &st);
> if (ret > 0)
> die("trigger specified but not supported by this kernel");
> @@ -2268,17 +2262,16 @@ static void make_sched_event(struct buffer_instance *instance,
> {
> char *path;
> char *p;
> + int ret;
>
> /* Do nothing if the event already exists */
> if (*event)
> return;
>
> - path = malloc(strlen(sched->filter_file) + strlen(sched_path) + 2);
> - if (!path)
> + ret = asprintf(&path, "%s", sched->filter_file);
Now this part is not correct. It is actually buggy. If you noticed the
malloc, it allocates strlen(sched->filter_file) + strlen(sched_path) + 2.
Which is probably a little more than needed.
> + if (ret < 0)
> die("Failed to allocate path for %s", sched_path);
>
> - sprintf(path, "%s", sched->filter_file);
> -
Here I copy in the sched->filter_file which is the path I want, but I
don't want the "/filter" part. So it is cut off below and the
sched_path is copied in.
Hmm, what would be better is to:
asprintf(path, "%s/%s", dirname(sched->filter_file), sched_path);
And remove all this open coded stuff. The same can probably be done
above where you had the "fixme".
> /* Remove the /filter from filter file */
> p = path + strlen(path) - strlen("filter");
> sprintf(p, "%s/filter", sched_path);
> @@ -2310,10 +2303,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);
>
> @@ -3956,6 +3948,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") +
> @@ -3963,10 +3957,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);
> }
> }
>
> @@ -4357,7 +4350,7 @@ void trace_record (int argc, char **argv)
> usage(argv);
>
> for (;;) {
> - int option_index = 0;
> + int option_index = 0, ret;
> const char *opts;
> static struct option long_options[] = {
> {"date", no_argument, NULL, OPT_date},
> @@ -4420,12 +4413,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..778c199 100644
> --- a/trace-stat.c
> +++ b/trace-stat.c
> @@ -70,15 +70,16 @@ char *strstrip(char *str)
> return s;
> }
>
> +/* FIXME repeated */
What do you mean by that?
> 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 fbf8cea..29a7079 100644
> --- a/trace-util.c
> +++ b/trace-util.c
> @@ -88,14 +88,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);
> @@ -617,14 +618,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",
> @@ -710,7 +707,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;
> @@ -752,16 +749,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;
> }
> @@ -780,13 +777,11 @@ const char *tracecmd_get_tracing_dir(void)
> 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;
> }
>
> /**
> @@ -1392,7 +1387,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;
> @@ -1415,14 +1411,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);
> @@ -1509,15 +1501,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",
> @@ -1606,6 +1595,7 @@ char *tracecmd_get_tracing_file(const char *name)
> {
> static const char *tracing;
> char *file;
> + int ret;
>
> if (!tracing) {
> tracing = tracecmd_find_tracing_dir();
> @@ -1613,11 +1603,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;
> }
>
The rest looks good. Do you want to send an updated patch, or do you
want me to fix the above?
-- Steve
On Mon, 5 Jun 2017 11:31:18 +0200
Federico Vaga <[email protected]> wrote:
> show_file(name) and show_instance_file(&top_instance, name) are
> equivalent.
>
> Remove the show_file() function in order to have a single function for
> this task.
Actually I find nothing wrong with having a helper function like this.
IIRC, show_file() was first, and then show_instance_file() came later.
There's some files that only exist for the top_instance, and I like the
fact that this is annotated that way.
I'm curious to know what the benefit of removing show_file() is?
-- Steve
>
> Signed-off-by: Federico Vaga <[email protected]>
> ---
> trace-list.c | 21 ++++++---------------
> trace-local.h | 2 --
> trace-show.c | 18 ++----------------
> trace-snapshot.c | 2 +-
> 4 files changed, 9 insertions(+), 34 deletions(-)
\
On Friday, July 7, 2017 12:25:32 AM CEST Steven Rostedt wrote:
> On Mon, 5 Jun 2017 11:31:17 +0200
> Federico Vaga <[email protected]> wrote:
>
> Hi Federico,
>
> I finally got around to looking at these. Sorry for the really slow
> reply, but I had a bunch of kernel work I needed to get done before
> digging again into trace-cmd.
>
> > 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
>
> I like this!
>
> > 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.
>
> Yes, if we decide not to use GNU specific code, then we can implement
> our own version.
>
> > 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 | 56 +++++++++++++++++++++------------------------------
> > trace-recorder.c | 11 +++++-----
> > trace-show.c | 8 ++++----
> > trace-split.c | 7 ++++---
> > trace-stat.c | 7 ++++---
> > trace-util.c | 61
> > +++++++++++++++++++++++--------------------------------- 10 files
> > changed, 85 insertions(+), 114 deletions(-)
>
> [...]
>
> > @@ -2237,11 +2233,10 @@ create_event(struct buffer_instance *instance,
> > char *path, struct event_list *ol>
> > for (p = path + strlen(path) - 1; p > path; p--)
> >
> > if (*p == '/')
> >
> > break;
> >
> > - *p = '\0';
> > - p = malloc(strlen(path) + strlen("/enable") + 1);
> > - if (!p)
> > + *p = '\0'; /* FIXME is it ok ? */
>
> I'm going to remove the comment. If you look at the code above it, You
> will see that 'p' is used to find the last instance of '/' in path.
> Then the *p = '\0' is used to remove it.
At the beginning the logic was not clear to me, in particular "why is it doing
this?", then I understood by having a look at the usage of `create_event()`
but I forget to remove the FIXME.
But still, it is not immediately obvious why we need this without reading how
the function has been used.
Answer to the question:
we need it because when we call `create_event()` we pass the path to the
filter file, and not the path to the event directory.
In my opinion we should pass the path to the event directory, and from this we
can build the event_list's paths. To me, it sounds more correct for a function
named `create_event()`, rather than:
- taking a path to a specific event file,
- deduce the event directory,
- build the path for the other event files,
>
> > + ret = asprintf(&p, "%s/enable", path);
> > + if (ret < 0)
> >
> > die("Failed to allocate enable path for %s", path);
> >
> > - sprintf(p, "%s/enable", path);
> >
> > ret = stat(p, &st);
> > if (ret >= 0)
> >
> > event->enable_file = p;
> >
> > @@ -2249,10 +2244,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)
> > + ret = asprintf(&p, "%s/trigger", path);
> > + if (ret < 0)
> >
> > die("Failed to allocate trigger path for %s", path);
> >
> > - sprintf(p, "%s/trigger", path);
> >
> > ret = stat(p, &st);
> > if (ret > 0)
> >
> > die("trigger specified but not supported by this kernel");
> >
> > @@ -2268,17 +2262,16 @@ static void make_sched_event(struct
> > buffer_instance *instance,>
> > {
> >
> > char *path;
> > char *p;
> >
> > + int ret;
> >
> > /* Do nothing if the event already exists */
> > if (*event)
> >
> > return;
> >
> > - path = malloc(strlen(sched->filter_file) + strlen(sched_path) + 2);
> > - if (!path)
> > + ret = asprintf(&path, "%s", sched->filter_file);
>
> Now this part is not correct. It is actually buggy. If you noticed the
> malloc, it allocates strlen(sched->filter_file) + strlen(sched_path) + 2.
> Which is probably a little more than needed.
Yes, you are right.
>
> > + if (ret < 0)
> >
> > die("Failed to allocate path for %s", sched_path);
> >
> > - sprintf(path, "%s", sched->filter_file);
> > -
>
> Here I copy in the sched->filter_file which is the path I want, but I
> don't want the "/filter" part. So it is cut off below and the
> sched_path is copied in.
>
> Hmm, what would be better is to:
>
> asprintf(path, "%s/%s", dirname(sched->filter_file), sched_path);
>
> And remove all this open coded stuff. The same can probably be done
> above where you had the "fixme".
yes
> > /* Remove the /filter from filter file */
> > p = path + strlen(path) - strlen("filter");
> > sprintf(p, "%s/filter", sched_path);
> >
> > @@ -2310,10 +2303,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);
> >
> > @@ -3956,6 +3948,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") +
> >
> > @@ -3963,10 +3957,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);
> >
> > }
> >
> > }
> >
> > @@ -4357,7 +4350,7 @@ void trace_record (int argc, char **argv)
> >
> > usage(argv);
> >
> > for (;;) {
> >
> > - int option_index = 0;
> > + int option_index = 0, ret;
> >
> > const char *opts;
> > static struct option long_options[] = {
> >
> > {"date", no_argument, NULL, OPT_date},
> >
> > @@ -4420,12 +4413,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..778c199 100644
> > --- a/trace-stat.c
> > +++ b/trace-stat.c
> > @@ -70,15 +70,16 @@ char *strstrip(char *str)
> >
> > return s;
> >
> > }
> >
> > +/* FIXME repeated */
>
> What do you mean by that?
>
> > 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 fbf8cea..29a7079 100644
> > --- a/trace-util.c
> > +++ b/trace-util.c
> > @@ -88,14 +88,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);
> >
> > @@ -617,14 +618,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",
> >
> > @@ -710,7 +707,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;
> >
> > @@ -752,16 +749,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;
> >
> > }
> >
> > @@ -780,13 +777,11 @@ const char *tracecmd_get_tracing_dir(void)
> >
> > 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;
> >
> > }
> >
> > /**
> >
> > @@ -1392,7 +1387,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;
> >
> > @@ -1415,14 +1411,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);
> >
> > @@ -1509,15 +1501,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",
> >
> > @@ -1606,6 +1595,7 @@ char *tracecmd_get_tracing_file(const char *name)
> >
> > {
> >
> > static const char *tracing;
> > char *file;
> >
> > + int ret;
> >
> > if (!tracing) {
> >
> > tracing = tracecmd_find_tracing_dir();
> >
> > @@ -1613,11 +1603,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;
> >
> > }
>
> The rest looks good. Do you want to send an updated patch, or do you
> want me to fix the above?
I can send an updated patch, but I do not know when (weeks). It is a super
easy patch but I'm really busy and tired at the moment. Sorry :/
If you have time and you want to do it, please go ahead. Otherwise I will do
it when I will have free time :)
--
Federico Vaga
http://www.federicovaga.it/
On Friday, July 7, 2017 12:29:35 AM CEST Steven Rostedt wrote:
> On Mon, 5 Jun 2017 11:31:18 +0200
>
> Federico Vaga <[email protected]> wrote:
> > show_file(name) and show_instance_file(&top_instance, name) are
> > equivalent.
> >
> > Remove the show_file() function in order to have a single function for
> > this task.
>
> Actually I find nothing wrong with having a helper function like this.
> IIRC, show_file() was first, and then show_instance_file() came later.
> There's some files that only exist for the top_instance, and I like the
> fact that this is annotated that way.
>
> I'm curious to know what the benefit of removing show_file() is?
The show_file(name) and show_instance_file(&top_instance, name) are
equivalent: they do the same thing. By removing `show_file` the developers are
forced to use always the same function and being explicit about the instance
they want to use.
The name `show_file()` is so generic that does not implies automatically that
we are accessing the top_instance. This is not even clear by reading the
implementation; people must read the other functions used in `show_file()` to
understand that their instance scope is always 'top_instance'.
So, in my opinion, it makes the code easier to read and more explicit in what
is doing without too much effort.
> -- Steve
>
> > Signed-off-by: Federico Vaga <[email protected]>
> > ---
> >
> > trace-list.c | 21 ++++++---------------
> > trace-local.h | 2 --
> > trace-show.c | 18 ++----------------
> > trace-snapshot.c | 2 +-
> > 4 files changed, 9 insertions(+), 34 deletions(-)
>
> \
--
Federico Vaga
http://www.federicovaga.it/
Hi Steven,
I found some free time and unfortunately I can't enjoy the sun, so here I am
on this patch.
Before submitting the V2, one comment (inline)
On Monday, July 10, 2017 2:08:41 AM CEST Federico Vaga wrote:
> On Friday, July 7, 2017 12:25:32 AM CEST Steven Rostedt wrote:
> > On Mon, 5 Jun 2017 11:31:17 +0200
> > Federico Vaga <[email protected]> wrote:
> >
> > Hi Federico,
> >
> > I finally got around to looking at these. Sorry for the really slow
> > reply, but I had a bunch of kernel work I needed to get done before
> > digging again into trace-cmd.
> >
> > > 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
> >
> > I like this!
> >
> > > 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.
> >
> > Yes, if we decide not to use GNU specific code, then we can implement
> > our own version.
> >
> > > 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 | 56
> > > +++++++++++++++++++++------------------------------
> > > trace-recorder.c | 11 +++++-----
> > > trace-show.c | 8 ++++----
> > > trace-split.c | 7 ++++---
> > > trace-stat.c | 7 ++++---
> > > trace-util.c | 61
> > > +++++++++++++++++++++++--------------------------------- 10 files
> > > changed, 85 insertions(+), 114 deletions(-)
> >
> > [...]
> >
> > > @@ -2237,11 +2233,10 @@ create_event(struct buffer_instance *instance,
> > > char *path, struct event_list *ol>
> > >
> > > for (p = path + strlen(path) - 1; p > path; p--)
> > >
> > > if (*p == '/')
> > >
> > > break;
> > >
> > > - *p = '\0';
> > > - p = malloc(strlen(path) + strlen("/enable") + 1);
> > > - if (!p)
> > > + *p = '\0'; /* FIXME is it ok ? */
> >
> > I'm going to remove the comment. If you look at the code above it, You
> > will see that 'p' is used to find the last instance of '/' in path.
> > Then the *p = '\0' is used to remove it.
>
> At the beginning the logic was not clear to me, in particular "why is it
> doing this?", then I understood by having a look at the usage of
> `create_event()` but I forget to remove the FIXME.
>
>
> But still, it is not immediately obvious why we need this without reading
> how the function has been used.
>
> Answer to the question:
> we need it because when we call `create_event()` we pass the path to the
> filter file, and not the path to the event directory.
>
>
> In my opinion we should pass the path to the event directory, and from this
> we can build the event_list's paths. To me, it sounds more correct for a
> function named `create_event()`, rather than:
> - taking a path to a specific event file,
> - deduce the event directory,
> - build the path for the other event files,
While I was fixing my patch according to what we said last time, I think I
recall what was my true original meaning of "/* FIXME is it ok ? */". (What I
wrote last time is still valid anyway)
The questions comes by the fact that this line:
*p = '\0'; /* FIXME is it ok ? */
changes the input parameter by cutting it (it does what dirname() does).
So, "is it ok (to cut the input string)?". According to the internal usage,
when a function uses `create_event()`, it passes a generated string that then
is not used anymore. So, nobody cares if this string has been manipulated by
create_event().
I think that this should not happen. So I will propose a patch V2 where I use
`dirname()` as suggested but on local duplicate using `strdup()`. This
guarantee (even if it is not necessary) that the input string does not change.
>
> > > + ret = asprintf(&p, "%s/enable", path);
> > > + if (ret < 0)
> > >
> > > die("Failed to allocate enable path for %s", path);
> > >
> > > - sprintf(p, "%s/enable", path);
> > >
> > > ret = stat(p, &st);
> > > if (ret >= 0)
> > >
> > > event->enable_file = p;
> > >
> > > @@ -2249,10 +2244,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)
> > > + ret = asprintf(&p, "%s/trigger", path);
> > > + if (ret < 0)
> > >
> > > die("Failed to allocate trigger path for %s", path);
> > >
> > > - sprintf(p, "%s/trigger", path);
> > >
> > > ret = stat(p, &st);
> > > if (ret > 0)
> > >
> > > die("trigger specified but not supported by this kernel");
> > >
> > > @@ -2268,17 +2262,16 @@ static void make_sched_event(struct
> > > buffer_instance *instance,>
> > >
> > > {
> > >
> > > char *path;
> > > char *p;
> > >
> > > + int ret;
> > >
> > > /* Do nothing if the event already exists */
> > > if (*event)
> > >
> > > return;
> > >
> > > - path = malloc(strlen(sched->filter_file) + strlen(sched_path) + 2);
> > > - if (!path)
> > > + ret = asprintf(&path, "%s", sched->filter_file);
> >
> > Now this part is not correct. It is actually buggy. If you noticed the
> > malloc, it allocates strlen(sched->filter_file) + strlen(sched_path) + 2.
> > Which is probably a little more than needed.
>
> Yes, you are right.
>
> > > + if (ret < 0)
> > >
> > > die("Failed to allocate path for %s", sched_path);
> > >
> > > - sprintf(path, "%s", sched->filter_file);
> > > -
> >
> > Here I copy in the sched->filter_file which is the path I want, but I
> > don't want the "/filter" part. So it is cut off below and the
> > sched_path is copied in.
> >
> > Hmm, what would be better is to:
> > asprintf(path, "%s/%s", dirname(sched->filter_file), sched_path);
> >
> > And remove all this open coded stuff. The same can probably be done
> > above where you had the "fixme".
>
> yes
>
> > > /* Remove the /filter from filter file */
> > > p = path + strlen(path) - strlen("filter");
> > > sprintf(p, "%s/filter", sched_path);
> > >
> > > @@ -2310,10 +2303,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);
> > >
> > > @@ -3956,6 +3948,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") +
> > >
> > > @@ -3963,10 +3957,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);
> > >
> > > }
> > >
> > > }
> > >
> > > @@ -4357,7 +4350,7 @@ void trace_record (int argc, char **argv)
> > >
> > > usage(argv);
> > >
> > > for (;;) {
> > >
> > > - int option_index = 0;
> > > + int option_index = 0, ret;
> > >
> > > const char *opts;
> > > static struct option long_options[] = {
> > >
> > > {"date", no_argument, NULL, OPT_date},
> > >
> > > @@ -4420,12 +4413,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..778c199 100644
> > > --- a/trace-stat.c
> > > +++ b/trace-stat.c
> > > @@ -70,15 +70,16 @@ char *strstrip(char *str)
> > >
> > > return s;
> > >
> > > }
> > >
> > > +/* FIXME repeated */
> >
> > What do you mean by that?
I forget to answer to this point last time.
The function `append_file()` is implemented twice in trace-stat.c and trace-
util.c
I noticed that those two files are included in different binaries (trace-cmd
and the libraries). I just put a note because instead of having multiple
implementation we can have just one in a file that gets included where is
needed. Of course, if it is just for such a simple function it does not make
much sense right now. But if we can group all the internal helpers I believe
is better.
I will remove the fixme from the V2 patch
> >
> > > 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 fbf8cea..29a7079 100644
> > > --- a/trace-util.c
> > > +++ b/trace-util.c
> > > @@ -88,14 +88,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);
> > >
> > > @@ -617,14 +618,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",
> > >
> > > @@ -710,7 +707,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;
> > >
> > > @@ -752,16 +749,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;
> > >
> > > }
> > >
> > > @@ -780,13 +777,11 @@ const char *tracecmd_get_tracing_dir(void)
> > >
> > > 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;
> > >
> > > }
> > >
> > > /**
> > >
> > > @@ -1392,7 +1387,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;
> > >
> > > @@ -1415,14 +1411,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);
> > >
> > > @@ -1509,15 +1501,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",
> > >
> > > @@ -1606,6 +1595,7 @@ char *tracecmd_get_tracing_file(const char *name)
> > >
> > > {
> > >
> > > static const char *tracing;
> > > char *file;
> > >
> > > + int ret;
> > >
> > > if (!tracing) {
> > >
> > > tracing = tracecmd_find_tracing_dir();
> > >
> > > @@ -1613,11 +1603,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;
> > >
> > > }
> >
> > The rest looks good. Do you want to send an updated patch, or do you
> > want me to fix the above?
>
> I can send an updated patch, but I do not know when (weeks). It is a super
> easy patch but I'm really busy and tired at the moment. Sorry :/
>
> If you have time and you want to do it, please go ahead. Otherwise I will do
> it when I will have free time :)
--
Federico Vaga
http://www.federicovaga.it/
On Tue, 25 Jul 2017 16:21:11 +0200
Federico Vaga <[email protected]> wrote:
> Hi Steven,
>
> I found some free time and unfortunately I can't enjoy the sun, so here I am
> on this patch.
> Before submitting the V2, one comment (inline)
Ah you caught me in a middle of a very busy traveling week.
> > But still, it is not immediately obvious why we need this without reading
> > how the function has been used.
> >
> > Answer to the question:
> > we need it because when we call `create_event()` we pass the path to the
> > filter file, and not the path to the event directory.
> >
> >
> > In my opinion we should pass the path to the event directory, and from this
> > we can build the event_list's paths. To me, it sounds more correct for a
> > function named `create_event()`, rather than:
> > - taking a path to a specific event file,
> > - deduce the event directory,
> > - build the path for the other event files,
>
> While I was fixing my patch according to what we said last time, I think I
> recall what was my true original meaning of "/* FIXME is it ok ? */". (What I
> wrote last time is still valid anyway)
>
> The questions comes by the fact that this line:
>
> *p = '\0'; /* FIXME is it ok ? */
>
> changes the input parameter by cutting it (it does what dirname() does).
> So, "is it ok (to cut the input string)?". According to the internal usage,
> when a function uses `create_event()`, it passes a generated string that then
> is not used anymore. So, nobody cares if this string has been manipulated by
> create_event().
>
> I think that this should not happen. So I will propose a patch V2 where I use
> `dirname()` as suggested but on local duplicate using `strdup()`. This
> guarantee (even if it is not necessary) that the input string does not change.
>
That's a waste. The path parameter is not "const" which means that it
*can* be modified. When an input string should not be modified, then it
is documented by making it a const char* type.
Don't bother making a local out of it. If you still feel uneasy about
it, simply add a comment to the start of the function that the path
variable is modified.
> > > > diff --git a/trace-stat.c b/trace-stat.c
> > > > index adbf3c3..778c199 100644
> > > > --- a/trace-stat.c
> > > > +++ b/trace-stat.c
> > > > @@ -70,15 +70,16 @@ char *strstrip(char *str)
> > > >
> > > > return s;
> > > >
> > > > }
> > > >
> > > > +/* FIXME repeated */
> > >
> > > What do you mean by that?
>
> I forget to answer to this point last time.
>
> The function `append_file()` is implemented twice in trace-stat.c and trace-
> util.c
>
> I noticed that those two files are included in different binaries (trace-cmd
> and the libraries). I just put a note because instead of having multiple
> implementation we can have just one in a file that gets included where is
> needed. Of course, if it is just for such a simple function it does not make
> much sense right now. But if we can group all the internal helpers I believe
> is better.
>
> I will remove the fixme from the V2 patch
Or you can keep the comment, but make it better. That is:
/* FIXME: append_file() is duplicated and could be consolidated */
That way, it's self explanatory, and not confuse people even more ;-)
-- Steve
>
> > >
> > > > 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;
> > > >
> > > > }
> > > >
On Mon, 10 Jul 2017 02:15:35 +0200
Federico Vaga <[email protected]> wrote:
> On Friday, July 7, 2017 12:29:35 AM CEST Steven Rostedt wrote:
> > On Mon, 5 Jun 2017 11:31:18 +0200
> >
> > Federico Vaga <[email protected]> wrote:
> > > show_file(name) and show_instance_file(&top_instance, name) are
> > > equivalent.
> > >
> > > Remove the show_file() function in order to have a single function for
> > > this task.
> >
> > Actually I find nothing wrong with having a helper function like this.
> > IIRC, show_file() was first, and then show_instance_file() came later.
> > There's some files that only exist for the top_instance, and I like the
> > fact that this is annotated that way.
> >
> > I'm curious to know what the benefit of removing show_file() is?
>
> The show_file(name) and show_instance_file(&top_instance, name) are
> equivalent: they do the same thing. By removing `show_file` the developers are
> forced to use always the same function and being explicit about the instance
> they want to use.
>
> The name `show_file()` is so generic that does not implies automatically that
> we are accessing the top_instance. This is not even clear by reading the
> implementation; people must read the other functions used in `show_file()` to
> understand that their instance scope is always 'top_instance'.
>
> So, in my opinion, it makes the code easier to read and more explicit in what
> is doing without too much effort.
>
Just an FYI. You'll find lots of these types of helper functions in the
Linux Kernel. As I'm a Linux Kernel developer, I prefer them ;-)
-- Steve
Hi Steven,
On Mon, Jul 31, 2017 at 03:33:40PM -0400, Steven Rostedt wrote:
> On Tue, 25 Jul 2017 16:21:11 +0200
> Federico Vaga <[email protected]> wrote:
>> I found some free time and unfortunately I can't enjoy the sun, so
>> here I am
>> on this patch.
>> Before submitting the V2, one comment (inline)
>
> Ah you caught me in a middle of a very busy traveling week.
take your time then ;)
>> > But still, it is not immediately obvious why we need this without reading
>> > how the function has been used.
>> >
>> > Answer to the question:
>> > we need it because when we call `create_event()` we pass the path to the
>> > filter file, and not the path to the event directory.
>> >
>> >
>> > In my opinion we should pass the path to the event directory, and from this
>> > we can build the event_list's paths. To me, it sounds more correct for a
>> > function named `create_event()`, rather than:
>> > - taking a path to a specific event file,
>> > - deduce the event directory,
>> > - build the path for the other event files,
>>
>> While I was fixing my patch according to what we said last time, I
>> think I
>> recall what was my true original meaning of "/* FIXME is it ok ? */".
>> (What I
>> wrote last time is still valid anyway)
>>
>> The questions comes by the fact that this line:
>>
>> *p = '\0'; /* FIXME is it ok ? */
>>
>> changes the input parameter by cutting it (it does what dirname()
>> does).
>> So, "is it ok (to cut the input string)?". According to the internal
>> usage,
>> when a function uses `create_event()`, it passes a generated string
>> that then
>> is not used anymore. So, nobody cares if this string has been
>> manipulated by
>> create_event().
>>
>> I think that this should not happen. So I will propose a patch V2
>> where I use
>> `dirname()` as suggested but on local duplicate using `strdup()`. This
>> guarantee (even if it is not necessary) that the input string does not
>> change.
>>
>
> That's a waste. The path parameter is not "const" which means that it
> *can* be modified. When an input string should not be modified, then it
> is documented by making it a const char* type.
Indeed I was thinking to make it `const` as well
> Don't bother making a local out of it. If you still feel uneasy about
> it, simply add a comment to the start of the function that the path
> variable is modified.
It is not that I feel uneasy with that and yes it is a waste of
resources:
I agree. But since it is not
I just tend to be verbose when I see something that can have multiple
interpretations. Especially when, potentially, many heads/hands will
touch
the code.
For me a small comment that clarify that the input string (of course,
because
it is not `const`) will be modified (because it does) it is enough. So
that
the string will not be used accidentally in the functions that make use
of `create_event()`. I just believe that it is easy to fall into these
traps
(even if the lack of `const` should ring a bell).
I will propose a comment.
>> > > > diff --git a/trace-stat.c b/trace-stat.c
>> > > > index adbf3c3..778c199 100644
>> > > > --- a/trace-stat.c
>> > > > +++ b/trace-stat.c
>> > > > @@ -70,15 +70,16 @@ char *strstrip(char *str)
>> > > >
>> > > > return s;
>> > > >
>> > > > }
>> > > >
>> > > > +/* FIXME repeated */
>> > >
>> > > What do you mean by that?
>>
>> I forget to answer to this point last time.
>>
>> The function `append_file()` is implemented twice in trace-stat.c and
>> trace-
>> util.c
>>
>> I noticed that those two files are included in different binaries
>> (trace-cmd
>> and the libraries). I just put a note because instead of having
>> multiple
>> implementation we can have just one in a file that gets included where
>> is
>> needed. Of course, if it is just for such a simple function it does
>> not make
>> much sense right now. But if we can group all the internal helpers I
>> believe
>> is better.
>>
>> I will remove the fixme from the V2 patch
>
> Or you can keep the comment, but make it better. That is:
>
> /* FIXME: append_file() is duplicated and could be consolidated */
>
> That way, it's self explanatory, and not confuse people even more ;-)
ACK
On Mon, Jul 31, 2017 at 03:35:39PM -0400, Steven Rostedt wrote:
> On Mon, 10 Jul 2017 02:15:35 +0200
> Federico Vaga <[email protected]> wrote:
>
>> On Friday, July 7, 2017 12:29:35 AM CEST Steven Rostedt wrote:
>> > On Mon, 5 Jun 2017 11:31:18 +0200
>> >
>> > Federico Vaga <[email protected]> wrote:
>> > > show_file(name) and show_instance_file(&top_instance, name) are
>> > > equivalent.
>> > >
>> > > Remove the show_file() function in order to have a single function for
>> > > this task.
>> >
>> > Actually I find nothing wrong with having a helper function like this.
>> > IIRC, show_file() was first, and then show_instance_file() came later.
>> > There's some files that only exist for the top_instance, and I like the
>> > fact that this is annotated that way.
>> >
>> > I'm curious to know what the benefit of removing show_file() is?
>>
>> The show_file(name) and show_instance_file(&top_instance, name) are
>> equivalent: they do the same thing. By removing `show_file` the
>> developers are
>> forced to use always the same function and being explicit about the
>> instance
>> they want to use.
>>
>> The name `show_file()` is so generic that does not implies
>> automatically that
>> we are accessing the top_instance. This is not even clear by reading
>> the
>> implementation; people must read the other functions used in
>> `show_file()` to
>> understand that their instance scope is always 'top_instance'.
>>
>> So, in my opinion, it makes the code easier to read and more explicit
>> in what
>> is doing without too much effort.
>>
>
> Just an FYI. You'll find lots of these types of helper functions in the
> Linux Kernel. As I'm a Linux Kernel developer, I prefer them ;-)
We are kind of artists: there is always a bit of personal taste
(preferences)
in what we do; that justify our presence instead of an AI (for the time
being) :P
As I said in the other email, I tend to be verbose when something does
not have
an unique interpretation at first sight.
If this is your preference, I will not say anything more and I will not
send a V2
for this patch.
Federico Vaga