[dpdk-dev] [PATCH 09/13] devargs: parse "bus=" argument

[Gaëtan Rivet]

I still disagree with parsing the body of devargs to retrieve bus
information. They should not be mixed.

This conceptual discrepancy can be seen plainly in the code structure:
you emulate librte_kvargs because you do not want to depend on it within
the EAL, but copy its behavior and are forced to rewrite it partially.

Can you justify this? The commitlog does not describe the problem being
solved.

I think there should be a discussion about the future format of device
declarations. One version has been integrated in RC1 but it will be reworked
anyway. I'd like to hear more opinions.

On Tue, Jul 11, 2017 at 07:25:08PM -0400, Jan Blunck wrote:
> Let the rte_eal_devargs_parse() function parse the "bus=" argument.
> 
> Signed-off-by: Jan Blunck <jblunck at infradead.org>
> ---
>  lib/librte_eal/common/eal_common_devargs.c | 131 ++++++++++++++++++-----------
>  test/test/test_devargs.c                   |  19 +++++
>  2 files changed, 102 insertions(+), 48 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_devargs.c b/lib/librte_eal/common/eal_common_devargs.c
> index 2bdee9a30..a0d0e6e91 100644
> --- a/lib/librte_eal/common/eal_common_devargs.c
> +++ b/lib/librte_eal/common/eal_common_devargs.c
> @@ -77,6 +77,40 @@ rte_eal_parse_devargs_str(const char *devargs_str,
>  	return 0;
>  }
>  
> +/*
> + * Parse "bus=" argument without adding a dependency on rte_kvargs.h
> + */
> +static char *parse_bus_arg(const char *args)
> +{
> +	const char *c;
> +	char *str = NULL;
> +
> +	if (!args || args[0] == '\0')
> +		return NULL;
> +
> +	c = args;
> +
> +	do {
> +		if (strncmp(c, "bus=", 4) == 0) {
> +			c += 4;
> +			break;
> +		}
> +
> +		c = strchr(c, ',');
> +		if (c)
> +			c++;
> +	} while (c);
> +
> +	if (c) {
> +		str = strdup(c);
> +		c = strchr(str, ',');
> +		if (c)
> +			c = '\0';
> +	}
> +
> +	return str;
> +}
> +
>  static int
>  devargs_add(const char *busname, const char *name, const char *args)
>  {
> @@ -113,64 +147,65 @@ devargs_add(const char *busname, const char *name, const char *args)
>  	return -1;
>  }
>  
> -static int
> -bus_name_cmp(const struct rte_bus *bus, const void *name)
> -{
> -	return strncmp(bus->name, name, strlen(bus->name));
> -}
> -
>  int
>  rte_eal_devargs_parse(const char *dev, struct rte_devargs *da)
>  {
> -	struct rte_bus *bus = NULL;
> -	const char *devname;
> -	const size_t maxlen = sizeof(da->name);
> -	size_t i;
> +	char *busname;
> +	char *devname;
> +	char *drvargs;
> +	int ret;
>  
>  	if (dev == NULL || da == NULL)
>  		return -EINVAL;
> -	/* Retrieve eventual bus info */
> -	do {
> -		devname = dev;
> -		bus = rte_bus_find(bus, bus_name_cmp, dev);
> -		if (bus == NULL)
> -			break;
> -		devname = dev + strlen(bus->name) + 1;
> -		if (rte_bus_find_by_device_name(devname) == bus)
> -			break;
> -	} while (1);
> -	/* Store device name */
> -	i = 0;
> -	while (devname[i] != '\0' && devname[i] != ',') {
> -		da->name[i] = devname[i];
> -		i++;
> -		if (i == maxlen) {
> -			fprintf(stderr, "WARNING: Parsing \"%s\": device name should be shorter than %zu\n",
> -				dev, maxlen);
> -			da->name[i - 1] = '\0';
> -			return -EINVAL;
> -		}
> -	}
> -	da->name[i] = '\0';
> -	if (bus == NULL) {
> -		bus = rte_bus_find_by_device_name(da->name);
> +
> +	ret = rte_eal_parse_devargs_str(dev, &devname, &drvargs);
> +	if (ret != 0)
> +		return ret;
> +
> +	RTE_LOG(DEBUG, EAL, "%s: adding devargs for device [%s]: %s\n",
> +		__FUNCTION__, devname, drvargs);
> +
> +	/* Retrieve eventual bus info (...,bus=...)*/
> +	busname = parse_bus_arg(drvargs);
> +	if (busname == NULL) {
> +		struct rte_bus *bus;
> +
> +		bus = rte_bus_find_by_device_name(devname);
>  		if (bus == NULL) {
> -			fprintf(stderr, "ERROR: failed to parse device \"%s\"\n",
> -				da->name);
> -			return -EFAULT;
> +			RTE_LOG(ERR, EAL, "failed to parse device [%s]\n",
> +				devname);
> +			ret = -EINVAL;
> +			goto fail;
>  		}
> +
> +		busname = strdup(bus->name);
>  	}
> -	da->bus = bus;
> -	/* Parse eventual device arguments */
> -	if (devname[i] == ',')
> -		da->args = strdup(&devname[i + 1]);
> -	else
> -		da->args = strdup("");
> -	if (da->args == NULL) {
> -		fprintf(stderr, "ERROR: not enough memory to parse arguments\n");
> -		return -ENOMEM;
> +
> +	ret = devargs_add(busname, devname, drvargs);
> +	if (ret != 0) {
> +		RTE_LOG(ERR, EAL, "devargs_add failed for device [%s]\n",
> +			devname);
> +		goto fail;
>  	}
> -	return 0;
> +
> +	ret = snprintf(da->busname, sizeof(da->busname), "%s", busname);
> +	if (ret < 0 || ret >= (int)sizeof(da->busname))
> +		goto fail;
> +
> +	ret = snprintf(da->name, sizeof(da->name), "%s", devname);
> +	if (ret < 0 || ret >= (int)sizeof(da->name))
> +		goto fail;
> +
> +	da->args = strdup(drvargs);
> +	if (da->args == NULL)
> +		ret = -ENOMEM;
> +	ret = 0;
> +
> +fail:
> +	free(busname);
> +	free(devname);
> +	free(drvargs);
> +	return ret;
>  }
>  
>  static const struct rte_bus_conf BUS_CONF_WHITELIST = {
> diff --git a/test/test/test_devargs.c b/test/test/test_devargs.c
> index 048b3d00b..3a1033ca3 100644
> --- a/test/test/test_devargs.c
> +++ b/test/test/test_devargs.c
> @@ -58,6 +58,7 @@ test_devargs(void)
>  {
>  	struct rte_devargs_list save_devargs_list;
>  	struct rte_devargs *devargs;
> +	struct rte_devargs devargs_stack;
>  
>  	/* save the real devargs_list, it is restored at the end of the test */
>  	save_devargs_list = devargs_list;
> @@ -109,6 +110,24 @@ test_devargs(void)
>  		goto fail;
>  	free_devargs_list();
>  
> +	if (rte_eal_devargs_parse("", &devargs_stack) == 0) {
> +		printf("Error in rte_eal_devargs_parse\n");
> +		goto fail;
> +	}
> +
> +	if (rte_eal_devargs_parse("08:00.1,foo=bar,bus=pci", &devargs_stack) != 0) {
> +		printf("Error in rte_eal_devargs_parse 08:00.1,bus=pci\n");
> +		goto fail;
> +	}
> +	devargs = TAILQ_FIRST(&devargs_list);
> +	if (strcmp(devargs->name, "08:00.1") != 0)
> +		goto fail;
> +	if (strcmp(devargs->busname, "pci") != 0)
> +		goto fail;
> +	if (!devargs->args || strcmp(devargs->args, "foo=bar,bus=pci") != 0)
> +		goto fail;
> +
> +	free_devargs_list();
>  	devargs_list = save_devargs_list;
>  	return 0;
>  
> -- 
> 2.13.2
> 

-- 
Gaëtan Rivet
6WIND