[2/2] eal: add eal_parse_optionlist to parse user input

  In current version, there is a function which parses
the corelist based on user value. A new generic
function eal_parse_optionlist is added which will
parse corelist as well as similar user input so
that we can use it as a public API too.

Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 45 ++++++++++++++++++------------
 lib/librte_eal/common/include/rte_eal.h    | 34 ++++++++++++++++++++++
 lib/librte_eal/rte_eal_version.map         |  1 +
 3 files changed, 62 insertions(+), 18 deletions(-)
  

On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:
> In current version, there is a function which parses
> the corelist based on user value. A new generic
> function eal_parse_optionlist is added which will
> parse corelist as well as similar user input so
> that we can use it as a public API too.
> 
> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>

Hi David,

Overall this patchset is to add '--portlist' command to testpmd and remove
existing 64 port limitation.

And in this patch re-uses the exiting parser function in eal and converts it to
API, question is if eal is good place to have this API, what do you think about it?

> ---
>  lib/librte_eal/common/eal_common_options.c | 45 ++++++++++++++++++------------
>  lib/librte_eal/common/include/rte_eal.h    | 34 ++++++++++++++++++++++
>  lib/librte_eal/rte_eal_version.map         |  1 +
>  3 files changed, 62 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 5920233..aa59f8b 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -571,33 +571,36 @@ eal_parse_service_corelist(const char *corelist)
>  	return 0;
>  }
>  
> -static int
> -eal_parse_corelist(const char *corelist, int *cores)
> +int
> +eal_parse_optionlist(const char *list, int *values, int maxsize)
>  {
>  	unsigned count = 0;
>  	char *end = NULL;
>  	int min, max;
>  	int idx;
>  
> -	for (idx = 0; idx < RTE_MAX_LCORE; idx++)
> -		cores[idx] = -1;
> +	if (list == NULL || values == NULL || maxsize < 0)
> +		return -1;
> +
> +	for (idx = 0; idx < maxsize; idx++)
> +		values[idx] = -1;
>  
>  	/* Remove all blank characters ahead */
> -	while (isblank(*corelist))
> -		corelist++;
> +	while (isblank(*list))
> +		list++;
> +
> +	min = maxsize;
>  
> -	/* Get list of cores */
> -	min = RTE_MAX_LCORE;
>  	do {
> -		while (isblank(*corelist))
> -			corelist++;
> -		if (*corelist == '\0')
> +		while (isblank(*list))
> +			list++;
> +		if (*list == '\0')
>  			return -1;
>  		errno = 0;
> -		idx = strtol(corelist, &end, 10);
> +		idx = strtol(list, &end, 10);
>  		if (errno || end == NULL)
>  			return -1;
> -		if (idx < 0 || idx >= RTE_MAX_LCORE)
> +		if (idx < 0 || idx >= maxsize)
>  			return -1;
>  		while (isblank(*end))
>  			end++;
> @@ -605,18 +608,18 @@ eal_parse_corelist(const char *corelist, int *cores)
>  			min = idx;
>  		} else if ((*end == ',') || (*end == '\0')) {
>  			max = idx;
> -			if (min == RTE_MAX_LCORE)
> +			if (min == maxsize)
>  				min = idx;
>  			for (idx = min; idx <= max; idx++) {
> -				if (cores[idx] == -1) {
> -					cores[idx] = count;
> +				if (values[idx] == -1) {
> +					values[idx] = count;
>  					count++;
>  				}
>  			}
> -			min = RTE_MAX_LCORE;
> +			min = maxsize;
>  		} else
>  			return -1;
> -		corelist = end + 1;
> +		list = end + 1;
>  	} while (*end != '\0');
>  
>  	if (count == 0)
> @@ -624,6 +627,12 @@ eal_parse_corelist(const char *corelist, int *cores)
>  	return 0;
>  }
>  
> +static int
> +eal_parse_corelist(const char *corelist, int *cores)
> +{
> +	return eal_parse_optionlist(corelist, cores, RTE_MAX_LCORE);
> +}
> +
>  /* Changes the lcore id of the master thread */
>  static int
>  eal_parse_master_lcore(const char *arg)
> diff --git a/lib/librte_eal/common/include/rte_eal.h b/lib/librte_eal/common/include/rte_eal.h
> index 2f9ed29..567b754 100644
> --- a/lib/librte_eal/common/include/rte_eal.h
> +++ b/lib/librte_eal/common/include/rte_eal.h
> @@ -71,6 +71,40 @@ enum rte_proc_type_t rte_eal_process_type(void);
>  int rte_eal_iopl_init(void);
>  
>  /**
> + * Parse the user input
> + *
> + * This function can be used to read and parse the user input
> + * from the command line. For example, when the user specifies
> + * corelist or port list this function will read the input
> + * and set the forwarding cores or ports
> + *
> + * @param[in] list
> + *   String containing the user input. User can specify
> + *   in these formats 1,3,5 or 1-3 or 1-2,5 or 3,5-6.
> + *   For example, if the user wants to use all the available
> + *   4 ports in his system, then the input can be 0-3 or 0,1,2,3.
> + *   If the user wants to use only the ports 1,2 then the input
> + *   is 1,2.
> + *   valid characters are '-' and ','
> + *   invalid chars like '.' or '#' will result in
> + *   EAL: Error - exiting with code: 1
> + *     Cause: Invalid fwd port list
> + * @param[in] values
> + *   An array pointer, used by this function to set the
> + *   array contents to a positive value if they are listed
> + *   in the input
> + *   else sets it to -1
> + * @param[in] maxsize
> + *   This is the maximum value the list string can contain
> + * @return
> + *   -On success, returns 0.
> + *   -On failure, returns -1.
> + */
> +__rte_experimental
> +int
> +eal_parse_optionlist(const char *list, int *values, int maxsize);
> +
> +/**
>   * Initialize the Environment Abstraction Layer (EAL).
>   *
>   * This function is to be executed on the MASTER lcore only, as soon
> diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
> index e38d025..3d72df8 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -332,4 +332,5 @@ EXPERIMENTAL {
>  	# added in 19.11
>  	rte_log_get_stream;
>  	rte_mcfg_get_single_file_segments;
> +	eal_parse_optionlist;
>  };
>
  

On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:
> > In current version, there is a function which parses
> > the corelist based on user value. A new generic
> > function eal_parse_optionlist is added which will
> > parse corelist as well as similar user input so
> > that we can use it as a public API too.
> >
> > Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
>
> Hi David,
>
> Overall this patchset is to add '--portlist' command to testpmd and remove
> existing 64 port limitation.
>
> And in this patch re-uses the exiting parser function in eal and converts it to
> API, question is if eal is good place to have this API, what do you think about it?

Exporting string parsers from the EAL has little value.
Ok we avoid code duplication (and I can see other places in the tree
where it might be used), but in the end we will have to maintain this
API in the ABI when it enters the stable ABI.

I am for avoiding this.
  

On 1/29/2020 5:44 PM, David Marchand wrote:
> On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:
>>> In current version, there is a function which parses
>>> the corelist based on user value. A new generic
>>> function eal_parse_optionlist is added which will
>>> parse corelist as well as similar user input so
>>> that we can use it as a public API too.
>>>
>>> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>
>>
>> Hi David,
>>
>> Overall this patchset is to add '--portlist' command to testpmd and remove
>> existing 64 port limitation.
>>
>> And in this patch re-uses the exiting parser function in eal and converts it to
>> API, question is if eal is good place to have this API, what do you think about it?
> 
> Exporting string parsers from the EAL has little value.
> Ok we avoid code duplication (and I can see other places in the tree
> where it might be used), but in the end we will have to maintain this
> API in the ABI when it enters the stable ABI.
> 
> I am for avoiding this.
> 
>

The same function can be used in some sample applications too (which are using
port mask), so instead of duplicating it multiple times, it would be nice to
have this function somewhere that applications can use.

Does it makes sense to have a rte_util.c (under in eal or as a separate library)
to have this kind of application helper functions?
  

On Wed, 29 Jan 2020 18:07:05 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/29/2020 5:44 PM, David Marchand wrote:
> > On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:  
> >> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:  
> >>> In current version, there is a function which parses
> >>> the corelist based on user value. A new generic
> >>> function eal_parse_optionlist is added which will
> >>> parse corelist as well as similar user input so
> >>> that we can use it as a public API too.
> >>>
> >>> Signed-off-by: Hariprasad Govindharajan <hariprasad.govindharajan@intel.com>  
> >>
> >> Hi David,
> >>
> >> Overall this patchset is to add '--portlist' command to testpmd and remove
> >> existing 64 port limitation.
> >>
> >> And in this patch re-uses the exiting parser function in eal and converts it to
> >> API, question is if eal is good place to have this API, what do you think about it?  
> > 
> > Exporting string parsers from the EAL has little value.
> > Ok we avoid code duplication (and I can see other places in the tree
> > where it might be used), but in the end we will have to maintain this
> > API in the ABI when it enters the stable ABI.
> > 
> > I am for avoiding this.
> > 
> >  
> 
> The same function can be used in some sample applications too (which are using
> port mask), so instead of duplicating it multiple times, it would be nice to
> have this function somewhere that applications can use.
> 
> Does it makes sense to have a rte_util.c (under in eal or as a separate library)
> to have this kind of application helper functions?

It makes sense to have a rte library that handles arbitrary size bitvector
and has string handling functions. Kind of like what kernel has for
the cpuset parsing.  This could be used for cpus in EAL and port-masks
or other arrays in applications.

But just doing copy/paste of existing code without thinking about how
API should work is a bad idea.
  

Hi,
I am planning to move the existing parser function to the testpmd file instead of keeping it in the eal and will revert the eal back.

Also, I am planning to create an util file in eal with this parser and do a RFC.

@Stephen Hemminger, We already investigated the existing function and then decided to re-use it as seen in the patch. For creating an API, is there any other specific requirements 
that should be addressed? Please clarify us.

Thanks
G Hariprasad

-----Original Message-----
From: Stephen Hemminger <stephen@networkplumber.org> 
Sent: Wednesday, January 29, 2020 7:20 PM
To: Yigit, Ferruh <ferruh.yigit@intel.com>
Cc: David Marchand <david.marchand@redhat.com>; Thomas Monjalon <thomas@monjalon.net>; Govindharajan, Hariprasad <hariprasad.govindharajan@intel.com>; dev <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse user input

On Wed, 29 Jan 2020 18:07:05 +0000
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/29/2020 5:44 PM, David Marchand wrote:
> > On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:  
> >> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:  
> >>> In current version, there is a function which parses the corelist 
> >>> based on user value. A new generic function eal_parse_optionlist 
> >>> is added which will parse corelist as well as similar user input 
> >>> so that we can use it as a public API too.
> >>>
> >>> Signed-off-by: Hariprasad Govindharajan 
> >>> <hariprasad.govindharajan@intel.com>
> >>
> >> Hi David,
> >>
> >> Overall this patchset is to add '--portlist' command to testpmd and 
> >> remove existing 64 port limitation.
> >>
> >> And in this patch re-uses the exiting parser function in eal and 
> >> converts it to API, question is if eal is good place to have this API, what do you think about it?
> > 
> > Exporting string parsers from the EAL has little value.
> > Ok we avoid code duplication (and I can see other places in the tree 
> > where it might be used), but in the end we will have to maintain 
> > this API in the ABI when it enters the stable ABI.
> > 
> > I am for avoiding this.
> > 
> >  
> 
> The same function can be used in some sample applications too (which 
> are using port mask), so instead of duplicating it multiple times, it 
> would be nice to have this function somewhere that applications can use.
> 
> Does it makes sense to have a rte_util.c (under in eal or as a 
> separate library) to have this kind of application helper functions?

It makes sense to have a rte library that handles arbitrary size bitvector and has string handling functions. Kind of like what kernel has for the cpuset parsing.  This could be used for cpus in EAL and port-masks or other arrays in applications.

But just doing copy/paste of existing code without thinking about how API should work is a bad idea.
  

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, January 29, 2020 7:20 PM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: David Marchand <david.marchand@redhat.com>; Thomas Monjalon
> <thomas@monjalon.net>; Govindharajan, Hariprasad
> <hariprasad.govindharajan@intel.com>; dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [PATCH 2/2] eal: add eal_parse_optionlist to parse
> user input
> 
> On Wed, 29 Jan 2020 18:07:05 +0000
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 1/29/2020 5:44 PM, David Marchand wrote:
> > > On Tue, Jan 28, 2020 at 6:35 PM Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
> > >> On 1/27/2020 10:30 AM, Hariprasad Govindharajan wrote:
> > >>> In current version, there is a function which parses the corelist
> > >>> based on user value. A new generic function eal_parse_optionlist
> > >>> is added which will parse corelist as well as similar user input
> > >>> so that we can use it as a public API too.
> > >>>
> > >>> Signed-off-by: Hariprasad Govindharajan
> > >>> <hariprasad.govindharajan@intel.com>
> > >>
> > >> Hi David,
> > >>
> > >> Overall this patchset is to add '--portlist' command to testpmd and
> > >> remove existing 64 port limitation.
> > >>
> > >> And in this patch re-uses the exiting parser function in eal and
> > >> converts it to API, question is if eal is good place to have this API, what
> do you think about it?
> > >
> > > Exporting string parsers from the EAL has little value.
> > > Ok we avoid code duplication (and I can see other places in the tree
> > > where it might be used), but in the end we will have to maintain
> > > this API in the ABI when it enters the stable ABI.
> > >
> > > I am for avoiding this.
> > >
> > >
> >
> > The same function can be used in some sample applications too (which
> > are using port mask), so instead of duplicating it multiple times, it
> > would be nice to have this function somewhere that applications can use.
> >
> > Does it makes sense to have a rte_util.c (under in eal or as a
> > separate library) to have this kind of application helper functions?
> 
> It makes sense to have a rte library that handles arbitrary size bitvector and
> has string handling functions. Kind of like what kernel has for the cpuset
> parsing.  This could be used for cpus in EAL and port-masks or other arrays in
> applications.
> 
> But just doing copy/paste of existing code without thinking about how API
> should work is a bad idea.

[Govindharajan, Hariprasad] 
Hi,

PLEASE IGNORE MY PREVIOUS EMAIL.

I am planning to move the existing parser function to the testpmd file instead of keeping it in the eal and will revert the eal back.

Also, I am planning to create an util file in eal with this parser and do a RFC.

@Stephen Hemminger, We already investigated the existing function and then decided to re-use it as seen in the patch. For creating an API, is there any other specific requirements that should be addressed? Please clarify us.

Thanks
G Hariprasad