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

[Andrew Rybchenko]

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