Re: [PATCH v3 1/1] ethdev: parsing multiple representor devargs string

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

On 1/16/24 12:55, Harman Kalra wrote:
> Adding support for parsing multiple representor devargs strings
> passed to a PCI BDF. There may be scenario where port representors
> for various PFs or VFs under PFs are required and all these are
> representor ports shall be backed by single pci device. In such
> case port representors can be created using devargs string:
> <PCI BDF>,representor=[pf[0-1],pf2vf[1,2-3],[4-5]]
> 
> Signed-off-by: Harman Kalra <hkalra@marvell.com>

see below

> diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
> index bd917a15fc..8a49511516 100644
> --- a/lib/ethdev/ethdev_driver.c
> +++ b/lib/ethdev/ethdev_driver.c
> @@ -2,6 +2,7 @@
>    * Copyright(c) 2022 Intel Corporation
>    */
>   
> +#include <ctype.h>
>   #include <stdlib.h>
>   #include <pthread.h>
>   
> @@ -459,9 +460,23 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
>   			break;
>   
>   		case 3: /* Parsing list */
> -			if (*letter == ']')
> -				state = 2;
> -			else if (*letter == '\0')
> +			if (*letter == ']') {
> +				/* Multiple representor case has ']' dual meaning, first end of
> +				 * individual pfvf list and other end of consolidated list of
> +				 * representors.
> +				 * Complete multiple representors list to be considered as one
> +				 * pair value.
> +				 */
> +				if ((strcmp("representor", pair->key) == 0) &&
> +				    ((*(letter + 2) == 'p' && *(letter + 3) == 'f')   ||

Sorry, but it is unclear why it is not out-of-bound access.

> +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')   ||
> +				     (*(letter + 2) == 's' && *(letter + 3) == 'f')   ||

may be it is better to use strncmp() instead? IMHO it is a bit hard to
follow.

> +				     (*(letter + 2) == 'c' && isdigit(*(letter + 3))) ||
> +				     (*(letter + 2) == '[' && isdigit(*(letter + 3)))))
> +					state = 3;
> +				else
> +					state = 2;
> +			} else if (*letter == '\0')
>   				return -EINVAL;
>   			break;
>   		}
> @@ -469,16 +484,56 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, const char *str_in)
>   	}
>   }
>   
> +static int
> +eth_dev_tokenise_representor_list(char *p_val, struct rte_eth_devargs *eth_devargs,
> +				  uint8_t nb_da)
> +{
> +	struct rte_eth_devargs *eth_da;
> +	char da_val[BUFSIZ];
> +	char delim[] = "]";
> +	int devargs = 0;
> +	int result = 0;
> +	char *token;
> +
> +	token = strtok(&p_val[1], delim);

since strtok() is MT-unsafe, I'd recommend to use strtok_r()

> +	while (token != NULL) {
> +		eth_da = &eth_devargs[devargs];
> +		memset(eth_da, 0, sizeof(*eth_da));
> +		snprintf(da_val, BUFSIZ, "%s%c", (token[0] == ',') ? ++token : token, ']');
> +		/* Parse the tokenised devarg value */
> +		result = rte_eth_devargs_parse_representor_ports(da_val, eth_da);
> +		if (result < 0)
> +			goto parse_cleanup;
> +		devargs++;
> +		if (devargs > nb_da) {
> +			RTE_ETHDEV_LOG_LINE(ERR,
> +					    "Devargs parsed %d > max array size %d",
> +					    devargs, nb_da);
> +			result = -1;
> +			goto parse_cleanup;
> +		}
> +		token = strtok(NULL, delim);
> +	}
> +
> +	result = devargs;
> +
> +parse_cleanup:
> +	return result;
> +
> +}
> +
>   int
> -rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
> +rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_devargs,
> +		      uint8_t nb_da)

I see no single reason to limit nb_da to uint8_t type. IMHO it should be
'unsigned int' as an unsigned number of default type.
'unsigned int' is used for number of stats and ptypes in array.

[snip]