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 788CEA04B6; Tue, 13 Oct 2020 11:33:37 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4A5C71BF34; Tue, 13 Oct 2020 11:33:36 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 1EBDA1BF2F for ; Tue, 13 Oct 2020 11:33:33 +0200 (CEST) IronPort-SDR: KelnLAQjjbOWYN8HQfRbAQAXJ27xPBka0r0FCW6MWyEZpkM3cYo7cWMezoxO4iEmGnenhyZ0kG w3x2bcRVjsdA== X-IronPort-AV: E=McAfee;i="6000,8403,9772"; a="162416893" X-IronPort-AV: E=Sophos;i="5.77,369,1596524400"; d="scan'208";a="162416893" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Oct 2020 02:33:32 -0700 IronPort-SDR: 6OA+RBNnGLKZ4zeNnIoPskLE799dlqeRY01uI71riLtD72HejGrHb71Spb1Cv4G3bZbpdsoXoS JT6IPdYnJ6nA== X-IronPort-AV: E=Sophos;i="5.77,369,1596524400"; d="scan'208";a="530326995" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.214.64]) ([10.213.214.64]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Oct 2020 02:33:27 -0700 To: Thomas Monjalon , Andrew Rybchenko Cc: dev@dpdk.org, arybchenko@solarflare.com, Liron Himi , Stephen Hemminger , Ray Kinsella , Neil Horman , Rahul Lakkireddy , Gaetan Rivet , Jakub Grajciar , Matan Azrad , Shahaf Shuler , Viacheslav Ovsiienko , Zyta Szpak , Stephen Hemminger , "K. Y. Srinivasan" , Haiyang Zhang , Long Li , Maxime Coquelin , Chenbo Xia , Zhihong Wang References: <20200913220711.3768597-1-thomas@monjalon.net> <1850948.foQ0BKb2Qe@thomas> <13936658.LjDx0tDdet@thomas> From: Ferruh Yigit Message-ID: Date: Tue, 13 Oct 2020 10:33:23 +0100 MIME-Version: 1.0 In-Reply-To: <13936658.LjDx0tDdet@thomas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 3/3] ethdev: allow close function to return an error 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/13/2020 9:55 AM, Thomas Monjalon wrote: <...> >>>>> dev = &rte_eth_devices[port_id]; >>>>> >>>>> - RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_close); >>>>> - (*dev->dev_ops->dev_close)(dev); >>>>> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_close, -ENOTSUP); >>>>> + *lasterr = (*dev->dev_ops->dev_close)(dev); >>>>> + if (*lasterr != 0) >>>>> + lasterr = &binerr; >>>>> >>>>> rte_ethdev_trace_close(port_id); >>>>> - rte_eth_dev_release_port(dev); >>>>> + *lasterr = rte_eth_dev_release_port(dev); >>>>> + >>>>> + return firsterr; >>>>> } >>>> >>>> This may be personal taste but above error handling looks like unnecessary >>>> complex, what do you think something like below: >>>> >>>> close_err = (*dev->dev_ops->dev_close)(dev); >>>> release_err = rte_eth_dev_release_port(dev); >>>> return close_err ? close_err : release_err; >>> >>> This is what I did first. Then thought about this "elegant" handling. >>> The pointer solution is just one more line and is more future proof. >>> >>> I'm fine with any choice. Andrew? >>> >> >> Hm, really hard to make a choice. I tend to choose Thomas's >> version. Yes, I agree that it is a bit over-complicated in >> this particular case, but thoughts to be future-proof >> overweight for me. Plus it adds a pattern on how to handle >> such cases in the future. > > Yes it's an elegant pattern :) > OK :)