Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef

[Jan Viktorin <viktorin@rehivetech.com>]

On Mon, 13 Jun 2016 14:18:40 +0000
Shreyansh Jain <shreyansh.jain@nxp.com> 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 <viktorin@rehivetech.com>; David Marchand
> > <david.marchand@6wind.com>; Thomas Monjalon <thomas.monjalon@6wind.com>;
> > Bruce Richardson <bruce.richardson@intel.com>; Declan Doherty
> > <declan.doherty@intel.com>; jianbo.liu@linaro.org;
> > jerin.jacob@caviumnetworks.com; Keith Wiles <keith.wiles@intel.com>; Stephen
> > Hemminger <stephen@networkplumber.org>
> > 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 <viktorin@rehivetech.com>
> > ---
> >  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 <stdio.h>
> >  #include <stdint.h>
> >  #include <limits.h>
> >  #include <unistd.h>
> > @@ -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...

> 
> 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

Regards
Jan

[...]