> -----Original Message----- > From: Stephen Hemminger <step...@networkplumber.org> > Sent: Friday, April 10, 2020 20:15 > To: Slava Ovsiienko <viachesl...@mellanox.com> > Cc: Matan Azrad <ma...@mellanox.com>; Shahaf Shuler > <shah...@mellanox.com>; dev@dpdk.org; Alexander Kozyrev > <akozy...@mellanox.com> > Subject: Re: [PATCH] common/mlx5: fix bogus assert > > On Tue, 31 Mar 2020 15:09:43 +0000 > Slava Ovsiienko <viachesl...@mellanox.com> wrote: > > > > -----Original Message----- > > > From: Stephen Hemminger <step...@networkplumber.org> > > > Sent: Tuesday, March 31, 2020 17:55 > > > To: Slava Ovsiienko <viachesl...@mellanox.com> > > > Cc: Matan Azrad <ma...@mellanox.com>; Shahaf Shuler > > > <shah...@mellanox.com>; dev@dpdk.org; Alexander Kozyrev > > > <akozy...@mellanox.com> > > > Subject: Re: [PATCH] common/mlx5: fix bogus assert > > > > > > On Tue, 31 Mar 2020 07:31:48 +0000 > > > Slava Ovsiienko <viachesl...@mellanox.com> wrote: > > > > > > > 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 > > > relevant here. > > > > 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 > > > > > > The real root cause is that sizeof(mac_own) is the wrong thing to > > > do. The error handling is up to you. > > > > > > Since ASSERT's are compiled out they are never tested and are > > > actually making code less safe. > > > > 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 > > enabled and a lot of MACs came into game - we got an issue and your patch > is here 😊. > > > > >> making code less safe. > > 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 the > 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. You are quite right. It is obvious bug and must be fixed, thank you for the patch. And let me make you sure I did not mind fixing in anyway. My only proposal was to fix ASSERT as well instead of dropping one, sorry if I did not express it in clear way. Something like this: MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);
> mac_own is a pointer so doing sizeof(mac_own) will not give what 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 Whatever you'd prefer - please, fix ASSERT, or let me know if I should. Thanks in advance, Slava
