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 65816A0547; Wed, 21 Apr 2021 13:28:47 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E9FD141A72; Wed, 21 Apr 2021 13:28:46 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 9C1F3419EC; Wed, 21 Apr 2021 13:28:45 +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 CA5CA7F50B; Wed, 21 Apr 2021 14:28:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru CA5CA7F50B DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1619004525; bh=UHGuYHu6CyJAxfObecNaVrVDlxWGck78r232ftYbvX4=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=igzMjFTRCs1+HWEc5EOZv1W0mwi7OYTSbEOvYpWsRoWSUFVCtJ+RVWfUmoHitC8gU IAtj49C7+VzHToYAarbxRGuYtKkxqa6/ya6ris3Z8NKKenG8vhMedBPE8uMBzAXS/x G3SiP9xRN4DNZRTfHeK3Qq0wCJnJN6qo/dKFCt1U= To: Ferruh Yigit , Thomas Monjalon , Gaetan Rivet , Stephen Hemminger , Qi Zhang , Ali Alnubani , Yuanhan Liu , Matan Azrad , Konstantin Ananyev , Zhiyong Yang , Adrien Mazarguil Cc: dev@dpdk.org, "Min Hu (Connor)" , stable@dpdk.org, Kevin Traynor References: <1618645179-11582-1-git-send-email-humin29@huawei.com> <20210421023700.1640488-1-ferruh.yigit@intel.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <7b5f899e-aa34-ac36-ace3-7b2830257ff4@oktetlabs.ru> Date: Wed, 21 Apr 2021 14:28:44 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <20210421023700.1640488-1-ferruh.yigit@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v9] 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/21/21 5:36 AM, Ferruh Yigit wrote: > From: "Min Hu (Connor)" > > 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 > > Signed-off-by: Min Hu (Connor) > Signed-off-by: Ferruh Yigit > Reviewed-by: Andrew Rybchenko > Acked-by: Kevin Traynor Few nits below. Other than that I confirm my "Reviewed-by". The patch is really long. It would be better to split it into few: - relocate dev assignment - empty lines mangling (when it is unrelated to previous item) - ops check before usage (combined with related style checks) - error logs refinement However, since the patch is already reviewed this way, may be it is better to push as is after review notes processing. > @@ -817,7 +859,12 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id) > uint16_t pid; > > if (name == NULL) { > - RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n"); > + RTE_ETHDEV_LOG(ERR, "Cannot get port ID from NULL name"); > + return -EINVAL; > + } > + > + if (port_id == NULL) { > + RTE_ETHDEV_LOG(ERR, "Cannot get port ID to NULL\n"); Since name is already checked above, I think it would be useful to log 'name' here to provide context. > return -EINVAL; > } > [snip] > @@ -3256,6 +3370,20 @@ rte_eth_dev_fw_version_get(uint16_t port_id, char *fw_version, size_t fw_size) > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > dev = &rte_eth_devices[port_id]; > > + if (fw_version == NULL) { > + RTE_ETHDEV_LOG(ERR, > + "Cannot get ethdev port %u FW version to NULL\n", > + port_id); > + return -EINVAL; > + } > + > + if (fw_size == 0) { > + RTE_ETHDEV_LOG(ERR, > + "Cannot get ethdev port %u FW version to buffer with zero size\n", > + port_id); > + return -EINVAL; > + } > + The only error condition is NULL fw_version with positive fw_size. Othwerwise, it could be just a call to get required size of buffer for FW version. > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fw_version_get, -ENOTSUP); > return eth_err(port_id, (*dev->dev_ops->fw_version_get)(dev, > fw_version, fw_size)); [snip] > @@ -3323,6 +3457,21 @@ rte_eth_dev_get_supported_ptypes(uint16_t port_id, uint32_t ptype_mask, > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > dev = &rte_eth_devices[port_id]; > + > + if (ptypes == NULL) { > + RTE_ETHDEV_LOG(ERR, > + "Cannot get ethdev port %u supported packet types to NULL\n", > + port_id); > + return -EINVAL; > + } > + > + if (num == 0) { > + RTE_ETHDEV_LOG(ERR, > + "Cannot get ethdev port %u supported packet types to array with zero size\n", > + port_id); > + return -EINVAL; > + } > + The error condition is "ptypes == NULL && num > 0". Otherwise, if num == 0 (regardless ptypes) it is just a call to get size of required buffer. Same as below. > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_supported_ptypes_get, 0); > all_ptypes = (*dev->dev_ops->dev_supported_ptypes_get)(dev); > [snip]