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

2016-07-04 Thread Jan Viktorin
Hello Shreyansh,

On Thu, 16 Jun 2016 11:47:29 +
Shreyansh Jain  wrote:

> Sorry, didn't notice this email earlier...
> Comments inline
> 
> > -Original Message-
> > From: Jan Viktorin [mailto:viktorin at rehivetech.com]
> > Sent: Wednesday, June 15, 2016 3:26 PM
> > To: Shreyansh Jain 
> > Cc: dev at dpdk.org; David Marchand ; Thomas 
> > Monjalon
> > ; Bruce Richardson  > intel.com>;
> > Declan Doherty ; jianbo.liu at linaro.org;
> > jerin.jacob at caviumnetworks.com; Keith Wiles ; 
> > Stephen
> > Hemminger 
> > Subject: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> > eal_parse_sysfs_valuef
> > 
> > On Tue, 14 Jun 2016 04:30:57 +
> > Shreyansh Jain  wrote:
> >   
> > > Hi Jan,
> > >  
> 
> [...]
> 
> 
> > > > >
> > > > > 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.  
> 
> Asymmetrical because cascading function names maybe additive for easy 
> reading/recall.
> 
> 'eal_parse_sysfs_value ==> eal_parse_sysfs_value_ ==> 
> eal_parse_sysfs_value__'
> 
> Obviously, this is not a rule - it just makes reading and recalling of 
> cascade easier.
> As for:
> 
> eal_parse_sysfs_value => eal_parse_sysfs_file_value
> 
> inserts an identifier between a name, making it (slightly) difficult to 
> correlate.
> 
> Again, as I mentioned earlier, this is subjective argument and matter of 
> (personal!) choice.
> 
> >   
> > >
> > > 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.
> >   
> [...]

I've finally returned to your idea to name it eal_parse_sysfs_value_read.

Thanks.
Jan

> 
> -
> Shreyansh



-- 
   Jan Viktorin  E-mail: Viktorin at RehiveTech.com
   System Architect  Web:www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic


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

2016-06-16 Thread Shreyansh Jain
Sorry, didn't notice this email earlier...
Comments inline

> -Original Message-
> From: Jan Viktorin [mailto:viktorin at rehivetech.com]
> Sent: Wednesday, June 15, 2016 3:26 PM
> To: Shreyansh Jain 
> Cc: dev at dpdk.org; David Marchand ; Thomas 
> Monjalon
> ; Bruce Richardson  intel.com>;
> Declan Doherty ; jianbo.liu at linaro.org;
> jerin.jacob at caviumnetworks.com; Keith Wiles ; 
> Stephen
> Hemminger 
> Subject: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> eal_parse_sysfs_valuef
> 
> On Tue, 14 Jun 2016 04:30:57 +
> Shreyansh Jain  wrote:
> 
> > Hi Jan,
> >

[...]


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

Asymmetrical because cascading function names maybe additive for easy 
reading/recall.

'eal_parse_sysfs_value ==> eal_parse_sysfs_value_ ==> 
eal_parse_sysfs_value__'

Obviously, this is not a rule - it just makes reading and recalling of cascade 
easier.
As for:

eal_parse_sysfs_value => eal_parse_sysfs_file_value

inserts an identifier between a name, making it (slightly) difficult to 
correlate.

Again, as I mentioned earlier, this is subjective argument and matter of 
(personal!) choice.

> 
> >
> > 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.
> 
[...]

-
Shreyansh


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

2016-06-15 Thread Jan Viktorin
On Tue, 14 Jun 2016 04:30:57 +
Shreyansh Jain  wrote:

> Hi Jan,
> 
> > -Original Message-
> > From: Jan Viktorin [mailto:viktorin at rehivetech.com]
> > Sent: Monday, June 13, 2016 7:55 PM
> > To: Shreyansh Jain 
> > Cc: dev at dpdk.org; David Marchand ; Thomas 
> > Monjalon
> > ; Bruce Richardson  > intel.com>;
> > Declan Doherty ; jianbo.liu at linaro.org;
> > jerin.jacob at 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 +
> > Shreyansh Jain  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 ; David Marchand
> > > > ; Thomas Monjalon  > > > 6wind.com>;
> > > > Bruce Richardson ; Declan Doherty
> > > > ; jianbo.liu at linaro.org;
> > > > jerin.jacob at caviumnetworks.com; Keith Wiles  > > > intel.com>;  
> > 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 _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

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

2016-06-14 Thread Shreyansh Jain
Hi Jan,

