[dpdk-dev] eal: fix rte_errno values for IPC API
Checks
Commit Message
rte_errno values should not be negative.
Fixes: bacaa2754017 ("eal: add channel for multi-process communication")
Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
Cc: jianfeng.tan@intel.com
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
Notes:
This is in DPDK coding style guide:
http://dpdk.org/doc/guides/contributing/coding_style.html
However, i should note that situation with this is a mess
in DPDK, and in a lot of cases, rte_errno is set to -errno.
lib/librte_eal/common/eal_common_proc.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
Comments
> -----Original Message-----
> From: Burakov, Anatoly
> Sent: Saturday, February 10, 2018 9:15 PM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng
> Subject: [PATCH] eal: fix rte_errno values for IPC API
>
> rte_errno values should not be negative.
>
> Fixes: bacaa2754017 ("eal: add channel for multi-process communication")
> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> Cc: jianfeng.tan@intel.com
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
Thanks for fixing this.
> > rte_errno values should not be negative.
> >
> > Fixes: bacaa2754017 ("eal: add channel for multi-process communication")
> > Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> > Cc: jianfeng.tan@intel.com
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
>
> Thanks for fixing this.
Applied, thanks
There are a lot of similar issues:
git grep -l 'rte_errno = -E' | sed 's,[^/]*$,,' | sort -u
drivers/event/opdl/
drivers/event/sw/
drivers/net/avf/
drivers/net/e1000/
drivers/net/ena/
drivers/net/enic/
drivers/net/fm10k/
drivers/net/i40e/
drivers/net/ixgbe/
drivers/net/qede/
drivers/net/vmxnet3/
examples/ipsec-secgw/
lib/librte_ether/
lib/librte_eventdev/
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, February 13, 2018 1:51 PM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_errno values for IPC API
>
> > > rte_errno values should not be negative.
> > >
> > > Fixes: bacaa2754017 ("eal: add channel for multi-process communication")
> > > Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> > > Cc: jianfeng.tan@intel.com
> > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >
> > Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
> >
> > Thanks for fixing this.
>
> Applied, thanks
>
> There are a lot of similar issues:
>
> git grep -l 'rte_errno = -E' | sed 's,[^/]*$,,' | sort -u
>
> drivers/event/opdl/
> drivers/event/sw/
<snip>
> lib/librte_eventdev/
I just checked the eventdev.h port_link() docs, which indicate negative return values.
Perhaps the header is wrong too - but the PMDs adhere to the library header in this case.
Is there a requirement for rte_errno to be positive?
It looks to be declared as per-lcore signed int in rte_errno.h +20
Either-way, if we want to change the PMDs, we should change the Eventdev APIs,
which means API breakage, and application changes to handle changed return values.
Sound like more work than it is worth it to me?
On 13-Feb-18 2:16 PM, Van Haaren, Harry wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
>> Sent: Tuesday, February 13, 2018 1:51 PM
>> To: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_errno values for IPC API
>>
>>>> rte_errno values should not be negative.
>>>>
>>>> Fixes: bacaa2754017 ("eal: add channel for multi-process communication")
>>>> Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
>>>> Cc: jianfeng.tan@intel.com
>>>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>>
>>> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
>>>
>>> Thanks for fixing this.
>>
>> Applied, thanks
>>
>> There are a lot of similar issues:
>>
>> git grep -l 'rte_errno = -E' | sed 's,[^/]*$,,' | sort -u
>>
>> drivers/event/opdl/
>> drivers/event/sw/
> <snip>
>> lib/librte_eventdev/
>
>
> I just checked the eventdev.h port_link() docs, which indicate negative return values.
> Perhaps the header is wrong too - but the PMDs adhere to the library header in this case.
>
> Is there a requirement for rte_errno to be positive?
> It looks to be declared as per-lcore signed int in rte_errno.h +20
>
> Either-way, if we want to change the PMDs, we should change the Eventdev APIs,
> which means API breakage, and application changes to handle changed return values.
>
> Sound like more work than it is worth it to me?
>
To be clear, documentation doesn't *explicitly* spell this out, so it
can be interpreted as not having an opinion on this. So i think the
first step should be fixing documentation to clearly indicate this is to
be expected at least of new code (unless decided against for consistency
reasons).
However, while docs don't state this outright, i think they imply it
both in cases of referring to errno values and returning "-errno" values
(which implies sign isn't part of the errno value). Plus, errno values
in Linux are always positive, plus rte_strerror() expects a positive
value, and (as correctly pointed out by Harry) our custom errno values -
so at least our API expects it to be positive, even if it doesn't state
it outright.
On Tue, Feb 13, 2018 at 02:16:08PM +0000, Van Haaren, Harry wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Tuesday, February 13, 2018 1:51 PM
> > To: Burakov, Anatoly <anatoly.burakov@intel.com>
> > Cc: dev@dpdk.org; Tan, Jianfeng <jianfeng.tan@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] eal: fix rte_errno values for IPC API
> >
> > > > rte_errno values should not be negative.
> > > >
> > > > Fixes: bacaa2754017 ("eal: add channel for multi-process communication")
> > > > Fixes: 783b6e54971d ("eal: add synchronous multi-process communication")
> > > > Cc: jianfeng.tan@intel.com
> > > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > >
> > > Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > >
> > > Thanks for fixing this.
> >
> > Applied, thanks
> >
> > There are a lot of similar issues:
> >
> > git grep -l 'rte_errno = -E' | sed 's,[^/]*$,,' | sort -u
> >
> > drivers/event/opdl/
> > drivers/event/sw/
> <snip>
> > lib/librte_eventdev/
>
>
> I just checked the eventdev.h port_link() docs, which indicate negative return values.
> Perhaps the header is wrong too - but the PMDs adhere to the library header in this case.
>
> Is there a requirement for rte_errno to be positive?
> It looks to be declared as per-lcore signed int in rte_errno.h +20
>
I think I wrote that part of the documentation, and it never crossed my
mind that people would set rte_errno to negative values, given how
errno from system calls are always positive. However, I think this
omission should be rectified, and we should enforce having rte_errno
values as positive.
> Either-way, if we want to change the PMDs, we should change the Eventdev APIs,
> which means API breakage, and application changes to handle changed return values.
>
> Sound like more work than it is worth it to me?
I would view it as restoring sanity (or balance to the force if you
prefer! :-) ), so I'd definitely be ok with an ABI break to do that.
/Bruce
@@ -131,16 +131,16 @@ validate_action_name(const char *name)
{
if (name == NULL) {
RTE_LOG(ERR, EAL, "Action name cannot be NULL\n");
- rte_errno = -EINVAL;
+ rte_errno = EINVAL;
return -1;
}
if (strnlen(name, RTE_MP_MAX_NAME_LEN) == 0) {
RTE_LOG(ERR, EAL, "Length of action name is zero\n");
- rte_errno = -EINVAL;
+ rte_errno = EINVAL;
return -1;
}
if (strnlen(name, RTE_MP_MAX_NAME_LEN) == RTE_MP_MAX_NAME_LEN) {
- rte_errno = -E2BIG;
+ rte_errno = E2BIG;
return -1;
}
return 0;
@@ -156,7 +156,7 @@ rte_mp_action_register(const char *name, rte_mp_t action)
entry = malloc(sizeof(struct action_entry));
if (entry == NULL) {
- rte_errno = -ENOMEM;
+ rte_errno = ENOMEM;
return -1;
}
strcpy(entry->action_name, name);
@@ -165,7 +165,7 @@ rte_mp_action_register(const char *name, rte_mp_t action)
pthread_mutex_lock(&mp_mutex_action);
if (find_action_entry_by_name(name) != NULL) {
pthread_mutex_unlock(&mp_mutex_action);
- rte_errno = -EEXIST;
+ rte_errno = EEXIST;
free(entry);
return -1;
}
@@ -505,7 +505,7 @@ check_input(const struct rte_mp_msg *msg)
{
if (msg == NULL) {
RTE_LOG(ERR, EAL, "Msg cannot be NULL\n");
- rte_errno = -EINVAL;
+ rte_errno = EINVAL;
return false;
}
@@ -514,14 +514,14 @@ check_input(const struct rte_mp_msg *msg)
if (msg->len_param > RTE_MP_MAX_PARAM_LEN) {
RTE_LOG(ERR, EAL, "Message data is too long\n");
- rte_errno = -E2BIG;
+ rte_errno = E2BIG;
return false;
}
if (msg->num_fds > RTE_MP_MAX_FD_NUM) {
RTE_LOG(ERR, EAL, "Cannot send more than %d FDs\n",
RTE_MP_MAX_FD_NUM);
- rte_errno = -E2BIG;
+ rte_errno = E2BIG;
return false;
}
@@ -560,7 +560,7 @@ mp_request_one(const char *dst, struct rte_mp_msg *req,
pthread_mutex_unlock(&sync_requests.lock);
if (exist) {
RTE_LOG(ERR, EAL, "A pending request %s:%s\n", dst, req->name);
- rte_errno = -EEXIST;
+ rte_errno = EEXIST;
return -1;
}
@@ -596,7 +596,7 @@ mp_request_one(const char *dst, struct rte_mp_msg *req,
if (sync_req.reply_received == 0) {
RTE_LOG(ERR, EAL, "Fail to recv reply for request %s:%s\n",
dst, req->name);
- rte_errno = -ETIMEDOUT;
+ rte_errno = ETIMEDOUT;
return -1;
}
@@ -604,7 +604,7 @@ mp_request_one(const char *dst, struct rte_mp_msg *req,
if (!tmp) {
RTE_LOG(ERR, EAL, "Fail to alloc reply for request %s:%s\n",
dst, req->name);
- rte_errno = -ENOMEM;
+ rte_errno = ENOMEM;
return -1;
}
memcpy(&tmp[reply->nb_received], &msg, sizeof(msg));
@@ -676,7 +676,7 @@ rte_mp_reply(struct rte_mp_msg *msg, const char *peer)
if (peer == NULL) {
RTE_LOG(ERR, EAL, "peer is not specified\n");
- rte_errno = -EINVAL;
+ rte_errno = EINVAL;
return -1;
}