From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id B9554A04B6;
	Tue, 13 Oct 2020 10:40:35 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 4C83E1DA5C;
	Tue, 13 Oct 2020 10:40:34 +0200 (CEST)
Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113])
 by dpdk.org (Postfix) with ESMTP id 9E9011DA4E
 for <dev@dpdk.org>; Tue, 13 Oct 2020 10:40:32 +0200 (CEST)
Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17])
 (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 2A9EC7F4FD;
 Tue, 13 Oct 2020 11:40:31 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 2A9EC7F4FD
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru;
 s=default; t=1602578431;
 bh=3Rp7Lf9raEHfqlgD+3rJljdXsuhvd8JDShgTytBLSZs=;
 h=Subject:To:Cc:References:From:Date:In-Reply-To;
 b=IxQei+BTlJZH9AFHEHWHUdPrUyg6Hg15Vm/MXKjIRfxGZ6K79LQZSN2xf/J5cT3+y
 Q6RNXurzSP3zHaRBIxjp/qhoURZ63rEhzzcTGXEvUU3OCFV7SdzzZTQ1ao25fLkiiH
 rRrJi0MnswFhkJgls+SU8FfZhr04xU6LLmJJJ0oA=
To: Thomas Monjalon <thomas@monjalon.net>,
 Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org, arybchenko@solarflare.com, Liron Himi <lironh@marvell.com>, 
 Stephen Hemminger <stephen@networkplumber.org>, Ray Kinsella
 <mdr@ashroe.eu>, Neil Horman <nhorman@tuxdriver.com>,
 Rahul Lakkireddy <rahul.lakkireddy@chelsio.com>,
 Gaetan Rivet <grive@u256.net>, Jakub Grajciar <jgrajcia@cisco.com>,
 Matan Azrad <matan@nvidia.com>, Shahaf Shuler <shahafs@nvidia.com>,
 Viacheslav Ovsiienko <viacheslavo@nvidia.com>, Zyta Szpak <zr@semihalf.com>,
 Stephen Hemminger <sthemmin@microsoft.com>,
 "K. Y. Srinivasan" <kys@microsoft.com>,
 Haiyang Zhang <haiyangz@microsoft.com>, Long Li <longli@microsoft.com>,
 Maxime Coquelin <maxime.coquelin@redhat.com>,
 Chenbo Xia <chenbo.xia@intel.com>, Zhihong Wang <zhihong.wang@intel.com>
References: <20200913220711.3768597-1-thomas@monjalon.net>
 <20201005170820.1018715-4-thomas@monjalon.net>
 <1910c28d-bdab-29c4-a43a-66409f1c7b61@intel.com> <1850948.foQ0BKb2Qe@thomas>
From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Organization: OKTET Labs
Message-ID: <c5005e10-b58c-4c6b-c796-6f62d44df4bc@oktetlabs.ru>
Date: Tue, 13 Oct 2020 11:40:31 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.3.1
MIME-Version: 1.0
In-Reply-To: <1850948.foQ0BKb2Qe@thomas>
Content-Type: text/plain; charset=utf-8
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On 10/6/20 1:57 PM, Thomas Monjalon wrote:
> 06/10/2020 11:43, Ferruh Yigit:
>> On 10/5/2020 6:08 PM, Thomas Monjalon wrote:
>>> The API function rte_eth_dev_close() was returning void.
>>> The return type is changed to int for notifying of errors.
>>>
>>> If an error happens during a close operation,
>>> the status of the port is undefined,
>>> a maximum of resources having been freed.
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>> Reviewed-by: Liron Himi <lironh@marvell.com>
>>> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
>>
>> <...>
>>
>>> -void
>>> +int
>>>   rte_eth_dev_close(uint16_t port_id)
>>>   {
>>>   	struct rte_eth_dev *dev;
>>> +	int firsterr, binerr;
>>> +	int *lasterr = &firsterr;
>>>   
>>> -	RTE_ETH_VALID_PORTID_OR_RET(port_id);
>>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);

Shouldn't it be -ENODEV in fact?
IMHO it would be nice to make it uniform across ethdev API.

>>>   	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.