From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id F2AF2A051E; Wed, 10 Jun 2020 20:13:18 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 014B02C38; Wed, 10 Jun 2020 20:13:18 +0200 (CEST) Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by dpdk.org (Postfix) with ESMTP id 6FEEB2C16 for ; Wed, 10 Jun 2020 20:13:16 +0200 (CEST) X-Originating-IP: 86.246.31.132 Received: from u256.net (lfbn-idf2-1-566-132.w86-246.abo.wanadoo.fr [86.246.31.132]) (Authenticated sender: grive@u256.net) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 466FC1BF205; Wed, 10 Jun 2020 18:13:15 +0000 (UTC) Date: Wed, 10 Jun 2020 20:13:09 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Maxime Coquelin Cc: dev@dpdk.org, david.marchand@redhat.com, wenzhuo.lu@intel.com, beilei.xing@intel.com, bernard.iremonger@intel.com Message-ID: <20200610181309.s7pvscontdfwcxbu@u256.net> References: <20200608155330.163874-1-maxime.coquelin@redhat.com> <20200608155330.163874-3-maxime.coquelin@redhat.com> <20200610120641.xtfonyyjahx5wiqo@u256.net> <8c865cd1-80fd-7db3-d447-ef05c6615d4a@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8c865cd1-80fd-7db3-d447-ef05c6615d4a@redhat.com> Subject: Re: [dpdk-dev] [PATCH v20.11 2/2] eal: improve device probing API 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 10/06/20 19:08 +0200, Maxime Coquelin wrote: > Hi Gaëtan, > > On 6/10/20 2:06 PM, Gaëtan Rivet wrote: > > Hello Maxime, > > > > On 08/06/20 17:53 +0200, Maxime Coquelin wrote: > >> This patch makes rte_dev_probe() to return the > >> rte_device pointer on success and NULL on error > >> instead of returning 0 on success and negative > >> value on error. > >> > >> The goal is to avoid that the calling application > >> iterates the devices list afterwards to retrieve > >> the pointer. Retrieving the pointer is required > >> for calling rte_dev_remove() later. > >> > > > > I agree with the idea. I recall starting to do it on the legacy functions > > (rte_eal_hotplug_*), but it was scrapped for API compat. > > > >> Signed-off-by: Maxime Coquelin > >> --- > >> app/test-pmd/testpmd.c | 2 +- > >> drivers/net/failsafe/failsafe.c | 5 +++-- > >> lib/librte_eal/common/eal_common_dev.c | 18 ++++++++++-------- > >> lib/librte_eal/include/rte_dev.h | 4 ++-- > >> 4 files changed, 16 insertions(+), 13 deletions(-) > >> > ... > >> > >> int > >> diff --git a/lib/librte_eal/include/rte_dev.h b/lib/librte_eal/include/rte_dev.h > >> index c8d985fb5c..9cf7c7fd71 100644 > >> --- a/lib/librte_eal/include/rte_dev.h > >> +++ b/lib/librte_eal/include/rte_dev.h > >> @@ -148,9 +148,9 @@ int rte_eal_hotplug_add(const char *busname, const char *devname, > >> * @param devargs > >> * Device arguments including bus, class and driver properties. > >> * @return > >> - * 0 on success, negative on error. > >> + * Generic device pointer on success, NULL on error. > > > > If rte_errno is set, mapping codes to meanings would be helpful here. > > Actually David made me the same comment before I post the patch. > I am fine with setting rte_errno. If we do that, I think we should have > fixed error code in rte_dev_probe() and not propagate error codes from > functions it calls. Otherwise it's likely the API doc will be outdated > quite rapidly. > > What do you think? > Well we're stuck with the classic errno limitations. I agree with you, if we consider possible errors as part of a function API, then we cannot recursively inherit this API from callees. That being said, masking errors is not ok. If an error cannot be handled by rte_dev_probe(), it should log an appropriate message and set rte_errno to a value that is part of its API. If the error can be handled, then errno should be reset to its original value preceding rte_dev_probe() call. Of course the EAL rarely does it, and even myself I probably rarely respected this behavior, but it would be nice if we could all define a common agreed-upon discipline in the EAL and stick to it. I think the current state of error reporting in EAL is terrible for users downstream. > > Acked-by: Gaetan Rivet > > > > Great, could you also ack the deprecation notice, as this is the part > that needs to be merged into v20.08? > I wanted to refresh myself with the latest rules about API breakage before doing so but got context switched away :) . I will get back to it. > Thanks! > Maxime > -- Gaëtan