> -Original Message-
> From: Jan Viktorin [mailto:viktorin at rehivetech.com]
> Sent: Monday, June 13, 2016 7:55 PM
> To: Shreyansh Jain 
> Cc: dev at dpdk.org; David Marchand ; Thomas 
> Monjalon
> ; Bruce Richardson  intel.com>;
> Declan Doherty ; jianbo.liu at linaro.org;
> jerin.jacob at 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 +
> Shreyansh Jain  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 ; David Marchand
> > > ; Thomas Monjalon  > > 6wind.com>;
> > > Bruce Richardson ; Declan Doherty
> > > ; jianbo.liu at linaro.org;
> > > jerin.jacob at 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 _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


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

2016-06-13 Thread Jan Viktorin
On Mon, 13 Jun 2016 14:18:40 +
Shreyansh Jain  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 ; David Marchand
> > ; Thomas Monjalon  > 6wind.com>;
> > Bruce Richardson ; Declan Doherty
> > ; jianbo.liu at linaro.org;
> > jerin.jacob at 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 _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

[...]


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

2016-06-13 Thread Shreyansh Jain
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 ; David Marchand
> ; Thomas Monjalon ;
> Bruce Richardson ; Declan Doherty
> ; jianbo.liu at linaro.org;
> jerin.jacob at 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 _config;
>  }
> 
> +int
> +eal_parse_sysfs_valuef(FILE *f, unsigned long *val)

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 almost skipped the '..f' in the name and wondered how two functions having 
same name exist :D

> +{
> + char buf[BUFSIZ];
> + char *end = NULL;
> +
> + RTE_VERIFY(f != NULL);
> +
> + if (fgets(buf, sizeof(buf), f) == NULL)
> + return -1;
> +
> + *val = strtoul(buf, , 0);
> + if ((buf[0] == '\0') || (end == NULL) || (*end != '\n'))
> + return -2;
> +
> + return 0;
> +}
> +
>  /* parse a sysfs (or other) file containing one integer value */
>  int
>  eal_parse_sysfs_value(const char *filename, unsigned long *val)
>  {
> + int ret;
>   FILE *f;
> - char buf[BUFSIZ];
> - char *end = NULL;
> 
>   if ((f = fopen(filename, "r")) == NULL) {
>   RTE_LOG(ERR, EAL, "%s(): cannot open sysfs value %s\n",
> @@ -140,21 +157,18 @@ eal_parse_sysfs_value(const char *filename, unsigned
> long *val)
>   return -1;
>   }
> 
> - if (fgets(buf, sizeof(buf), f) == NULL) {
> + ret = eal_parse_sysfs_valuef(f, val);
> + if (ret == -1) {
>   RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
> - __func__, filename);
> - fclose(f);
> - return -1;
> + __func__, filename);
>   }
> - *val = strtoul(buf, , 0);
> - if ((buf[0] == '\0') || (end == NULL) || (*end != '\n')) {
> + else if (ret < 0) {
>   RTE_LOG(ERR, EAL, "%s(): cannot parse sysfs value %s\n",
>   __func__, filename);
> - fclose(f);
> - return -1;
>   }
> +
>   fclose(f);
> - return 0;
> + return ret;
>  }
> 
> 
> --
> 2.8.0

-
Shreyansh



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

2016-05-06 Thread Jan Viktorin
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 _config;
 }

+int
+eal_parse_sysfs_valuef(FILE *f, unsigned long *val)
+{
+   char buf[BUFSIZ];
+   char *end = NULL;
+
+   RTE_VERIFY(f != NULL);
+
+   if (fgets(buf, sizeof(buf), f) == NULL)
+   return -1;
+
+   *val = strtoul(buf, , 0);
+   if ((buf[0] == '\0') || (end == NULL) || (*end != '\n'))
+   return -2;
+
+   return 0;
+}
+
 /* parse a sysfs (or other) file containing one integer value */
 int
 eal_parse_sysfs_value(const char *filename, unsigned long *val)
 {
+   int ret;
FILE *f;
-   char buf[BUFSIZ];
-   char *end = NULL;

if ((f = fopen(filename, "r")) == NULL) {
RTE_LOG(ERR, EAL, "%s(): cannot open sysfs value %s\n",
@@ -140,21 +157,18 @@ eal_parse_sysfs_value(const char *filename, unsigned long 
*val)
return -1;
}

-   if (fgets(buf, sizeof(buf), f) == NULL) {
+   ret = eal_parse_sysfs_valuef(f, val);
+   if (ret == -1) {
RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
-   __func__, filename);
-   fclose(f);
-   return -1;
+   __func__, filename);
}
-   *val = strtoul(buf, , 0);
-   if ((buf[0] == '\0') || (end == NULL) || (*end != '\n')) {
+   else if (ret < 0) {
RTE_LOG(ERR, EAL, "%s(): cannot parse sysfs value %s\n",
__func__, filename);
-   fclose(f);
-   return -1;
}
+
fclose(f);
-   return 0;
+   return ret;
 }


-- 
2.8.0