[dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef
Shreyansh Jain
shreyansh.jain at nxp.com
Tue Jun 14 06:30:57 CEST 2016
Hi Jan,
> -----Original Message-----
> From: Jan Viktorin [mailto:viktorin at rehivetech.com]
> Sent: Monday, June 13, 2016 7:55 PM
> To: Shreyansh Jain <shreyansh.jain at nxp.com>
> Cc: dev at dpdk.org; 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: 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 <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...
>
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.
Or, eal_parse_sysfs_value -> eal_parse_sysfs_value_read() may be...
But, eal_parse_sysfs_file_value is still preferred than eal_parse_sysfs_fd_value, for me.
>
> Regards
> Jan
>
> [...]
-
Shreyansh
More information about the dev
mailing list