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

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 25 Jul 2017 16:21:11 +0200
Federico Vaga <federico.vaga@vaga.pv.it> 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;
> > > >  
> > > >  }
> > > >