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 8D111A0524; Tue, 13 Apr 2021 10:44:31 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 63698160C95; Tue, 13 Apr 2021 10:44:31 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id B2C84160C8E for ; Tue, 13 Apr 2021 10:44:29 +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 C1A627F523; Tue, 13 Apr 2021 11:44:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru C1A627F523 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1618303468; bh=zeMH5lvHIKRmZ1M7tDu+KE1IuRMG14xsYMgq2POFFWE=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=H4dMrqi88FaLR7GJzGXxOZpkpkuJeyjYWKyjot5mU94f7RkXLkfE0k8DTyFh/1LMz PJUcnHfw4pcceb436wR7WA84uDyGEF5I4TOexkjxNvDbjkF6IflBV74BnLIrgpEynG idSUYbi7aXLtsvnlX3Wd+wGTwM50XvVb0k6NVOec= 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> <1618284134-26152-1-git-send-email-humin29@huawei.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Tue, 13 Apr 2021 11:44:28 +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: <1618284134-26152-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 v2] 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/13/21 6:22 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 > > Signed-off-by: Min Hu (Connor) Many thanks for working on it. Few notes below. [snip] > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 6b5cfd6..e1655b5 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -678,6 +684,9 @@ rte_eth_dev_owner_set(const uint16_t port_id, > { > int ret; > > + if (owner == NULL) > + return -EINVAL; > + Here and in many-many cases below I think the order of checks is important in cases when different error codes are returned. When there is no any very good reasons why arguments should be checked in different order, arguments should be checked in order specified in function prototype. In this cases (and many cases below), port_id should be checked first. In this particular case it means that the pointer check should be done in a static helper function. One more point is error logging in the case of failure. Right now I'd use RTE_ETHDEV_LOG(ERR, ...). May be later we'll find out that some of messages should be made INFO or DEBUG. Something like: RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n", port_id); I'm not 100% sure in format, but my requirements are: - log messages should be unique - log messages should be human readable (i.e. I'd avoid usage of function name) - log messages should provide enough information to understand what went wrong and provide context (basically it correlates with uniqueness requirement) @Thomas, @Ferruh, what do you think? It would be good if we reach an argement before mass changes are done? > @@ -2491,6 +2515,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t queue_id, uint32_t free_cnt) > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP); > > + if (queue_id >= dev->data->nb_tx_queues) { > + RTE_ETHDEV_LOG(ERR, "Queue id should be < %u.", > + dev->data->nb_tx_queues); > + return -EINVAL; > + } > + I'm not 100% sure that it is a control path. > /* Call driver to free pending mbufs. */ > ret = (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id], > free_cnt); [snip]