From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id C33A7DE3 for ; Mon, 8 Oct 2018 09:58:47 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id E044021910; Mon, 8 Oct 2018 03:58:46 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Mon, 08 Oct 2018 03:58:46 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=eOUw9DskTpaupnhcPNqNAOuoJBmjyr6pSihdjNb+rmA=; b=OSKbW/Swc4QP ox5XpZ/co8uprkQvCpyosL09QoIFbZkYjqnHmHWynefKwr08374QLQeEI8mXcVTu klTeL+HREXFyMUvhJVtR+UKNaTFncMXqMAiuGFrkX3i0g9mUotESZQSd/VWTtgzN 8WSFZs2OCLZh+WJ7XZg1iECVF8xYUlE= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=eOUw9DskTpaupnhcPNqNAOuoJBmjyr6pSihdjNb+r mA=; b=kwEO0lcOccuIoM1NQE26TpTl+HjpdTGANxsJS+z45iyHH0BrbT4TzKExJ CN8HUdxakoHNIlftX6Gob+LEeXzGXCIcc0rhaFgdLcOit3WtzHCM6p7bgi1NsKXp iccO72H6ko8VuJ8w/pzJ/2B2y82BauOdDL2N59WVNy3tx04DIS0tR1Ot/hHmJOYA tz2fohDjbA0/JVCBYTZ7gd3gtaDCblh5VbVHPsH8B1CKG0hGKj/lnwLQx/k5a24k n5PviB5MYm/TB1sX77wCTjweqpKEDMoqPhqnQW3HSYS7WbP6f4DvKYkVgXCl09Ep /0fY7lpNZNxWy9aOxL746zUTbZw3w== X-ME-Sender: X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id A9736E4240; Mon, 8 Oct 2018 03:58:45 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: dev@dpdk.org, gaetan.rivet@6wind.com, ophirmu@mellanox.com, ferruh.yigit@intel.com Date: Mon, 08 Oct 2018 09:58:44 +0200 Message-ID: <2689730.oytcUrqQT6@xps> In-Reply-To: <8689ac9f-c5dd-aa29-8f8b-e32674338160@solarflare.com> 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-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH 2/5] ethdev: add an iterator to match some devargs input X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Oct 2018 07:58:48 -0000 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!