From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH 3/5] ethdev: allow iterating with only class filter Date: Mon, 08 Oct 2018 10:07:16 +0200 Message-ID: <3841557.hIBcaeYvXo@xps> References: <20181007222554.4886-1-thomas@monjalon.net> <20181007222554.4886-4-thomas@monjalon.net> 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 Return-path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id AD3B31DB8 for ; Mon, 8 Oct 2018 10:07:19 +0200 (CEST) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 08/10/2018 09:20, Andrew Rybchenko: > On 10/8/18 1:25 AM, Thomas Monjalon wrote: > > If no rte_device is given in the iterator, > > eth_dev_match() is looking at all ports without any restriction, > > except the ethdev kvargs filter. > > > > It allows to iterate with a devargs filter referencing only > > some ethdev parameters. The format (from the new devargs syntax) is: > > class=eth,paramY=Y > > > > Fixes: e815a7f69371 ("ethdev: register as a class") > > > > Signed-off-by: Thomas Monjalon > > --- > > --- a/lib/librte_ethdev/rte_class_eth.c > > +++ b/lib/librte_ethdev/rte_class_eth.c > > @@ -42,7 +42,7 @@ eth_dev_match(const struct rte_eth_dev *edev, > > > > if (edev->state == RTE_ETH_DEV_UNUSED) > > return -1; > > - if (edev->device != arg->device) > > + if (arg->device != NULL && arg->device != edev->device) > > return -1; > > if (kvlist == NULL) > > /* Empty string matches everything. */ > > It looks like it is the only hunk which > Fixes: e815a7f69371 ("ethdev: register as a class") Yes this hunk is fixing above commit. > everything else adjusts the previous patch. Yes but the whole goal of this patch is to allow ethdev pure filter. All is related in this patch. > I think this fix should go before and the rest should be squashed > in the previous patch. It was really questionable why it is safe > to dereference iter->bus without checking that it is not NULL. No iter->bus was safe because iter->cls was checked before and implied that iter->bus was successfully set. I still think it is better for understanding to split different kind of filters in 2 patches. I may remove the Fixes line however. Opinion? [...] > > + /* Handle a case from future syntax, without any bus-level argument. */ > > + if (strncmp(devargs_str, iter_anybus_str, > > + strlen(iter_anybus_str)) == 0) { > > + iter->cls_str = devargs_str + strlen(iter_anybus_str); > > + goto end; > > + } > > + > > It looks like a hack, but I guess we need it since rte_devargs_parse() > cannot handle the case. May be it is acceptable if we have no time > to solve it, but it would be good to highlight it better in the comments. This function is a mix of old and new syntax. When only new syntax will be supported, it will be better. It is a work in progress. I do not know how to better explain it.