From: Stephen Hemminger <stephen@networkplumber.org>
To: Slava Ovsiienko <viacheslavo@mellanox.com>
Cc: Matan Azrad <matan@mellanox.com>,
Shahaf Shuler <shahafs@mellanox.com>,
"dev@dpdk.org" <dev@dpdk.org>,
Alexander Kozyrev <akozyrev@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert
Date: Tue, 31 Mar 2020 07:55:14 -0700 [thread overview]
Message-ID: <20200331075514.775408ee@hermes.lan> (raw)
In-Reply-To: <AM4PR05MB3265DC2FC996A1CCBDDF3471D2C80@AM4PR05MB3265.eurprd05.prod.outlook.com>
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.
next prev parent reply other threads:[~2020-03-31 14:55 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-31 6:02 Stephen Hemminger
2020-03-31 7:31 ` Slava Ovsiienko
2020-03-31 14:55 ` Stephen Hemminger [this message]
2020-03-31 15:09 ` Slava Ovsiienko
2020-04-10 17:14 ` Stephen Hemminger
2020-04-13 9:51 ` Slava Ovsiienko
2020-05-14 7:09 ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
2020-05-14 15:11 ` Alexander Kozyrev
2020-05-14 20:38 ` Slava Ovsiienko
2020-05-17 12:02 ` Matan Azrad
2020-05-17 12:39 ` Raslan Darawsheh
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200331075514.775408ee@hermes.lan \
--to=stephen@networkplumber.org \
--cc=akozyrev@mellanox.com \
--cc=dev@dpdk.org \
--cc=matan@mellanox.com \
--cc=shahafs@mellanox.com \
--cc=viacheslavo@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).