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 0E40D1B3E8 for ; Tue, 9 Oct 2018 11:49:30 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 9EB902201E; Tue, 9 Oct 2018 05:49:29 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Tue, 09 Oct 2018 05:49:29 -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=tIzK1qFEvIqzejUrJT4dAHBuq5d71kZdz/AoOiUT9uk=; b=PEgqCXNlqcz4 zdnhwZ0WEtKHAPVUd0ceTf2fJboJDfo/e6HI0L9+9an8pM7aImjZVBqI07kFsDUH V2cjKFC6RyIFMEbR1RdxWimCIztb/GYZ6HyK66PB7bRw1DOnTVpd3cR7Dp3Ntq+x /f+AtkUjlsF8pGaynocP/OoAFt6pNb0= 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=fm1; bh=tIzK1qFEvIqzejUrJT4dAHBuq5d71kZdz/AoOiUT9 uk=; b=Sk2kwNOS1yXNJqxOWrz053lCpZCiURxhufT1mjKXf0/6dNSErAcY2LzW8 nzx9G5e7fqQK3tcBOaTbAKxi92N5Y5y+DYdz0tatIQ6bp0q2lfbyVWBe7LyO0u2v Av8T93tilB6AZzf+7XTH6X3HqZNLMRyhetkeqJzSUkYbfeIYmfkePtJONVMnoJ7j pf1T6dvksa0q5TyfpHC58h/ZqGrC0xcPPwRDrzlcShkyk5mJ5J/PEhXQmgVv64Pg G5zDcVQRnJu/PShEB2r/Y05AZ0Lzu6/Qn6snQnv6z9aEGBdnHpN9d/JbMD0k0gmt CaF2KLF8srclBuECc/rNuhvkcU1Ug== 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 03CC5E454F; Tue, 9 Oct 2018 05:49:27 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: dev@dpdk.org, gaetan.rivet@6wind.com, ophirmu@mellanox.com, ferruh.yigit@intel.com Date: Tue, 09 Oct 2018 11:49:26 +0200 Message-ID: <1711996.UYlBJmHL7p@xps> In-Reply-To: <8b6df6a2-93ea-396f-c35e-31ae710c30c9@solarflare.com> References: <20181007222554.4886-1-thomas@monjalon.net> <20181009001616.10497-3-thomas@monjalon.net> <8b6df6a2-93ea-396f-c35e-31ae710c30c9@solarflare.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v2 2/6] ethdev: add iterator to match 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: Tue, 09 Oct 2018 09:49:30 -0000 09/10/2018 11:31, Andrew Rybchenko: > On 10/9/18 3:16 AM, Thomas Monjalon wrote: > > if (str != NULL) { > > - kvargs = rte_kvargs_parse(str, eth_params_keys); > > + if (str[0] == '+') /* no validation of keys */ > > + str ++; > > As I understand it should be no space before ++ Yes, I ran checkpatch after sending the patches :( [...] > > + /* > > + * Assume parameters of old syntax can match only at ethdev level. > > + * Extra parameters will be ignored, thanks to "+" prefix. > > + */ > > + str_size = strlen(devargs.args) + 2; > > + cls_str = malloc(str_size); > > + if (cls_str == NULL) { > > + ret = -ENOMEM; > > + goto error; > > + } > > + ret = snprintf(cls_str, str_size, "+%s", devargs.args); > > + if (ret < 0) { > > As I understand we expect ret == str_size - 1 here. May be it makes sent > to harden the check. No strong opinion. OK I'll change the check. > > + ret = -EINVAL; > > + goto error; > > + } > > + iter->cls_str = cls_str; > > + free(devargs.args); /* allocated by rte_devargs_parse() */ > > + devargs.args = NULL; > > + > > + iter->bus = devargs.bus; > > + if (iter->bus->dev_iterate == NULL) { > > + ret = -ENOTSUP; > > + goto error; > > + } > > + > > + /* Convert bus args to new syntax for use with new API dev_iterate. */ > > + if (strcmp(iter->bus->name, "vdev") == 0) > > + bus_param_key = "name"; > > + else if (strcmp(iter->bus->name, "pci") == 0) > > + bus_param_key = "addr"; > > I thought that if one branch has curly brackets other branches should > have curly brackets as well. Yes, I don't like this coding rule but I will change. > > + else { > > + ret = -ENOTSUP; > > + goto error; > > + } > > + str_size = strlen(bus_param_key) + strlen(devargs.name) + 2; > > + bus_str = malloc(str_size); > > + if (bus_str == NULL) { > > + ret = -ENOMEM; > > + goto error; > > + } > > + ret = snprintf(bus_str, str_size, "%s=%s", > > + bus_param_key, devargs.name); > > + if (ret < 0) { > > May be it makes sense to make the check more strict: ret != str_size - 1 OK > > + ret = -EINVAL; > > + goto error; > > + } [...] > > +void __rte_experimental > > +rte_eth_iterator_free(struct rte_dev_iterator *iter) > > Yes, I know that the name is suggested by me, but maybe > it should be rte_eth_iterator_fini() or _cleanup() as pair to _init. Yes, you're right, it is not freeing the whole structure. I will name it "cleanup", and will use memset to reset all fields. > > +{ > > + free(RTE_CAST_FIELD(iter, bus_str, char *)); /* workaround const */ > > + iter->bus_str = NULL; > > + free(RTE_CAST_FIELD(iter, cls_str, char *)); /* workaround const */ > > + iter->cls_str = NULL; > > +} [...] > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change without prior notice. > > + * > > + * Free some allocated fields of the iterator. > > + * > > + * This function is automatically called by rte_eth_iterator_next() > > + * on the last iteration (i.e. when no more matching port is found). > > + * > > May be it makes sense to mention that it is safe to call it twice. > It could be simpler to use it taking the possibility into account. OK, good idea. > > + * @param iter > > + * Device iterator handle initialized by rte_eth_iterator_init(). > > + * The fields bus_str and cls_str are freed if needed. > > + */ > > +__rte_experimental > > +void rte_eth_iterator_free(struct rte_dev_iterator *iter); [...] > > --- a/lib/librte_ethdev/rte_ethdev_version.map > > +++ b/lib/librte_ethdev/rte_ethdev_version.map > > @@ -237,6 +237,8 @@ EXPERIMENTAL { > > rte_eth_dev_owner_unset; > > rte_eth_dev_rx_offload_name; > > rte_eth_dev_tx_offload_name; > > + rte_eth_iterator_init; > > + rte_eth_iterator_next; > > rte_eth_iterator_free() or renamed Yes, good catch! As usual, thanks for the good review Andrew.