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!
