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 8AA91A0C4A; Tue, 13 Jul 2021 11:26:44 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CA5E7411F3; Tue, 13 Jul 2021 11:26:42 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 690D84069E for ; Tue, 13 Jul 2021 11:26:41 +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 0B2117F523; Tue, 13 Jul 2021 12:26:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 0B2117F523 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1626168401; bh=l4hun8N/SwScKYzqx0MJZ7E1uaH9MF8odV0wroT9wIU=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=c/WJh2q/wQYDfzFWuJArP/0SmDJOKsUlbTVLBd8VoOt9zPrrygUSUInhpVB+nWpW8 jwvlp3hba/bRVtIBrk7enNP+jOUhH3LZJoqtfZ2z4DG75IBOJKP44eeMNJdsUAHB4+ P2lom0rk8vLs8+utHubWItZPabjxJnWbHznCuK/o= To: "Min Hu (Connor)" , =?UTF-8?Q?Havl=c3=adk_Martin?= Cc: dev@dpdk.org, viktorin@cesnet.cz, chas3@att.com, declan.doherty@intel.com, tomaszx.kulasek@intel.com References: <20210622092531.73112-1-xhavli56@stud.fit.vutbr.cz> <20210622092531.73112-3-xhavli56@stud.fit.vutbr.cz> <3b731983-2bc2-746e-4303-6654b2de1193@huawei.com> <6972a4cc793604dcb6c2e7ce4e0d1586@stud.fit.vutbr.cz> <4648adf2-e63d-39da-57b4-7e4aaab4473f@huawei.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <27a0e379-2021-c4d6-1e35-ce03310f8d3c@oktetlabs.ru> Date: Tue, 13 Jul 2021 12:26:40 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <4648adf2-e63d-39da-57b4-7e4aaab4473f@huawei.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 2/3] net/bonding: fix not checked return value 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 7/9/21 3:09 AM, Min Hu (Connor) wrote: > > > 在 2021/7/8 21:20, Havlík Martin 写道: >> Dne 2021-07-08 14:43, Min Hu (Connor) napsal: >>> 在 2021/6/22 17:25, Martin Havlik 写道: >>>> Return value from bond_ethdev_8023ad_flow_set() is now checked >>>> and appropriate message is logged on error. >>>> >>>> Fixes: 112891cd27e5 ("net/bonding: add dedicated HW queues for LACP >>>> control") >>>> Cc: tomaszx.kulasek@intel.com >>>> >>>> Signed-off-by: Martin Havlik >>>> Cc: Jan Viktorin >>>> --- >>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 8 +++++++- >>>>   1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c >>>> b/drivers/net/bonding/rte_eth_bond_pmd.c >>>> index 4c43bf916..a6755661c 100644 >>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c >>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c >>>> @@ -1819,8 +1819,14 @@ slave_configure(struct rte_eth_dev >>>> *bonded_eth_dev, >>>>                     >>>> internals->mode4.dedicated_queues.flow[slave_eth_dev->data->port_id], >>>>                       &flow_error); >>>>   -        bond_ethdev_8023ad_flow_set(bonded_eth_dev, >>>> +        errval = bond_ethdev_8023ad_flow_set(bonded_eth_dev, >>>>                   slave_eth_dev->data->port_id); >>>> +        if (errval != 0) { >>>> +            RTE_BOND_LOG(ERR, >>>> +                "bond_ethdev_8023ad_flow_set: port=%d, err (%d)", >>>> +                slave_eth_dev->data->port_id, errval); >>>> +            return errval; >>>> +        } >>>>       } >>>> >>> Firstly, I think your patch is OK, >>> But I want to know what is your standard to decide which function >>> should check return value or not(of course, they are none-void)? >>> >> I encountered the problem with dedicated queues on mlx5, I looked into >> the source code and I saw the cause, so I fixed it. If I had seen any >> other issue, I would've fixed it too. That, for example, applies to >> the log message fix I included. My standard is to check all non-void >> return values. > Got it. >>> While, I noticed "bond_ethdev_lsc_event_callback" nearby was not >>> checked, but you ignored it. >>> >> Not ignored, just didn't properly review more code than what closely >> surrounded the problematic lines. >>>>       /* Start device */ >>>> > Acked by: Min Hu (Connor) >> . Acked-by: Min Hu (Connor) Reviewed-by: Andrew Rybchenko Applied, thanks.