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

From: Federico Vaga
Date: Mon Jul 31 2017 - 19:13:06 EST

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 <federico.vaga@xxxxxxxxxx> 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