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

[Andrew Rybchenko]

On 1/11/24 09:44, 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],representor=pf2vf[1,2-3],representor=[4-5]

Signed-off-by: Harman Kalra <hka...@marvell.com>

[snip]

diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index bd917a15fc..62a06b75a2 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -470,15 +470,14 @@ eth_dev_devargs_tokenise(struct rte_kvargs *arglist, 
const char *str_in)
  }

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)
  {
-       struct rte_kvargs args;
+       struct rte_eth_devargs *eth_da;
        struct rte_kvargs_pair *pair;
-       unsigned int i;
+       struct rte_kvargs args;
+       unsigned int i, j = 0;

Please, avoid multiple variable in one line declaration with
initialization. It is too error prone. Also j is a bad name
here. It is not a loop counter or seomthing like this.
Maybe 'parsed' or 'devargs_parsed'?

        int result = 0;

- memset(eth_da, 0, sizeof(*eth_da));

-
        result = eth_dev_devargs_tokenise(&args, dargs);
        if (result < 0)
                goto parse_cleanup;
@@ -486,18 +485,16 @@ rte_eth_devargs_parse(const char *dargs, struct 
rte_eth_devargs *eth_da)
        for (i = 0; i < args.count; i++) {
                pair = &args.pairs[i];
                if (strcmp("representor", pair->key) == 0) {
-                       if (eth_da->type != RTE_ETH_REPRESENTOR_NONE) {
-                               RTE_ETHDEV_LOG_LINE(ERR, "duplicated representor 
key: %s",
-                                       dargs);
-                               result = -1;
-                               goto parse_cleanup;
-                       }
+                       eth_da = &eth_devargs[j];
+                       memset(eth_da, 0, sizeof(*eth_da));
                        result = rte_eth_devargs_parse_representor_ports(
                                        pair->value, eth_da);
                        if (result < 0)
                                goto parse_cleanup;
+                       j++;
                }
        }
+       result = (int)j;

parse_cleanup:

        free(args.str);
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index b482cd12bb..6f4f1a1606 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1800,10 +1800,10 @@ rte_eth_representor_id_get(uint16_t port_id,
   * @param devargs
   *  device arguments
   * @param eth_devargs
- *  parsed ethdev specific arguments.
+ *  contiguous memory populated with parsed ethdev specific arguments.

Caller must provide information about array size. Right now you simply
introduce out-of-bound array access.
All drivers just provide one entry and if I pass many-many devargs I
can write far beyond that location and corrupt stack.

   *
   * @return
- *   Negative errno value on error, 0 on success.
+ *   Negative errno value on error, no of devargs parsed on success.
   */
  __rte_internal
  int