From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH 2/5] ethdev: add an iterator to match some
	devargs input
Date: Mon, 08 Oct 2018 09:58:44 +0200
Message-ID: <2689730.oytcUrqQT6@xps>
References: <20181007222554.4886-1-thomas@monjalon.net>
 <20181007222554.4886-3-thomas@monjalon.net>
 <8689ac9f-c5dd-aa29-8f8b-e32674338160@solarflare.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7Bit
Cc: dev@dpdk.org, gaetan.rivet@6wind.com, ophirmu@mellanox.com,
 ferruh.yigit@intel.com
To: Andrew Rybchenko <arybchenko@solarflare.com>
Return-path: <dev-bounces@dpdk.org>
Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com
 [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id C33A7DE3
 for <dev@dpdk.org>; Mon,  8 Oct 2018 09:58:47 +0200 (CEST)
In-Reply-To: <8689ac9f-c5dd-aa29-8f8b-e32674338160@solarflare.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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!