From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: "Min Hu (Connor)" <humin29@huawei.com>, dev@dpdk.org
Cc: ferruh.yigit@intel.com, thomas@monjalon.net
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: add sanity checks in control APIs
Date: Tue, 13 Apr 2021 11:44:28 +0300 [thread overview]
Message-ID: <dd9e5af1-c7d2-da58-f1b5-5e1e76c39b95@oktetlabs.ru> (raw)
In-Reply-To: <1618284134-26152-1-git-send-email-humin29@huawei.com>
On 4/13/21 6:22 AM, Min Hu (Connor) wrote:
> This patch adds more sanity checks in control path APIs.
>
> Fixes: 214ed1acd125 ("ethdev: add iterator to match devargs input")
> Fixes: 3d98f921fbe9 ("ethdev: unify prefix for static functions and variables")
> Fixes: 0366137722a0 ("ethdev: check for invalid device name")
> Fixes: d948f596fee2 ("ethdev: fix port data mismatched in multiple process model")
> Fixes: 5b7ba31148a8 ("ethdev: add port ownership")
> Fixes: f8244c6399d9 ("ethdev: increase port id range")
> Cc: stable@dpdk.org
>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
Many thanks for working on it. Few notes below.
[snip]
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 6b5cfd6..e1655b5 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -678,6 +684,9 @@ rte_eth_dev_owner_set(const uint16_t port_id,
> {
> int ret;
>
> + if (owner == NULL)
> + return -EINVAL;
> +
Here and in many-many cases below I think the order of checks
is important in cases when different error codes are returned.
When there is no any very good reasons why arguments should
be checked in different order, arguments should be checked in
order specified in function prototype. In this cases (and many
cases below), port_id should be checked first.
In this particular case it means that the pointer check
should be done in a static helper function.
One more point is error logging in the case of failure.
Right now I'd use RTE_ETHDEV_LOG(ERR, ...). May be later we'll
find out that some of messages should be made INFO or DEBUG.
Something like:
RTE_ETHDEV_LOG(ERR, "Failed to set ethdev port %u owner to NULL\n",
port_id);
I'm not 100% sure in format, but my requirements are:
- log messages should be unique
- log messages should be human readable (i.e. I'd avoid
usage of function name)
- log messages should provide enough information to understand
what went wrong and provide context (basically it correlates
with uniqueness requirement)
@Thomas, @Ferruh, what do you think? It would be good if we
reach an argement before mass changes are done?
> @@ -2491,6 +2515,12 @@ rte_eth_tx_done_cleanup(uint16_t port_id, uint16_t queue_id, uint32_t free_cnt)
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_done_cleanup, -ENOTSUP);
>
> + if (queue_id >= dev->data->nb_tx_queues) {
> + RTE_ETHDEV_LOG(ERR, "Queue id should be < %u.",
> + dev->data->nb_tx_queues);
> + return -EINVAL;
> + }
> +
I'm not 100% sure that it is a control path.
> /* Call driver to free pending mbufs. */
> ret = (*dev->dev_ops->tx_done_cleanup)(dev->data->tx_queues[queue_id],
> free_cnt);
[snip]
next prev parent reply other threads:[~2021-04-13 8:44 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-10 9:18 [dpdk-dev] [PATCH] " Min Hu (Connor)
2021-04-12 23:08 ` Ferruh Yigit
2021-04-13 3:23 ` Min Hu (Connor)
2021-04-13 3:22 ` [dpdk-dev] [PATCH v2] " Min Hu (Connor)
2021-04-13 8:44 ` Andrew Rybchenko [this message]
2021-04-13 8:58 ` Thomas Monjalon
2021-04-13 9:24 ` Ferruh Yigit
2021-04-14 11:12 ` Min Hu (Connor)
2021-04-29 17:48 ` Tyler Retzlaff
2021-04-29 18:18 ` Stephen Hemminger
2021-04-14 11:11 ` [dpdk-dev] [PATCH v3] " Min Hu (Connor)
2021-04-14 12:00 ` Andrew Rybchenko
2021-04-15 0:52 ` Min Hu (Connor)
2021-04-15 0:52 ` [dpdk-dev] [PATCH v4] " Min Hu (Connor)
2021-04-15 8:15 ` Andrew Rybchenko
2021-04-15 11:09 ` Min Hu (Connor)
2021-04-15 11:57 ` Thomas Monjalon
2021-04-15 12:03 ` Andrew Rybchenko
2021-04-15 12:20 ` Thomas Monjalon
2021-04-15 12:43 ` Andrew Rybchenko
2021-04-15 12:04 ` Kevin Traynor
2021-04-15 12:15 ` Thomas Monjalon
2021-04-16 7:01 ` Min Hu (Connor)
2021-04-16 7:00 ` Min Hu (Connor)
2021-04-16 10:09 ` Kevin Traynor
2021-04-16 10:44 ` Min Hu (Connor)
2021-04-15 11:09 ` [dpdk-dev] [PATCH v5] " Min Hu (Connor)
2021-04-15 15:38 ` Ferruh Yigit
2021-04-16 7:02 ` Min Hu (Connor)
2021-04-16 16:19 ` Stephen Hemminger
2021-04-15 15:45 ` Ferruh Yigit
2021-04-15 16:21 ` Thomas Monjalon
2021-04-16 7:04 ` Min Hu (Connor)
2021-04-16 16:25 ` Stephen Hemminger
2021-04-16 6:52 ` [dpdk-dev] [PATCH v6] " Min Hu (Connor)
2021-04-16 10:22 ` Kevin Traynor
2021-04-16 11:00 ` Min Hu (Connor)
2021-04-16 16:28 ` Stephen Hemminger
2021-04-17 0:28 ` Min Hu (Connor)
2021-04-17 21:37 ` Thomas Monjalon
2021-04-19 0:34 ` Min Hu (Connor)
2021-04-17 7:42 ` Min Hu (Connor)
2021-04-16 11:00 ` [dpdk-dev] [PATCH v7] " Min Hu (Connor)
2021-04-16 11:31 ` Ferruh Yigit
2021-04-16 12:02 ` Thomas Monjalon
2021-04-17 7:39 ` Min Hu (Connor)
2021-04-17 7:39 ` [dpdk-dev] [PATCH v8] " Min Hu (Connor)
2021-04-20 10:04 ` Thomas Monjalon
2021-04-20 13:59 ` Ferruh Yigit
2021-04-20 14:20 ` Kevin Traynor
2021-04-20 14:33 ` Thomas Monjalon
2021-04-21 2:36 ` [dpdk-dev] [PATCH v9] " Ferruh Yigit
2021-04-21 10:48 ` Thomas Monjalon
2021-04-21 11:28 ` Andrew Rybchenko
2021-04-21 12:36 ` Min Hu (Connor)
2021-04-21 12:38 ` Kevin Traynor
2021-04-21 13:19 ` Ferruh Yigit
2021-04-21 13:40 ` Ferruh Yigit
2021-04-21 13:50 ` Andrew Rybchenko
2021-04-21 13:50 ` Andrew Rybchenko
2021-04-21 14:17 ` Ferruh Yigit
2021-04-21 12:36 ` [dpdk-dev] [PATCH v10] " Min Hu (Connor)
2021-04-21 14:19 ` Ferruh Yigit
2021-04-21 16:22 ` Ferruh Yigit
2021-04-21 17:16 ` Ferruh Yigit
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=dd9e5af1-c7d2-da58-f1b5-5e1e76c39b95@oktetlabs.ru \
--to=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=humin29@huawei.com \
--cc=thomas@monjalon.net \
/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).