Re: [dpdk-dev] [PATCH v3 4/5] bus: add device arguments name parsing API

[Thomas Monjalon <thomas@monjalon.net>]

The commit log should start by explaining it is adding a callback
to the bus drivers for the new devargs syntax.

30/03/2021 14:15, Xueming Li:
> To use Global Device Syntax as devargs, name is required for device
> management.

Context is missing.
You mean the argument "name" for the vdev bus?

> 
> In legacy parsing API, devargs name was extracted after bus name:
>   bus:name,kv_params,,,
> 
> To parse new Global Device Syntax, this patch introduces new bus API to
> parse devargs and update name, different bus driver might choose
> different keys from parameters with unified formating, example:
>  -a bus=pci,addr=83:00.0/class=eth/driver=mlx5,...
>     name: 0000:03:00.0
>  -a bus=vdev,name=pcap0/class=eth/driver=pcap,...
>     name:pcap0

Only PCI and vdev buses are implemented.
What can be the plan for others?
We should track the progress somewhere, maybe with TODO comments.

This commit log could also state what is the status
of the global syntax support, talking about class and device drivers.

We could update this comment in ethdev:
     * A new syntax is in development (not yet supported):
     *   - bus=X,paramX=x/class=Y,paramY=y/driver=Z,paramZ=z

[...]
> +int
> +rte_pci_devargs_parse(struct rte_devargs *da)
> +{
> +	struct rte_kvargs *kvargs;
> +	const char *addr_str;
> +	struct rte_pci_addr addr;
> +	int ret;
> +
> +	if (da == NULL)
> +		return 0;
> +	RTE_ASSERT(da->bus_str != NULL);
> +
> +	kvargs = rte_kvargs_parse(da->bus_str, NULL);
> +	if (kvargs == NULL) {
> +		RTE_LOG(ERR, EAL, "cannot parse argument list: %s\n",
> +			da->bus_str);
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	addr_str = rte_kvargs_get(kvargs, pci_params_keys[RTE_PCI_PARAM_ADDR]);
> +	if (addr_str == NULL) {
> +		RTE_LOG(ERR, EAL, "No PCI address specified using '%s=<id>' in: %s\n",
> +			pci_params_keys[RTE_PCI_PARAM_ADDR], da->bus_str);
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	ret = rte_pci_addr_parse(addr_str, &addr);
> +	if (ret != 0) {
> +		RTE_LOG(ERR, EAL, "PCI address invalid: %s\n", da->bus_str);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	rte_pci_device_name(&addr, da->name, sizeof(da->name));
> +
> +	/* TODO: class parse -> driver parse */

Please could you give a longer explanation of what is missing?