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

[Shreyansh Jain <shreyansh.jain@nxp.com>]

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)

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, &end, 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, &end, 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