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 80552A0547; Wed, 21 Apr 2021 15:40:38 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6311541AC8; Wed, 21 Apr 2021 15:40:38 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mails.dpdk.org (Postfix) with ESMTP id 52DB3410F9; Wed, 21 Apr 2021 15:40:36 +0200 (CEST) IronPort-SDR: 3QDBm3dJGaXcuvPNopWg6TaWzLEpgLsyxNXp7fGGeeVR8AsIviIZaQ5GQanPlMXl3V7u1FOMYy M0Js5bmz+hTA== X-IronPort-AV: E=McAfee;i="6200,9189,9961"; a="182828297" X-IronPort-AV: E=Sophos;i="5.82,240,1613462400"; d="scan'208";a="182828297" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Apr 2021 06:40:34 -0700 IronPort-SDR: 1OuldFKjmUFLi0PtUGDNhytwr2fztCzaaUPzFwSFhtQy0+clQsuQJkrBIuVbavdKE2qZR33/Gp qPhZg+NQX+yw== X-IronPort-AV: E=Sophos;i="5.82,240,1613462400"; d="scan'208";a="455345776" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.206.218]) ([10.213.206.218]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Apr 2021 06:40:31 -0700 From: Ferruh Yigit To: Andrew Rybchenko , 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> X-User: ferruhy Message-ID: Date: Wed, 21 Apr 2021 14:40:27 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed 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/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.