Messages in this thread | | | Date | Mon, 31 Jul 2017 15:33:40 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH 1/2] trace-cmd: use asprintf when possible |
| |
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; > > > > > > > > } > > > >
| |