From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751769AbdGaXNB (ORCPT ); Mon, 31 Jul 2017 19:13:01 -0400 Received: from mx.kolabnow.com ([95.128.36.41]:32238 "EHLO mx.kolabnow.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751296AbdGaXNA (ORCPT ); Mon, 31 Jul 2017 19:13:00 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Tue, 01 Aug 2017 01:12:57 +0200 From: Federico Vaga To: Steven Rostedt Cc: LKML Subject: Re: [PATCH 1/2] trace-cmd: use asprintf when possible In-Reply-To: <20170731153340.4999978a@gandalf.local.home> References: <20170605093118.29962-1-federico.vaga@vaga.pv.it> <20170706182532.212cba4f@gandalf.local.home> <2876336.80y3AYhZJ7@harkonnen> <1967443.iUBOHzf2Bp@harkonnen> <20170731153340.4999978a@gandalf.local.home> Message-ID: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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