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

Jan Viktorin viktorin at rehivetech.com
Mon Jun 13 16:24:31 CEST 2016


On Mon, 13 Jun 2016 14:18:40 +0000
Shreyansh Jain <shreyansh.jain at nxp.com> wrote:

> Hi Jan,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Jan Viktorin
> > Sent: Friday, May 06, 2016 7:18 PM
> > To: dev at dpdk.org
> > Cc: Jan Viktorin <viktorin at rehivetech.com>; David Marchand
> > <david.marchand at 6wind.com>; Thomas Monjalon <thomas.monjalon at 6wind.com>;
> > Bruce Richardson <bruce.richardson at intel.com>; Declan Doherty
> > <declan.doherty at intel.com>; jianbo.liu at linaro.org;
> > jerin.jacob at caviumnetworks.com; Keith Wiles <keith.wiles at intel.com>; Stephen
> > Hemminger <stephen at 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 at 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

[...]


More information about the dev mailing list