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 40975A0547; Wed, 21 Apr 2021 15:50:36 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2FD0A41B00; Wed, 21 Apr 2021 15:50:36 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 2B789410F9; Wed, 21 Apr 2021 15:50:35 +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 9F2657F600; Wed, 21 Apr 2021 16:50:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 9F2657F600 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1619013034; bh=v71zkU40eKTtqCPt/WBnnA4E8cf/rIFmuLkdbDgwi24=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=R0j70qJO/Gsu52jILvVlPxtXnlIO6b+FExy84VhAx9zZtIh/kRjsCKYRn5WGQSP9X jqk0jhUjDOSaruWcoNRGM3x2s8wKiyIBNJ00GcST9lg28w+348dGECHwI0EVy9w+JJ Cs5rDUjitXMYJUjaGJXRXELdez73deLRHRFTzhgg= 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> <7b5f899e-aa34-ac36-ace3-7b2830257ff4@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <0adf6e17-e627-e3b0-6bf0-6ffe94cae3dc@oktetlabs.ru> Date: Wed, 21 Apr 2021 16:50:34 +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: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 4:40 PM, Ferruh Yigit wrote: > On 4/21/2021 2:19 PM, Ferruh Yigit wrote: >> On 4/21/2021 12:28 PM, Andrew Rybchenko wrote: >>> 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. >>> >> >> Right, above is wrong. >> >> Agree that "fw_version == NULL && fw_size > 0" is error condition, >> but is it clear if how this API should behave on >> "fw_version == NULL && fw_size == 0"? >> >> Like sfc has following, >> if ((fw_version == NULL) || (fw_size == 0)) >>      return -EINVAL; > > axgbe, qede also returns error when fw_version is NULL, independent from > fw_size. > > But I think taking "fw_version == NULL && fw_size > 0" as only error > condition is reasonable, although some PMDs will be behaving wrong. > I can send a separate patch to unify the behavior. Many thanks, please, let me know if you need help with net/sfc.