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

[Harman Kalra <hkalra@marvell.com>]

Hi Andrew,

Thanks for the review comments.
Please see responses inline.
Kindly review V4 as well.

> -----Original Message-----


<snip>

> > @@ -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.

Sorry I missed that, added in V4

> 
> > +				     (*(letter + 2) == 'v' && *(letter + 3) == 'f')
> ||
> > +				     (*(letter + 2) == 's' && *(letter + 3) == 'f')
> ||
> 
> may be it is better to use strncmp() instead?.

Yes strncmp can be used but I kept as is for symmetry with other comparisons.
Moreover I needed 2nd and 3rd letter comparison from current position, so just
for ease I kept as is.

> IMHO it is a bit hard to follow
I reworded the comment in V4 to explain the changes, I hope it is making sense now.


> 
> > +				     (*(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()

Thanks, changed in V4

> 
> > +	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.

Ack, changed in V4

Thanks
Harman

> 
> [snip]