* Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef
@ 2016-06-13 14:18 Shreyansh Jain
2016-06-13 14:24 ` Jan Viktorin
0 siblings, 1 reply; 7+ messages in thread
From: Shreyansh Jain @ 2016-06-13 14:18 UTC (permalink / raw)
To: Jan Viktorin
Cc: dev, David Marchand, Thomas Monjalon, Bruce Richardson,
Declan Doherty, jianbo.liu, jerin.jacob, Keith Wiles,
Stephen Hemminger
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef
2016-06-13 14:18 [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef Shreyansh Jain
@ 2016-06-13 14:24 ` Jan Viktorin
2016-06-14 4:30 ` Shreyansh Jain
0 siblings, 1 reply; 7+ messages in thread
From: Jan Viktorin @ 2016-06-13 14:24 UTC (permalink / raw)
To: Shreyansh Jain
Cc: dev, David Marchand, Thomas Monjalon, Bruce Richardson,
Declan Doherty, jianbo.liu, jerin.jacob, Keith Wiles,
Stephen Hemminger
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
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef
2016-06-13 14:24 ` Jan Viktorin
@ 2016-06-14 4:30 ` Shreyansh Jain
2016-06-15 9:56 ` Jan Viktorin
0 siblings, 1 reply; 7+ messages in thread
From: Shreyansh Jain @ 2016-06-14 4:30 UTC (permalink / raw)
To: Jan Viktorin
Cc: dev, David Marchand, Thomas Monjalon, Bruce Richardson,
Declan Doherty, jianbo.liu, jerin.jacob, Keith Wiles,
Stephen Hemminger
Hi Jan,
> -----Original Message-----
> From: Jan Viktorin [mailto:viktorin@rehivetech.com]
> Sent: Monday, June 13, 2016 7:55 PM
> To: Shreyansh Jain <shreyansh.jain@nxp.com>
> Cc: dev@dpdk.org; 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: 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@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...
>
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef
2016-06-14 4:30 ` Shreyansh Jain
@ 2016-06-15 9:56 ` Jan Viktorin
2016-06-16 11:47 ` Shreyansh Jain
0 siblings, 1 reply; 7+ messages in thread
From: Jan Viktorin @ 2016-06-15 9:56 UTC (permalink / raw)
To: Shreyansh Jain
Cc: dev, David Marchand, Thomas Monjalon, Bruce Richardson,
Declan Doherty, jianbo.liu, jerin.jacob, Keith Wiles,
Stephen Hemminger
On Tue, 14 Jun 2016 04:30:57 +0000
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> Hi Jan,
>
> > -----Original Message-----
> > From: Jan Viktorin [mailto:viktorin@rehivetech.com]
> > Sent: Monday, June 13, 2016 7:55 PM
> > To: Shreyansh Jain <shreyansh.jain@nxp.com>
> > Cc: dev@dpdk.org; 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: 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@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...
> >
>
> 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.
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.
>
> 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.
>
> >
> > Regards
> > Jan
> >
> > [...]
>
> -
> Shreyansh
Thanks for comments
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef
2016-06-15 9:56 ` Jan Viktorin
@ 2016-06-16 11:47 ` Shreyansh Jain
2016-07-04 13:24 ` Jan Viktorin
0 siblings, 1 reply; 7+ messages in thread
From: Shreyansh Jain @ 2016-06-16 11:47 UTC (permalink / raw)
To: Jan Viktorin
Cc: dev, David Marchand, Thomas Monjalon, Bruce Richardson,
Declan Doherty, jianbo.liu, jerin.jacob, Keith Wiles,
Stephen Hemminger
Sorry, didn't notice this email earlier...
Comments inline
> -----Original Message-----
> From: Jan Viktorin [mailto:viktorin@rehivetech.com]
> Sent: Wednesday, June 15, 2016 3:26 PM
> To: Shreyansh Jain <shreyansh.jain@nxp.com>
> Cc: dev@dpdk.org; 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: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> eal_parse_sysfs_valuef
>
> On Tue, 14 Jun 2016 04:30:57 +0000
> Shreyansh Jain <shreyansh.jain@nxp.com> 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_<XX> ==> eal_parse_sysfs_value_<XX>_<YY>'
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef
2016-06-16 11:47 ` Shreyansh Jain
@ 2016-07-04 13:24 ` Jan Viktorin
0 siblings, 0 replies; 7+ messages in thread
From: Jan Viktorin @ 2016-07-04 13:24 UTC (permalink / raw)
To: Shreyansh Jain
Cc: dev, David Marchand, Thomas Monjalon, Bruce Richardson,
Declan Doherty, jianbo.liu, jerin.jacob, Keith Wiles,
Stephen Hemminger
Hello Shreyansh,
On Thu, 16 Jun 2016 11:47:29 +0000
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> Sorry, didn't notice this email earlier...
> Comments inline
>
> > -----Original Message-----
> > From: Jan Viktorin [mailto:viktorin@rehivetech.com]
> > Sent: Wednesday, June 15, 2016 3:26 PM
> > To: Shreyansh Jain <shreyansh.jain@nxp.com>
> > Cc: dev@dpdk.org; 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: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> > eal_parse_sysfs_valuef
> >
> > On Tue, 14 Jun 2016 04:30:57 +0000
> > Shreyansh Jain <shreyansh.jain@nxp.com> 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_<XX> ==> eal_parse_sysfs_value_<XX>_<YY>'
>
> 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@RehiveTech.com
System Architect Web: www.RehiveTech.com
RehiveTech
Brno, Czech Republic
^ permalink raw reply [flat|nested] 7+ messages in thread
* [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef
2016-01-01 21:05 ` [dpdk-dev] [RFC 0/7] " Jan Viktorin
@ 2016-05-06 13:47 ` Jan Viktorin
0 siblings, 0 replies; 7+ messages in thread
From: Jan Viktorin @ 2016-05-06 13:47 UTC (permalink / raw)
To: dev
Cc: Jan Viktorin, David Marchand, Thomas Monjalon, Bruce Richardson,
Declan Doherty, jianbo.liu, jerin.jacob, Keith Wiles,
Stephen Hemminger
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)
+{
+ 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-07-04 13:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-13 14:18 [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef Shreyansh Jain
2016-06-13 14:24 ` Jan Viktorin
2016-06-14 4:30 ` Shreyansh Jain
2016-06-15 9:56 ` Jan Viktorin
2016-06-16 11:47 ` Shreyansh Jain
2016-07-04 13:24 ` Jan Viktorin
-- strict thread matches above, loose matches on Subject: below --
2016-05-06 13:47 [dpdk-dev] [PATCH v1 00/28] Support non-PCI devices Jan Viktorin
2016-01-01 21:05 ` [dpdk-dev] [RFC 0/7] " Jan Viktorin
2016-05-06 13:47 ` [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef Jan Viktorin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).