Re: [dpdk-dev] [PATCH v4 2/9] ethdev: support representor port list

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 1/18/21 2:16 PM, Xueming Li wrote:
> To support extended representor syntax, this patch extends the
> representor list parsing to support for representor port range in
> devargs, examples:
>    representor=[1,2,3]         - single list
>    representor=[1,3-5,7,9-11]  - list with singles and ranges
> 
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>

See below

> ---
>  lib/librte_ethdev/ethdev_private.c | 105 ++++++++++++++---------------
>  lib/librte_ethdev/ethdev_private.h |   3 -
>  lib/librte_ethdev/rte_class_eth.c  |   4 +-
>  lib/librte_ethdev/rte_ethdev.c     |   5 +-
>  4 files changed, 54 insertions(+), 63 deletions(-)
> 
> diff --git a/lib/librte_ethdev/ethdev_private.c b/lib/librte_ethdev/ethdev_private.c
> index c1a411dba4..12bcc7e98d 100644
> --- a/lib/librte_ethdev/ethdev_private.c
> +++ b/lib/librte_ethdev/ethdev_private.c
> @@ -38,77 +38,71 @@ eth_find_device(const struct rte_eth_dev *start, rte_eth_cmp_t cmp,
>  	return NULL;
>  }
>  
> -int
> -rte_eth_devargs_parse_list(char *str, rte_eth_devargs_callback_t callback,
> -	void *data)
> +static int
> +rte_eth_devargs_enlist(uint16_t *list, uint16_t *len_list,
> +		       const uint16_t max_list, uint16_t val)
>  {
> -	char *str_start;
> -	int state;
> -	int result;
> -
> -	if (*str != '[')
> -		/* Single element, not a list */
> -		return callback(str, data);
> -
> -	/* Sanity check, then strip the brackets */
> -	str_start = &str[strlen(str) - 1];
> -	if (*str_start != ']') {
> -		RTE_LOG(ERR, EAL, "(%s): List does not end with ']'\n", str);
> -		return -EINVAL;
> -	}
> -	str++;
> -	*str_start = '\0';
> +	uint16_t i;
>  
> -	/* Process list elements */
> -	state = 0;
> -	while (1) {
> -		if (state == 0) {
> -			if (*str == '\0')
> -				break;
> -			if (*str != ',') {
> -				str_start = str;
> -				state = 1;
> -			}
> -		} else if (state == 1) {
> -			if (*str == ',' || *str == '\0') {
> -				if (str > str_start) {
> -					/* Non-empty string fragment */
> -					*str = '\0';
> -					result = callback(str_start, data);
> -					if (result < 0)
> -						return result;
> -				}
> -				state = 0;
> -			}
> -		}
> -		str++;
> +	if (*len_list >= max_list)
> +		return -1;

If current length is equal to max, but added value is already
is in the list, it should not be an error. So, these two lines
should be moved after below for loop.

> +	for (i = 0; i < *len_list; i++) {
> +		if (list[i] == val)
> +			return 0;
>  	}
> +	list[(*len_list)++] = val;
>  	return 0;
>  }
>  
> -static int
> +static char *
>  rte_eth_devargs_process_range(char *str, uint16_t *list, uint16_t *len_list,
>  	const uint16_t max_list)
>  {
>  	uint16_t lo, hi, val;
>  	int result;
> +	char *pos = str;
>  
>  	result = sscanf(str, "%hu-%hu", &lo, &hi);
>  	if (result == 1) {
> -		if (*len_list >= max_list)
> -			return -ENOMEM;
> -		list[(*len_list)++] = lo;
> +		if (rte_eth_devargs_enlist(list, len_list, max_list, lo) != 0)
> +			return NULL;
>  	} else if (result == 2) {
> -		if (lo >= hi || lo > RTE_MAX_ETHPORTS || hi > RTE_MAX_ETHPORTS)

Strictly speaking removal of comparision vs RTE_MAX_ETHPORTS is
a separate logical change with a separate motivation.

> -			return -EINVAL;
> +		if (lo >= hi)

I'd remove '=' here. It should not be a problem and handed
perfectly by below code. I see no point to deny 3-3 range
which is an equivalent for just 3. It could be convenient
in some cases.

> +			return NULL;
>  		for (val = lo; val <= hi; val++) {
> -			if (*len_list >= max_list)
> -				return -ENOMEM;
> -			list[(*len_list)++] = val;
> +			if (rte_eth_devargs_enlist(list, len_list, max_list,
> +						   val) != 0)
> +				return NULL;
>  		}
>  	} else
> -		return -EINVAL;
> -	return 0;
> +		return NULL;
> +	while (*pos != 0 && ((*pos >= '0' && *pos <= '9') || *pos == '-'))

*post != '\0' is a bit better looking at subsequent
comparisons. Yes, it is just style. Up to you.

> +		pos++;

It looks too fragile. May I suggest to use %n in above scanf to
be able to skip only parsed characters.

> +	return pos;
> +}
> +
> +static char *
> +rte_eth_devargs_process_list(char *str, uint16_t *list, uint16_t *len_list,
> +	const uint16_t max_list)
> +{
> +	char *pos = str;
> +
> +	if (*pos == '[')
> +		pos++;
> +	while (1) {
> +		pos = rte_eth_devargs_process_range(pos, list, len_list,
> +						    max_list);
> +		if (pos == NULL)
> +			return NULL;
> +		if (*pos != ',') /* end of list */
> +			break;
> +		pos++;
> +	}
> +	if (*str == '[' && *pos != ']')
> +		return NULL;
> +	if (*pos == ']')
> +		pos++;
> +	return pos;
>  }
>  
>  /*
> @@ -121,6 +115,9 @@ rte_eth_devargs_parse_representor_ports(char *str, void *data)
>  	struct rte_eth_devargs *eth_da = data;
>  
>  	eth_da->type = RTE_ETH_REPRESENTOR_VF;
> -	return rte_eth_devargs_process_range(str, eth_da->representor_ports,
> +	str = rte_eth_devargs_process_list(str, eth_da->representor_ports,
>  		&eth_da->nb_representor_ports, RTE_MAX_ETHPORTS);

Not directly related to the patch, but I dislike
RTE_MAX_ETHPORTS above.
RTE_DIM(eth_da->representor_ports) would be more
readable.

> +	if (str == NULL)
> +		RTE_LOG(ERR, EAL, "wrong representor format: %s\n", str);
> +	return str == NULL ? -1 : 0;
>  }


[snip]