From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7475EA0598; Fri, 10 Apr 2020 19:14:57 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EEA141D5FF; Fri, 10 Apr 2020 19:14:56 +0200 (CEST) Received: from mail-pj1-f65.google.com (mail-pj1-f65.google.com [209.85.216.65]) by dpdk.org (Postfix) with ESMTP id 178841D5FB for ; Fri, 10 Apr 2020 19:14:54 +0200 (CEST) Received: by mail-pj1-f65.google.com with SMTP id nu11so973373pjb.1 for ; Fri, 10 Apr 2020 10:14:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=hIi8H7kmbLqAGfghQ6/pqROg2uQkqZiQ0DD8EpYbQWE=; b=I0d1YKgw10/cTV3Uak8wUTS4smC7hyCTjhHpyYtYtElM2cSiL/KrBXKzBcTLfIm/Ma x5GZo7kIJZ7kkbHWAk5ykbG78w3LLw09U62/oRD+xUAfBTtmVCVylhnWN41eQ0Rc2jhM GlJbAm5EOTJev8KQLiDvvCZKVumsnnALCIirE9qnoK+Cph5zGZlALSqymnLPRcabjUN6 FUcJxnqr5SkL1CLbMRI6L5baXQd+vGDJqO0bSCHH5+N/ExBcAjX8C4oy0HgnRoBgQfeL hE9ELNiQ5PtLChW4ozvWSpYW0sTCjvgnz0sYb2xvQMHxRSxt4NnBaVHfVSiujhLbYbf2 s59Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=hIi8H7kmbLqAGfghQ6/pqROg2uQkqZiQ0DD8EpYbQWE=; b=gvo6jnjzDbd2w9bG0mAHVzp6squUZrPFNNwJeokhYNw8m8yG4UFHnBFEIvZZ8I6F4H 2enQOXCeqoQE1CCrlydeTapeMOmgVB/reSoW+6mmLznSrpOu5Rc7QteGABphXzTOGRll HpVcXlGBeU8ira3ZYDHLwkxYnfLVXURYJNlNi4qcz2iMR4NHv/DCnnpwFJlXuxfa/myF axMn1XKb2rOfgyYDsxTMpJt6Bn74E+LwDQKfyt4fknIhdgNLG118Db3oaC0Qz/qACWSC HjkyisUc92TApCODTQm3gDqOpwjXah1Ec6SeN/mF+BMlEralDkp6FBxsMDd3IEJ6MznN xkHA== X-Gm-Message-State: AGi0PuYV6i2rOZxfmK9R/HAfO2uXhekK6eJvhM9pmiiULk72FEEy6exS UdBndq9Iyn1Y/Q+QzZIx9widfQ== X-Google-Smtp-Source: APiQypJZW2QVI2+cAzxt3B7bvDj1ho3rWv9/LW690oYXt/2OTlmqxnRz8aWKkXorwHLoHr0FMbQj/A== X-Received: by 2002:a17:90b:3789:: with SMTP id mz9mr6600696pjb.38.1586538894152; Fri, 10 Apr 2020 10:14:54 -0700 (PDT) Received: from hermes.lan (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id a1sm2192352pfi.182.2020.04.10.10.14.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Apr 2020 10:14:53 -0700 (PDT) Date: Fri, 10 Apr 2020 10:14:45 -0700 From: Stephen Hemminger To: Slava Ovsiienko Cc: Matan Azrad , Shahaf Shuler , "dev@dpdk.org" , Alexander Kozyrev Message-ID: <20200410101445.34f2ab0f@hermes.lan> In-Reply-To: References: <20200331060247.10954-1-stephen@networkplumber.org> <20200331075514.775408ee@hermes.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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 Tue, 31 Mar 2020 15:09:43 +0000 Slava Ovsiienko wrote: > > -----Original Message----- > > From: Stephen Hemminger > > Sent: Tuesday, March 31, 2020 17:55 > > To: Slava Ovsiienko > > Cc: Matan Azrad ; Shahaf Shuler > > ; dev@dpdk.org; Alexander Kozyrev > > > > Subject: Re: [PATCH] common/mlx5: fix bogus assert > >=20 > > On Tue, 31 Mar 2020 07:31:48 +0000 > > Slava Ovsiienko wrote: > > =20 > > > Hi, Stephen > > > > > > Thank you for the fix. > > > > > > The exposed API to set MAC addresses: > > > - mlx5_mac_addr_set (invoked by rte_mac_addr_set ()) > > > - mlx5_set_mc_addr_list (invoked by rte_eth_dev_set_mc_addr_list()) > > > > > > Both routines call mlx5_internal_mac_addr_add(), it in its turn calls > > > mlx5_nl_mac_addr_add() (that is subject of the patch). > > > > > > mlx5_nl_mac_addr_add is internal function, not exposed external API, > > > the wrong parameter means the critical internal bug, so assert looks = to be =20 > > relevant here. =20 > > > I would not remove MLX5_ASSERT at all but fix just it. > > > Adding the parameter check and return an error is nice. > > > What do you think? > > > > > > With best regards, Slava =20 > >=20 > > The real root cause is that sizeof(mac_own) is the wrong thing to do. T= he > > error handling is up to you. > >=20 > > Since ASSERT's are compiled out they are never tested and are actually > > making code less safe. =20 >=20 > Generally speaking assert is not subject to test - I would consider it as= a part of debug means. > Yes, this assert was with wrong condition and was not tested, but once en= abled and a lot of MACs > came into game - we got an issue and your patch is here =F0=9F=98=8A.=20 >=20 > >> making code less safe. =20 > The debug version of code is usually less safe and has no performance. > Adding the check and error return is OK, it works always and improves th= e code, we do not expect engaging of it here, though. > I am done being diplomatic. You have repeatedly ignored the fact that doing sizeof a pointer is not correct here. mac_own is a pointer so doing sizeof(mac_own) will not give w= hat you want. You probably thought mac_own was an array, or that compiler would know that the pointer was an array. Any visible config option should work correctly. The code should not break. Any not visible config option #ifdefs should be expunged from the upstream code. Either take the patch, or fix your code please