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

[Slava Ovsiienko <viacheslavo@mellanox.com>]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, April 10, 2020 20:15
> 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 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.
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