DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jan Viktorin <viktorin@rehivetech.com>
To: Shreyansh Jain <shreyansh.jain@nxp.com>
Cc: "dev@dpdk.org" <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" <jianbo.liu@linaro.org>,
	"jerin.jacob@caviumnetworks.com" <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
Date: Wed, 15 Jun 2016 11:56:26 +0200	[thread overview]
Message-ID: <20160615115626.31bdd784@pcviktorin.fit.vutbr.cz> (raw)
In-Reply-To: <DB5PR0401MB205453ED883DD30F28E5E4F690540@DB5PR0401MB2054.eurprd04.prod.outlook.com>

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

  reply	other threads:[~2016-06-15 10:01 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-13 14:18 Shreyansh Jain
2016-06-13 14:24 ` Jan Viktorin
2016-06-14  4:30   ` Shreyansh Jain
2016-06-15  9:56     ` Jan Viktorin [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160615115626.31bdd784@pcviktorin.fit.vutbr.cz \
    --to=viktorin@rehivetech.com \
    --cc=bruce.richardson@intel.com \
    --cc=david.marchand@6wind.com \
    --cc=declan.doherty@intel.com \
    --cc=dev@dpdk.org \
    --cc=jerin.jacob@caviumnetworks.com \
    --cc=jianbo.liu@linaro.org \
    --cc=keith.wiles@intel.com \
    --cc=shreyansh.jain@nxp.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas.monjalon@6wind.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).