From: Yongseok Koh <yskoh@mellanox.com>
To: "Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>
Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>,
"dev@dpdk.org" <dev@dpdk.org>,
Shahaf Shuler <shahafs@mellanox.com>,
dpdk stable <stable@dpdk.org>,
"Xueming(Steven) Li" <xuemingl@mellanox.com>
Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix error number handling
Date: Tue, 5 Jun 2018 21:36:32 +0000 [thread overview]
Message-ID: <84C956B6-28EE-4F82-97AE-5E9C371DD115@mellanox.com> (raw)
In-Reply-To: <20180605065246.mw7xnk24cfwxy4an@laranjeiro-vm.dev.6wind.com>
> On Jun 4, 2018, at 11:52 PM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
>
> On Mon, Jun 04, 2018 at 10:37:31AM -0700, Yongseok Koh wrote:
>> rte_errno should be saved only if error has occurred because rte_errno
>> could have garbage value.
>>
>> Fixes: a6d83b6a9209 ("net/mlx5: standardize on negative errno values")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>> ---
>> drivers/net/mlx5/mlx5_flow.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
>> index 994be05be..eaffe7495 100644
>> --- a/drivers/net/mlx5/mlx5_flow.c
>> +++ b/drivers/net/mlx5/mlx5_flow.c
>> @@ -3561,7 +3561,8 @@ mlx5_fdir_filter_delete(struct rte_eth_dev *dev,
>> /* The flow does not match. */
>> continue;
>> }
>> - ret = rte_errno; /* Save rte_errno before cleanup. */
>> + if (ret)
>> + ret = rte_errno; /* Save rte_errno before cleanup. */
>> if (flow)
>> mlx5_flow_list_destroy(dev, &priv->flows, flow);
>> exit:
>> --
>> 2.11.0
>
> This patch is not enough, the returned value being -rte_errno if no
> error is detected by the function it cannot set rte_errno nor return it.
We may need to refactor this kind of code (saving and restoring rte_errno). I
still don't understand why we should preserve rte_errno like this.
Even if this function returns success, there's no obligation to preserve
rte_errno in the function. Once it is called, the ownership of rte_errno belongs
to this function. I can't find how we define this per-lcore variable but, from
the man page of errno,
The <errno.h> header file defines the integer variable errno, which
is set by system calls and some library functions in the event of an
error to indicate what went wrong. Its value is significant only when
the return value of the call indicated an error (i.e., -1 from most
system calls; -1 or NULL from most library functions);
a function that succeeds is allowed to change errno.
So, I still think an API can change rte_errno even if it succeeds, no need to
preserve it. If needed, the caller has to save it.
Thanks,
Yongseok
next prev parent reply other threads:[~2018-06-05 21:36 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-04 17:37 Yongseok Koh
2018-06-05 6:52 ` Nélio Laranjeiro
2018-06-05 21:36 ` Yongseok Koh [this message]
2018-06-06 6:55 ` Nélio Laranjeiro
2018-06-06 18:39 ` Yongseok Koh
2018-06-07 7:39 ` Nélio Laranjeiro
2018-06-18 17:06 ` Yongseok Koh
2018-06-19 11:48 ` Nélio Laranjeiro
2018-06-19 23:00 ` Yongseok Koh
2018-06-20 7:05 ` Nélio Laranjeiro
2018-06-19 23:13 ` [dpdk-stable] [PATCH v2] " Yongseok Koh
2018-06-20 7:02 ` Nélio Laranjeiro
2018-06-21 10:57 ` Shahaf Shuler
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=84C956B6-28EE-4F82-97AE-5E9C371DD115@mellanox.com \
--to=yskoh@mellanox.com \
--cc=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--cc=nelio.laranjeiro@6wind.com \
--cc=shahafs@mellanox.com \
--cc=stable@dpdk.org \
--cc=xuemingl@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).