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

[Slava Ovsiienko <viacheslavo@mellanox.com>]

> -----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.
Removing assert (instead of fixing one) reduces our debugging capabilities, so it is not OK, as for me.

With best regards, Slava