[dpdk-dev] [PATCH v7 6/6] devargs: parse bus info

[Gaëtan Rivet]

On Thu, Jul 06, 2017 at 02:37:16PM +0530, Shreyansh Jain wrote:
> Hello Gaetan,
> 
> On Wednesday 05 July 2017 05:25 AM, Gaetan Rivet wrote:
> >Signed-off-by: Gaetan Rivet <gaetan.rivet at 6wind.com>
> >---
> >  lib/librte_eal/common/eal_common_devargs.c  | 42 ++++++++++++++++++++++++-----
> >  lib/librte_eal/common/eal_common_vdev.c     |  6 +++--
> >  lib/librte_eal/common/include/rte_devargs.h |  3 +++
> >  3 files changed, 42 insertions(+), 9 deletions(-)
> >
> >diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> >index ffa8ad9..102bd8d 100644
> >--- a/lib/librte_eal/common/eal_common_devargs.c
> >+++ b/lib/librte_eal/common/eal_common_devargs.c
> >@@ -78,12 +78,23 @@ rte_eal_parse_devargs_str(const char *devargs_str,
> >  	return 0;
> >  }
> >+static int
> >+bus_name_cmp(const struct rte_bus *bus, const void *_name)
> >+{
> >+	const char *name = _name;
> >+
> >+	return strncmp(bus->name, name,
> >+		       strlen(bus->name));
> 
> Trivial: Any specific reason why this is split across multiple lines even
> though it is less than 80 chars in total?
> 

Not really, it's only a matter of taste.
If is mostly to hightlight that we are limiting the comparison to
bus->name length, by putting it on its own line.

> >+}
> >+
> >  /* store a whitelist parameter for later parsing */
> >  int
> >  rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
> >  {
> >  	struct rte_devargs *devargs = NULL;
> >-	char *buf = NULL;
> >+	struct rte_bus *bus = NULL;
> >+	char *dev = NULL;
> >+	const char *devname;
> >  	int ret;
> >  	/* use malloc instead of rte_malloc as it's called early at init */
> >@@ -94,34 +105,51 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)
> >  	memset(devargs, 0, sizeof(*devargs));
> >  	devargs->type = devtype;
> >-	if (rte_eal_parse_devargs_str(devargs_str, &buf, &devargs->args))
> >+	if (rte_eal_parse_devargs_str(devargs_str, &dev, &devargs->args))
> 
> A very basic (and possibly incorrect) question:
> 
> here, for example "'net_pcap0,rx_pcap=input.pcap,tx_pcap=output.pcap'" was
> passed to application, which would mean;
> 
> dev = "net_pcap0" (name of the device)
> devargs = "rx_pcap=input.pcap,tx_pcap=output.pcap"
> 
> >  		goto fail;
> >+	devname = dev;
> >+	do {
> >+		bus = rte_bus_find(bus, bus_name_cmp, dev);
> >+		if (bus == NULL)
> >+			break;
> >+		devname = dev + strlen(bus->name) + 1;
> 
> Assuming "vdev" bus:
>                       "net_pcap0"
>                             |
> devname points here --------' (4+1) chars from start of "net_pcap0".
> Is that the expectation?
> 

Yes :)
Well, actually, almost. The lines

> >+		bus = rte_bus_find(bus, bus_name_cmp, dev);
> >+		if (bus == NULL)
> >+			break;

Mean that at this point, we would already have broken out of the loop.
But to continue with your example, assuming that we have a bus that
is named "net_p":

> Probably I am missing something here (maybe the input already has a bus
> name.)
> 
> Or, if this is not the case, then we will have to change the test
> application (test_devargs.c) which passes such strings to
> "rte_eal_devargs_add".
> 
> >+		if (rte_bus_find_by_device_name(devname) == bus)
> 
> Obviously, if my assumption is correct, this fails. Or, maybe I am
> completely off the mark.
> 

It should not be necessary to update the tests (yet).
This bit of code tries to recognize bus names at the start of the
declaration. However, some device names might be ambiguous and start
like bus names for any reason. Device names have no specification beside
not having commas within, bus names have no specification at all.

Thus, we first try here to recognize a bus name within dev, but we do
not stop here. We also verify afterward that the resulting device would
be correct, and that its bus handler is actually the same as the bus we
first recognized.

In your example, the line


> >+        if (rte_bus_find_by_device_name(devname) == bus)

Fails, as the device is incorrect and rte_bus_find_by_device_name
returns NULL as no bus is able to handle a device named

> "cap0,rx_pcap=input.pcap,tx_pcap=output.pcap"

