Message-ID: <cb1cf66ce3d74027e951010e93569f83@vaga.pv.it>
Date:   Tue, 01 Aug 2017 01:12:57 +0200
From:   Federico Vaga <federico.vaga@...a.pv.it>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] trace-cmd: use asprintf when possible

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@...a.pv.it> 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

Powered by blists - more mailing lists