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 A38BDA0547; Wed, 21 Apr 2021 15:50:05 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 859A441AC8; Wed, 21 Apr 2021 15:50:05 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 3BAD3410F9; Wed, 21 Apr 2021 15:50:04 +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 56B867F5AC; Wed, 21 Apr 2021 16:50:03 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 56B867F5AC DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1619013003; bh=VsySrALHD9vqLVF4dtZuy0ytij2QZdVLwKaxn+QN2aM=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=vDYWvnK1aQ/7ntrqJLjG38cjHdDHSmBZ56soH9K3KjZ3W7hxGzL4uzxp6kig40x61 kZDiHbk9pZk+9B9W5Ct9f9igYzbkuiQkrPEG+bzaofMuXL2BxvDabdWi3PnfnrKBcb JXohbJZfpEDAe7+QqSModEzHczjmRVSj+dpPjvvA= 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: Date: Wed, 21 Apr 2021 16:50:03 +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-Transfer-Encoding: 8bit Content-Language: en-US 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: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; It looks like net/sfc is buggy here. My review notes are based on rte_eth_dev_fw_version_get() return values description.