Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 31 Mar 2020 15:09:43 +0000
Slava Ovsiienko <viacheslavo@mellanox.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Tuesday, March 31, 2020 17:55
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> > <shahafs@mellanox.com>; dev@dpdk.org; Alexander Kozyrev
> > <akozyrev@mellanox.com>
> > Subject: Re: [PATCH] common/mlx5: fix bogus assert
> > 
> > On Tue, 31 Mar 2020 07:31:48 +0000
> > Slava Ovsiienko <viacheslavo@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. 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