Re: [PATCH V3 1/2] use direname instead of custom code

[Steven Rostedt]

I'm back from vacation!

On Thu,  3 Aug 2017 00:15:57 +0200
Federico Vaga  wrote:

> Prefer well known functions like `dirname(3)` instead of custom
> implementation for the same functionality
> 
> Signed-off-by: Federico Vaga 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #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, );
>   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, );
>   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);

Re: [PATCH V3 1/2] use direname instead of custom code

[Steven Rostedt]

I'm back from vacation!

On Thu,  3 Aug 2017 00:15:57 +0200
Federico Vaga  wrote:

> Prefer well known functions like `dirname(3)` instead of custom
> implementation for the same functionality
> 
> Signed-off-by: Federico Vaga 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  
>  #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, );
>   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, );
>   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);

[PATCH V3 1/2] use direname instead of custom code

[Federico Vaga]

Prefer well known functions like `dirname(3)` instead of custom
implementation for the same functionality

Signed-off-by: Federico Vaga 
---
 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 
 #include 
 #include 
+#include 
 
 #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, );
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, );
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

[PATCH V3 1/2] use direname instead of custom code

[Federico Vaga]

Prefer well known functions like `dirname(3)` instead of custom
implementation for the same functionality

Signed-off-by: Federico Vaga 
---
 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 
 #include 
 #include 
+#include 
 
 #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, );
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, );
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