Considering this, we should break from this loop with no recognized bus.
As such, we enter afterward in the branch:

> >+			break;
> >+		devname = dev;

Note here that devname is set back to the start of dev.

> >+	} while (1);
> >+	if (bus == NULL) {
> >+		bus = rte_bus_find_by_device_name(devname);

Which will try to recognize a bus from the device name ("net_pcap0"),
which should succeed in recognizing vdev.

It is a little contrived, but I wanted this parsing to both be flexible
and perform as many checks as possible.

I am developing a new bus that have a high probability of name conflicts
with other device names, so I am extra careful on this side.

> >+		if (bus == NULL) {
> >+			fprintf(stderr, "ERROR: failed to parse bus info from device declaration\n");
> >+			goto fail;
> >+		}
> >+	}
> >+	devargs->bus = bus;
>   >   	switch (devargs->type) {
> >  	case RTE_DEVTYPE_WHITELISTED_PCI:
> >  	case RTE_DEVTYPE_BLACKLISTED_PCI:
> >  		/* try to parse pci identifier */
> >-		if (eal_parse_pci_BDF(buf, &devargs->pci.addr) != 0 &&
> >-		    eal_parse_pci_DomBDF(buf, &devargs->pci.addr) != 0)
> >+		if (bus->parse(devname, &devargs->pci.addr) != 0)
> >  			goto fail;
> >  		break;
> >  	case RTE_DEVTYPE_VIRTUAL:
> >  		/* save driver name */
> >  		ret = snprintf(devargs->virt.drv_name,
> >-			       sizeof(devargs->virt.drv_name), "%s", buf);
> >+			       sizeof(devargs->virt.drv_name), "%s", devname);
> >  		if (ret < 0 || ret >= (int)sizeof(devargs->virt.drv_name))
> >  			goto fail;
> >  		break;
> >  	}
> 
> I think all this will change in the devargs series.
> 

Indeed :)

> >-	free(buf);
> >+	free(dev);
> >  	TAILQ_INSERT_TAIL(&devargs_list, devargs, next);
> >  	return 0;
> >  fail:
> >-	free(buf);
> >+	free(dev);
> >  	if (devargs) {
> >  		free(devargs->args);
> >  		free(devargs);
> >diff --git a/lib/librte_eal/common/eal_common_vdev.c b/lib/librte_eal/common/eal_common_vdev.c
> >index 6ecd1b5..8fd1ef7 100644
> >--- a/lib/librte_eal/common/eal_common_vdev.c
> >+++ b/lib/librte_eal/common/eal_common_vdev.c
> >@@ -177,6 +177,7 @@ alloc_devargs(const char *name, const char *args)
> >  		return NULL;
> >  	devargs->type = RTE_DEVTYPE_VIRTUAL;
> >+	devargs->bus = rte_bus_find_by_name("vdev");
> >  	if (args)
> >  		devargs->args = strdup(args);
> >@@ -289,12 +290,13 @@ vdev_scan(void)
> >  {
> >  	struct rte_vdev_device *dev;
> >  	struct rte_devargs *devargs;
> >+	struct rte_bus *vbus;
> >  	/* for virtual devices we scan the devargs_list populated via cmdline */
> >-
> >+	vbus = rte_bus_find_by_name("vdev");
> >  	TAILQ_FOREACH(devargs, &devargs_list, next) {
> >-		if (devargs->type != RTE_DEVTYPE_VIRTUAL)
> >+		if (devargs->bus != vbus)
> >  			continue;
> >  		dev = find_vdev(devargs->virt.drv_name);
> >diff --git a/lib/librte_eal/common/include/rte_devargs.h b/lib/librte_eal/common/include/rte_devargs.h
> >index 88120a1..1f50a24 100644
> >--- a/lib/librte_eal/common/include/rte_devargs.h
> >+++ b/lib/librte_eal/common/include/rte_devargs.h
> >@@ -51,6 +51,7 @@ extern "C" {
> >  #include <stdio.h>
> >  #include <sys/queue.h>
> >  #include <rte_pci.h>
> >+#include <rte_bus.h>
> >  /**
> >   * Type of generic device
> >@@ -89,6 +90,8 @@ struct rte_devargs {
> >  			char drv_name[32];
> >  		} virt;
> >  	};
> >+	/** Bus handle for the device. */
> >+	struct rte_bus *bus;
> >  	/** Arguments string as given by user or "" for no argument. */
> >  	char *args;
> >  };
> >
> -
> Shreyansh

-- 
Gaëtan Rivet
6WIND