From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so1.wedos.net (wes1-so1.wedos.net [46.28.106.15]) by dpdk.org (Postfix) with ESMTP id B98DFADB6 for ; Wed, 15 Jun 2016 12:01:32 +0200 (CEST) Received: from pcviktorin.fit.vutbr.cz (pcviktorin.fit.vutbr.cz [147.229.13.147]) by wes1-so1.wedos.net (Postfix) with ESMTPSA id 3rV2982tYKz9NH; Wed, 15 Jun 2016 12:01:32 +0200 (CEST) Date: Wed, 15 Jun 2016 11:56:26 +0200 From: Jan Viktorin To: Shreyansh Jain Cc: "dev@dpdk.org" , David Marchand , Thomas Monjalon , Bruce Richardson , Declan Doherty , "jianbo.liu@linaro.org" , "jerin.jacob@caviumnetworks.com" , "Keith Wiles" , Stephen Hemminger Message-ID: <20160615115626.31bdd784@pcviktorin.fit.vutbr.cz> In-Reply-To: References: <20160613162431.714e56ae@pcviktorin.fit.vutbr.cz> Organization: RehiveTech MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 15 Jun 2016 10:01:32 -0000 On Tue, 14 Jun 2016 04:30:57 +0000 Shreyansh Jain wrote: > Hi Jan, > > > -----Original Message----- > > From: Jan Viktorin [mailto:viktorin@rehivetech.com] > > Sent: Monday, June 13, 2016 7:55 PM > > To: Shreyansh Jain > > Cc: dev@dpdk.org; David Marchand ; Thomas Monjalon > > ; Bruce Richardson ; > > Declan Doherty ; jianbo.liu@linaro.org; > > jerin.jacob@caviumnetworks.com; Keith Wiles ; Stephen > > Hemminger > > Subject: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function > > eal_parse_sysfs_valuef > > > > On Mon, 13 Jun 2016 14:18:40 +0000 > > Shreyansh Jain wrote: > > > > > Hi Jan, > > > > > > > -----Original Message----- > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jan Viktorin > > > > Sent: Friday, May 06, 2016 7:18 PM > > > > To: dev@dpdk.org > > > > Cc: Jan Viktorin ; David Marchand > > > > ; Thomas Monjalon ; > > > > Bruce Richardson ; Declan Doherty > > > > ; jianbo.liu@linaro.org; > > > > jerin.jacob@caviumnetworks.com; Keith Wiles ; > > Stephen > > > > Hemminger > > > > Subject: [dpdk-dev] [PATCH v1 02/28] eal: extract function > > > > eal_parse_sysfs_valuef > > > > > > > > The eal_parse_sysfs_value function accepts a filename however, such > > interface > > > > introduces race-conditions to the code. Introduce the variant of this > > > > function > > > > that accepts an already opened file instead of a filename. > > > > > > > > Signed-off-by: Jan Viktorin > > > > --- > > > > lib/librte_eal/common/eal_filesystem.h | 5 +++++ > > > > lib/librte_eal/linuxapp/eal/eal.c | 36 +++++++++++++++++++++++----- > > ---- > > > > -- > > > > 2 files changed, 30 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/lib/librte_eal/common/eal_filesystem.h > > > > b/lib/librte_eal/common/eal_filesystem.h > > > > index fdb4a70..7875454 100644 > > > > --- a/lib/librte_eal/common/eal_filesystem.h > > > > +++ b/lib/librte_eal/common/eal_filesystem.h > > > > @@ -43,6 +43,7 @@ > > > > /** Path of rte config file. */ > > > > #define RUNTIME_CONFIG_FMT "%s/.%s_config" > > > > > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -115,4 +116,8 @@ eal_get_hugefile_temp_path(char *buffer, size_t > > buflen, > > > > const char *hugedir, int > > > > * Used to read information from files on /sys */ > > > > int eal_parse_sysfs_value(const char *filename, unsigned long *val); > > > > > > > > +/** Function to read a single numeric value from a file on the > > filesystem. > > > > + * Used to read information from files on /sys */ > > > > +int eal_parse_sysfs_valuef(FILE *f, unsigned long *val); > > > > + > > > > #endif /* EAL_FILESYSTEM_H */ > > > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c > > > > b/lib/librte_eal/linuxapp/eal/eal.c > > > > index 4b28197..e8fce6b 100644 > > > > --- a/lib/librte_eal/linuxapp/eal/eal.c > > > > +++ b/lib/librte_eal/linuxapp/eal/eal.c > > > > @@ -126,13 +126,30 @@ rte_eal_get_configuration(void) > > > > return &rte_config; > > > > } > > > > > > > > +int > > > > +eal_parse_sysfs_valuef(FILE *f, unsigned long *val) > > > > Hi Shreyansh, > > > > > > > > Trivial Comment: Maybe it is just me, but this function name is too close > > to its caller 'eal_parse_sysfs_value'. Probably, the name of the caller can > > be changed to 'eal_parse_sysfs' because anyways value parsing is being done > > in this ('eal_parse_sysfs_valuef()) function now. And, of course, dropping > > the '..f' in this name. > > > > I don't like the idea of renaming the orignal function eal_parse_sysfs_value. > > The function > > name is not related to its actual body but to its semantics. And the > > semantics are still > > the same. This would introduce many other unneeded changes... > > > > Agree. I overlooked that. > > > > > > > I almost skipped the '..f' in the name and wondered how two functions > > having same name exist :D > > > > I agree that a better name would be nice here. This convention was based on > > the libc naming > > (fopen, fclose) but the "f" letter could not be at the beginning. > > > > What about one of those? > > > > * eal_parse_sysfs_fd_value > > * eal_parse_sysfs_file_value > > I don't have any better idea than above. > > Though, I still feel that 'eal_parse_sysfs_value -> eal_parse_sysfs_file_value' would be slightly asymmetrical - but again, this is highly subjective argument. I don't see any asymmetry here. The functions equal, just the new one accepts a file pointer instead of a path and we don't have function name overloading in C. > > Or, eal_parse_sysfs_value -> eal_parse_sysfs_value_read() may be... I think, I'll go with eal_parse_sysfs_file_value for v2. Ideally, it should be eal_parse_sysfs_path_value and eal_parse_sysfs_file_value. Thus, this looks like a good way. > > But, eal_parse_sysfs_file_value is still preferred than eal_parse_sysfs_fd_value, for me. Agree. > > > > > Regards > > Jan > > > > [...] > > - > Shreyansh Thanks for comments Jan