[dpdk-dev] [PATCH v7 5/6] bus: add helper to find a bus from a device name

[Shreyansh Jain]

Hello Gaetan,

Some comments inline:

On Wednesday 05 July 2017 05:25 AM, Gaetan Rivet wrote:
> Find which bus should be able to parse this device name into an internal
> device representation.
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet at 6wind.com>
> ---
>   lib/librte_eal/common/eal_common_bus.c | 21 +++++++++++++++++++++
>   lib/librte_eal/common/eal_private.h    | 12 ++++++++++++
>   2 files changed, 33 insertions(+)
> 
> diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> index 87b0c6e..34fcfa1 100644
> --- a/lib/librte_eal/common/eal_common_bus.c
> +++ b/lib/librte_eal/common/eal_common_bus.c
> @@ -204,3 +204,24 @@ rte_bus_find_by_name(const char *busname)
>   {
>   	return rte_bus_find(NULL, cmp_bus_name, (const void *)busname);
>   }
> +
> +static int
> +bus_can_parse(const struct rte_bus *bus, const void *_name)
> +{
> +	const char *name = _name;
> +
> +	return !(bus->parse && bus->parse(name, NULL) == 0);
> +}
> +
> +struct rte_bus *
> +rte_bus_find_by_device_name(const char *str)
> +{
> +	char name[32];

It is possible to use a constant here? Basically, I am not sure why '32' 
has been chosen - or maybe, it might remind a reader in future the 
reason for this value.

Just to clarify: is there any documented limit on bus name? Until this 
point, the name (and length) of bus was responsibility of bus driver 
implementation. eal_common_bus.c doesn't codify any limit - so, this may 
have to be advertised, even if just within the code.

> +	char *c;
> +
> +	snprintf(name, sizeof(name), "%s", str);
> +	c = strchr(name, ',');

I saw a lot of discussion on the naming assumptions/presumptions. 
Though, I am not sure I have an understood conclusion from that.
So, this might be incorrect/ill-informed:

Is it assumed that ',' is not present in device name?

Do you think it would be better if this is documented that ',' in a 
device name is reserved (as per devargs)? (or, is this already known?)

For example, in case of DPAA2 devices, I didn't consider this as a case 
while generating names while scanning the bus (though, I didn't have a 
',' in the name). But, if a well known fact, bus drivers can normalize 
their device names before creating device names.

> +	if (c != NULL)
> +		c[0] = '\0';
> +	return rte_bus_find(NULL, bus_can_parse, name);
> +}
> diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
> index 6cacce0..0836339 100644
> --- a/lib/librte_eal/common/eal_private.h
> +++ b/lib/librte_eal/common/eal_private.h
> @@ -338,4 +338,16 @@ int rte_eal_hugepage_attach(void);
>    */
>   bool rte_eal_using_phys_addrs(void);
>   
> +/**
> + * Find a bus capable of identifying a device.
> + *
> + * @param str
> + *   A device identifier (PCI address, virtual PMD name, ...).
> + *
> + * @return
> + *   A valid bus handle if found.
> + *   NULL if no bus is able to parse this device.
> + */
> +struct rte_bus *rte_bus_find_by_device_name(const char *str);
> +
>   #endif /* _EAL_PRIVATE_H_ */
> 

(Apologies for commenting so late in series - feel free to ignore if 
this disrupts your cycle or sounds out-of-context.)

-
Shreyansh