Re: [PATCH 2/5] ethdev: add an iterator to match some devargs input

[Thomas Monjalon <thomas@monjalon.net>]

08/10/2018 09:06, Andrew Rybchenko:
> On 10/8/18 1:25 AM, Thomas Monjalon wrote:
> > --- a/lib/librte_eal/common/include/rte_common.h
> > +++ b/lib/librte_eal/common/include/rte_common.h
> > @@ -164,6 +164,12 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
> >    */
> >   #define RTE_PTR_DIFF(ptr1, ptr2) ((uintptr_t)(ptr1) - (uintptr_t)(ptr2))
> >   
> > +/**
> > + * Workaround to cast a const field of a structure to non-const type.
> > + */
> > +#define RTE_CAST_FIELD(var,field,type) \
> 
> Missing space after each comma.
> 
> > +	(*(type*)((uintptr_t)(var) + offsetof(typeof(*(var)), field)))
> > +
> 
> In general it is bad symptom that we need it. I'd think more that twice
> before adding it :)

Yes, I tried to remove const in the struct but it brings other problems
when assigning const to the non-const fields.
And I think it was a good idea (from Gaetan) to give const hint to these
fields as they are not changed at each iteration.

So yes, it is a nasty hack, but I feel it is the best tradeoff.

[...]
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > +	/* Split bus, device and parameters. */
> > +	ret = rte_devargs_parse(&devargs, devargs_str);
> 
> rte_devargs_parse() does strdup() for args. It requires free() in the 
> function
> if devargs parsed successfully, but init fails.
> In the case of success it is moved to cls_str which is 'const' and I see
> not code which frees it as well.

Oh yes, you're right!

[...]
> > +	do { /* loop for matching rte_device */
> > +		if (iter->class_device == NULL) {
> > +			iter->device = iter->bus->dev_iterate(
> > +					iter->device, iter->bus_str, iter);
> > +			if (iter->device == NULL)
> > +				break;
> > +		}
> > +		iter->class_device = iter->cls->dev_iterate(
> > +				iter->class_device, iter->cls_str, iter);
> > +		if (iter->class_device != NULL)
> > +			return eth_dev_to_id(iter->class_device);
> > +	} while (iter->class_device == NULL);
> 
> It is non-obvious what is happening above. It would be very useful to
> add comments which explains it. May be just hints.

OK I will try to add good comments.

> > +
> > +	/* No more ethdev port to iterate. */
> > +	free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */
> > +	iter->bus_str = NULL;
> 
> Who does it if RTE_ETH_FOREACH_MATCHING_DEV caller breaks
> the loop before reaching maximum?

If the app breaks the loop, it becomes a responsibility of the app.
It is documented for the functions but not the macro.
I should add a comment in the doxygen of the macro, thanks.

[...]
> > +#define RTE_ETH_FOREACH_MATCHING_DEV(id, devargs, iter) \
> > +	for (rte_eth_iterator_init(iter, devargs), \
> > +	     id = rte_eth_iterator_next(iter); \
> > +	     id != RTE_MAX_ETHPORTS; \
> > +	     id = rte_eth_iterator_next(iter))
> > +
> 
> Such iterators are very convenient, but in this particular case
> it is a source of non-obvious memory leaks and necessity
> of the hack to discard 'const'.
> 
> May be iterator free callback with its own opaque data
> can help to avoid these hacks with const discard?
> I.e. rte_eth_iterator_free() which does the job and mentioned
> in the rte_eth_iterator_init() and the macro description.

Yes, definitely, I will add rte_eth_iterator_free().

Thanks for the very good review!