Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Federico Vaga]

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

Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Federico Vaga]

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

Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Steven Rostedt]

On Tue, 25 Jul 2017 16:21:11 +0200
Federico Vaga  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(, "%s/%s", dir, name);
> > > > +   if (ret < 0)
> > > > 
> > > > die("Failed to allocate %s/%s", dir, name);
> > > > 
> > > > -   sprintf(file, "%s/%s", dir, name);
> > > > 
> > > > return file;
> > > >  
> > > >  }
> > > > 

Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Steven Rostedt]

On Tue, 25 Jul 2017 16:21:11 +0200
Federico Vaga  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(, "%s/%s", dir, name);
> > > > +   if (ret < 0)
> > > > 
> > > > die("Failed to allocate %s/%s", dir, name);
> > > > 
> > > > -   sprintf(file, "%s/%s", dir, name);
> > > > 
> > > > return file;
> > > >  
> > > >  }
> > > > 

Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Federico Vaga]

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  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 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 
> > > ---
> > > 
> > >  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(, "%s/enable", path);
> > > + if (ret < 0)
> > > 
> > >   die("Failed to allocate enable path for %s", path);
> > > 
> > > - sprintf(p, "%s/enable", path);
> > > 
> > >   ret = stat(p, );
> > >   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(, "%s/trigger", path);
> > > + if (ret < 0)
> > > 
> > >   die("Failed to allocate trigger path for %s", path);
> > > 
> > > - sprintf(p, 

Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Federico Vaga]

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  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 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 
> > > ---
> > > 
> > >  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(, "%s/enable", path);
> > > + if (ret < 0)
> > > 
> > >   die("Failed to allocate enable path for %s", path);
> > > 
> > > - sprintf(p, "%s/enable", path);
> > > 
> > >   ret = stat(p, );
> > >   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(, "%s/trigger", path);
> > > + if (ret < 0)
> > > 
> > >   die("Failed to allocate trigger path for %s", path);
> > > 
> > > - sprintf(p, "%s/trigger", path);
> > > 
> > >   ret = stat(p, 

Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Federico Vaga]

On Friday, July 7, 2017 12:25:32 AM CEST Steven Rostedt wrote:
> On Mon,  5 Jun 2017 11:31:17 +0200
> Federico Vaga  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 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 
> > ---
> > 
> >  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(, "%s/enable", path);
> > +   if (ret < 0)
> > 
> > die("Failed to allocate enable path for %s", path);
> > 
> > -   sprintf(p, "%s/enable", path);
> > 
> > ret = stat(p, );
> > 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(, "%s/trigger", path);
> > +   if (ret < 0)
> > 
> > die("Failed to allocate trigger path for %s", path);
> > 
> > -   sprintf(p, "%s/trigger", path);
> > 
> > ret = stat(p, );
> > 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(, "%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, 

Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Federico Vaga]

On Friday, July 7, 2017 12:25:32 AM CEST Steven Rostedt wrote:
> On Mon,  5 Jun 2017 11:31:17 +0200
> Federico Vaga  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 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 
> > ---
> > 
> >  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(, "%s/enable", path);
> > +   if (ret < 0)
> > 
> > die("Failed to allocate enable path for %s", path);
> > 
> > -   sprintf(p, "%s/enable", path);
> > 
> > ret = stat(p, );
> > 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(, "%s/trigger", path);
> > +   if (ret < 0)
> > 
> > die("Failed to allocate trigger path for %s", path);
> > 
> > -   sprintf(p, "%s/trigger", path);
> > 
> > ret = stat(p, );
> > 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(, "%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);
> 
> 

Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Steven Rostedt]

On Mon,  5 Jun 2017 11:31:17 +0200
Federico Vaga  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 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 
> ---
>  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(, "%s/enable", path);
> + if (ret < 0)
>   die("Failed to allocate enable path for %s", path);
> - sprintf(p, "%s/enable", path);
>   ret = stat(p, );
>   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(, "%s/trigger", path);
> + if (ret < 0)
>   die("Failed to allocate trigger path for %s", path);
> - sprintf(p, "%s/trigger", path);
>   ret = stat(p, );
>   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(, "%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(, "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 

Re: [PATCH 1/2] trace-cmd: use asprintf when possible

[Steven Rostedt]

On Mon,  5 Jun 2017 11:31:17 +0200
Federico Vaga  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 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 
> ---
>  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(, "%s/enable", path);
> + if (ret < 0)
>   die("Failed to allocate enable path for %s", path);
> - sprintf(p, "%s/enable", path);
>   ret = stat(p, );
>   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(, "%s/trigger", path);
> + if (ret < 0)
>   die("Failed to allocate trigger path for %s", path);
> - sprintf(p, "%s/trigger", path);
>   ret = stat(p, );
>   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(, "%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(, "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)
>