From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CB0D0A0A0C; Thu, 15 Apr 2021 10:15:07 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 94EBA1620E6; Thu, 15 Apr 2021 10:15:07 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 09AD01620E4 for ; Thu, 15 Apr 2021 10:15:06 +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) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 2E9967F50E; Thu, 15 Apr 2021 11:15:06 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 2E9967F50E DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1618474506; bh=HH0SGFpVDG7fEXKSo8348Xxmd/RSx4sUDCQA1SNVWYU=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=PUOXPqsypiC8wwBeKwMecsm7FnF+P2kEvplCgqjVGZNvVBf6LOn/Jld3tOi2X5c6P PKKh4MX83wdYt/kWYgKe1eZ6Zp6SiAm6VGaDgaX9HAGP5xi/JsveY/HQHx/zl6nx2N 3CqpbEiUDvHrmMQJepv5weCnXI1W77sbuGp8cQM0= To: "Min Hu (Connor)" , dev@dpdk.org Cc: ferruh.yigit@intel.com, thomas@monjalon.net References: <1618046334-39857-1-git-send-email-humin29@huawei.com> <1618447925-12168-1-git-send-email-humin29@huawei.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Thu, 15 Apr 2021 11:15:05 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <1618447925-12168-1-git-send-email-humin29@huawei.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4] ethdev: add sanity checks in control APIs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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 4/15/21 3:52 AM, Min Hu (Connor) wrote: > This patch adds more sanity checks in control path APIs. > > Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input") > Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables") > Fixes: 0366137722a0 ("ethdev: check for invalid device name") > Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model") > Fixes: 5b7ba31148a8 ("ethdev: add port ownership") > Fixes: f8244c6399d9 ("ethdev: increase port id range") > Cc: stable@dpdk.org Many thanks. I'll keep log messages gramma review to native speekers. Content looks good to me. One minor issue below lost on previous review. Other than that, Reviewed-by: Andrew Rybchenko > > Signed-off-by: Min Hu (Connor) [snip] > @@ -1299,6 +1337,12 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > > + if (dev_conf == NULL) { > + RTE_ETHDEV_LOG(ERR, "Failed to configure ethdev port %u to NULL\n", > + port_id); > + return -EINVAL; > + } > + > dev = &rte_eth_devices[port_id]; I think it is better to keep: RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); dev = &rte_eth_devices[port_id]; together since they are tightly related. I.e. I'd even remove empty line between them when it is present and add args sanity check after the dev assignment. > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP); In theory, the first argument is sufficient to make the ops check, but I think it is the right solution to keep it as is since current tendency is to check operation support when driver callback is really required and we're going to use it. However, if we do it just after port_id check, we'll have a way to check for callback support without any side effects if we provide invalid argument value. I.e. if -ENOTSUP is returned, callback is not supported, if -EINVAL, callback is supported (but argument is invalid and nothing done). However, it looks a bit fragile and not always possible. Thoughts on it are welcome.