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 = ð_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]
