[dpdk-dev] [PATCH v5 01/12] bus: add bus iterator to find a bus

[Gaëtan Rivet]

Hi Bruce,

Thanks for reading.

On Mon, Jun 26, 2017 at 04:30:55PM +0100, Bruce Richardson wrote:
> On Mon, Jun 26, 2017 at 02:21:59AM +0200, Gaetan Rivet wrote:
> > From: Jan Blunck <jblunck at infradead.org>
> > 
> > This helper allows to iterate over all registered buses and find one
> > matching data used as parameter.
> > 
> > Signed-off-by: Jan Blunck <jblunck at infradead.org>
> > Signed-off-by: Gaetan Rivet <gaetan.rivet at 6wind.com>
> > ---
> >  lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  1 +
> >  lib/librte_eal/common/eal_common_bus.c          | 20 ++++++++++++
> >  lib/librte_eal/common/include/rte_bus.h         | 43 +++++++++++++++++++++++++
> >  lib/librte_eal/linuxapp/eal/rte_eal_version.map |  1 +
> >  4 files changed, 65 insertions(+)
> > 
> 
> Two minor suggestions below. Otherwise:
> 
> Acked-by: Bruce Richardson <bruce.richardson at intel.com>
> 
> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > index 2e48a73..ed09ab2 100644
> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > @@ -162,6 +162,7 @@ DPDK_17.02 {
> >  DPDK_17.05 {
> >  	global:
> >  
> > +	rte_bus_find;
> >  	rte_cpu_is_supported;
> >  	rte_log_dump;
> >  	rte_log_register;
> > diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> > index 8f9baf8..4619eb2 100644
> > --- a/lib/librte_eal/common/eal_common_bus.c
> > +++ b/lib/librte_eal/common/eal_common_bus.c
> > @@ -145,3 +145,23 @@ rte_bus_dump(FILE *f)
> >  		}
> >  	}
> >  }
> > +
> > +struct rte_bus *
> > +rte_bus_find(rte_bus_cmp_t cmp,
> > +	     const void *data,
> > +	     const struct rte_bus *start)
> > +{
> > +	struct rte_bus *bus = NULL;
> > +	int started = start == NULL;
> 
> Please put brackets around the "start == NULL" to improve readability.
> My first reading of this I assumed it was doing multiple assignment of
> both start and started to NULL. To make it extra clear, prefix the
> comparison with "!!".
> 
> > +
> > +	TAILQ_FOREACH(bus, &rte_bus_list, next) {
> > +		if (!started) {
> > +			if (bus == start)
> > +				started = 1;
> > +			continue;
> > +		}
> > +		if (cmp(bus, data) == 0)
> > +			break;
> > +	}
> > +	return bus;
> > +}
> 
> I also think the name "started" is confusing, and might be better as the
> slighly longer "start_found".
> 

Agreed on both account, it will be fixed in the next version.

-- 
Gaëtan Rivet
6WIND