* [PATCH] HID: hiddev: fix returned errno code in hiddev_connect()
@ 2015-09-30 9:52 Luis de Bethencourt
2015-09-30 9:56 ` Luis de Bethencourt
2015-09-30 19:40 ` Jiri Kosina
0 siblings, 2 replies; 8+ messages in thread
From: Luis de Bethencourt @ 2015-09-30 9:52 UTC (permalink / raw)
To: linux-kernel; +Cc: jikos, linux-usb, linux-input, Luis de Bethencourt
The driver is using -1 instead of the -ENOMEM defined macro to specify
that a buffer allocation failed. Since the error number is propagated,
the caller will get a -EPERM which is the wrong error condition.
Also, the smatch tool complains with the following warning:
hiddev_connect() warn: returning -1 instead of -ENOMEM is sloppy
Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
---
drivers/hid/usbhid/hiddev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 2f1ddca..c5290ff 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -894,7 +894,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
}
if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
- return -1;
+ return -ENOMEM;
init_waitqueue_head(&hiddev->wait);
INIT_LIST_HEAD(&hiddev->list);
--
2.5.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: hiddev: fix returned errno code in hiddev_connect()
2015-09-30 9:52 [PATCH] HID: hiddev: fix returned errno code in hiddev_connect() Luis de Bethencourt
@ 2015-09-30 9:56 ` Luis de Bethencourt
2015-09-30 10:04 ` Sudip Mukherjee
2015-09-30 19:40 ` Jiri Kosina
1 sibling, 1 reply; 8+ messages in thread
From: Luis de Bethencourt @ 2015-09-30 9:56 UTC (permalink / raw)
To: linux-kernel; +Cc: jikos, linux-usb, linux-input
On 30/09/15 10:52, Luis de Bethencourt wrote:
> The driver is using -1 instead of the -ENOMEM defined macro to specify
> that a buffer allocation failed. Since the error number is propagated,
> the caller will get a -EPERM which is the wrong error condition.
>
> Also, the smatch tool complains with the following warning:
> hiddev_connect() warn: returning -1 instead of -ENOMEM is sloppy
>
> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
> ---
> drivers/hid/usbhid/hiddev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 2f1ddca..c5290ff 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -894,7 +894,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
> }
>
> if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
> - return -1;
> + return -ENOMEM;
>
> init_waitqueue_head(&hiddev->wait);
> INIT_LIST_HEAD(&hiddev->list);
>
Hello,
I got an "Undelivered Mail Returned to Sender" from Jiri Kosina's
jikos@kernel.com email address. This email is listed multiple times
in the MAINTAINERS file, does he have a new address to update this
file?
Thanks,
Luis
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: hiddev: fix returned errno code in hiddev_connect()
2015-09-30 9:56 ` Luis de Bethencourt
@ 2015-09-30 10:04 ` Sudip Mukherjee
2015-09-30 10:05 ` Luis de Bethencourt
0 siblings, 1 reply; 8+ messages in thread
From: Sudip Mukherjee @ 2015-09-30 10:04 UTC (permalink / raw)
To: Luis de Bethencourt; +Cc: linux-kernel, jikos, linux-usb, linux-input
On Wed, Sep 30, 2015 at 10:56:26AM +0100, Luis de Bethencourt wrote:
> On 30/09/15 10:52, Luis de Bethencourt wrote:
> > The driver is using -1 instead of the -ENOMEM defined macro to specify
> > that a buffer allocation failed. Since the error number is propagated,
> > the caller will get a -EPERM which is the wrong error condition.
> >
> > Also, the smatch tool complains with the following warning:
> > hiddev_connect() warn: returning -1 instead of -ENOMEM is sloppy
> >
> > Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
> > ---
> > drivers/hid/usbhid/hiddev.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> > index 2f1ddca..c5290ff 100644
> > --- a/drivers/hid/usbhid/hiddev.c
> > +++ b/drivers/hid/usbhid/hiddev.c
> > @@ -894,7 +894,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
> > }
> >
> > if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
> > - return -1;
> > + return -ENOMEM;
> >
> > init_waitqueue_head(&hiddev->wait);
> > INIT_LIST_HEAD(&hiddev->list);
> >
>
> Hello,
>
> I got an "Undelivered Mail Returned to Sender" from Jiri Kosina's
> jikos@kernel.com email address. This email is listed multiple times
> in the MAINTAINERS file, does he have a new address to update this
> file?
Its jikos@kernel.org
regards
sudip
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: hiddev: fix returned errno code in hiddev_connect()
2015-09-30 10:04 ` Sudip Mukherjee
@ 2015-09-30 10:05 ` Luis de Bethencourt
0 siblings, 0 replies; 8+ messages in thread
From: Luis de Bethencourt @ 2015-09-30 10:05 UTC (permalink / raw)
To: Sudip Mukherjee; +Cc: linux-kernel, jikos, linux-usb, linux-input
On 30/09/15 11:04, Sudip Mukherjee wrote:
> On Wed, Sep 30, 2015 at 10:56:26AM +0100, Luis de Bethencourt wrote:
>> On 30/09/15 10:52, Luis de Bethencourt wrote:
>>> The driver is using -1 instead of the -ENOMEM defined macro to specify
>>> that a buffer allocation failed. Since the error number is propagated,
>>> the caller will get a -EPERM which is the wrong error condition.
>>>
>>> Also, the smatch tool complains with the following warning:
>>> hiddev_connect() warn: returning -1 instead of -ENOMEM is sloppy
>>>
>>> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
>>> ---
>>> drivers/hid/usbhid/hiddev.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
>>> index 2f1ddca..c5290ff 100644
>>> --- a/drivers/hid/usbhid/hiddev.c
>>> +++ b/drivers/hid/usbhid/hiddev.c
>>> @@ -894,7 +894,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
>>> }
>>>
>>> if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
>>> - return -1;
>>> + return -ENOMEM;
>>>
>>> init_waitqueue_head(&hiddev->wait);
>>> INIT_LIST_HEAD(&hiddev->list);
>>>
>>
>> Hello,
>>
>> I got an "Undelivered Mail Returned to Sender" from Jiri Kosina's
>> jikos@kernel.com email address. This email is listed multiple times
>> in the MAINTAINERS file, does he have a new address to update this
>> file?
> Its jikos@kernel.org
>
> regards
> sudip
>
Hi Sudip,
*facepalm* My mistake, yes.
Sorry,
Luis
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: hiddev: fix returned errno code in hiddev_connect()
2015-09-30 9:52 [PATCH] HID: hiddev: fix returned errno code in hiddev_connect() Luis de Bethencourt
2015-09-30 9:56 ` Luis de Bethencourt
@ 2015-09-30 19:40 ` Jiri Kosina
2015-10-03 21:34 ` Luis de Bethencourt
1 sibling, 1 reply; 8+ messages in thread
From: Jiri Kosina @ 2015-09-30 19:40 UTC (permalink / raw)
To: Luis de Bethencourt; +Cc: linux-kernel, linux-usb, linux-input
On Wed, 30 Sep 2015, Luis de Bethencourt wrote:
> The driver is using -1 instead of the -ENOMEM defined macro to specify
> that a buffer allocation failed. Since the error number is propagated,
> the caller will get a -EPERM which is the wrong error condition.
Generally I agree that the more specific errno, the better.
But I am not really sure where you are seeing the bug (mapping to -EPERM)
in this case? I think the only caller of hiddev_connect() should be
hid_connect(), and the only thing that guy cares about whether individual
callbacks succeed or fail, so that it sets hdev->clamed flags accordingly.
Could you please be more specific about the -EPERM mapping you are talking
about?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: hiddev: fix returned errno code in hiddev_connect()
2015-09-30 19:40 ` Jiri Kosina
@ 2015-10-03 21:34 ` Luis de Bethencourt
2015-10-05 14:24 ` Jiri Kosina
0 siblings, 1 reply; 8+ messages in thread
From: Luis de Bethencourt @ 2015-10-03 21:34 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-kernel, linux-usb, linux-input
On 30/09/15 20:40, Jiri Kosina wrote:
> On Wed, 30 Sep 2015, Luis de Bethencourt wrote:
>
>> The driver is using -1 instead of the -ENOMEM defined macro to specify
>> that a buffer allocation failed. Since the error number is propagated,
>> the caller will get a -EPERM which is the wrong error condition.
>
> Generally I agree that the more specific errno, the better.
>
> But I am not really sure where you are seeing the bug (mapping to -EPERM)
> in this case? I think the only caller of hiddev_connect() should be
> hid_connect(), and the only thing that guy cares about whether individual
> callbacks succeed or fail, so that it sets hdev->clamed flags accordingly.
>
> Could you please be more specific about the -EPERM mapping you are talking
> about?
>
> Thanks,
>
I agree with you. The only caller of hiddev_connect() only checks if the
callback succeded. It checks if the return < 0.
What I meant is that -1 means -EPERM. [0]
This patch is purely about the correctness of using -ENOMEM. The word
"propagated" was not the best way to describe this problem. I could edit
the commit message if you would like.
Thanks for the review,
Luis
[0] http://lxr.free-electrons.com/source/include/uapi/asm-generic/errno-base.h#L15
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: hiddev: fix returned errno code in hiddev_connect()
2015-10-03 21:34 ` Luis de Bethencourt
@ 2015-10-05 14:24 ` Jiri Kosina
2015-10-06 8:58 ` Luis de Bethencourt
0 siblings, 1 reply; 8+ messages in thread
From: Jiri Kosina @ 2015-10-05 14:24 UTC (permalink / raw)
To: Luis de Bethencourt; +Cc: linux-kernel, linux-usb, linux-input
On Sat, 3 Oct 2015, Luis de Bethencourt wrote:
> > But I am not really sure where you are seeing the bug (mapping to
> > -EPERM) in this case? I think the only caller of hiddev_connect()
> > should be hid_connect(), and the only thing that guy cares about
> > whether individual callbacks succeed or fail, so that it sets
> > hdev->clamed flags accordingly.
> >
> > Could you please be more specific about the -EPERM mapping you are
> > talking about?
> >
> I agree with you. The only caller of hiddev_connect() only checks if the
> callback succeded. It checks if the return < 0.
> What I meant is that -1 means -EPERM. [0]
I still don't understand what problem you are chasing here, sorry. EPERM
is defined to be 1, yes. So are many other completely unrelated #defines.
> This patch is purely about the correctness of using -ENOMEM. The word
> "propagated" was not the best way to describe this problem. I could edit
> the commit message if you would like.
You seem to imply that someone might be interpreting that -1 as a define
from errno.h. But that's not the case.
Are you going to look at every 'return -1' occurence in the kernel and
convert it to something else? That can keep you busy for quite some time:
$ git grep 'return -1' | wc -l
9167
The only cleanup I'd imagine at least remotely possible in this case would
be to convert the ->connect() callbacks return bool.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] HID: hiddev: fix returned errno code in hiddev_connect()
2015-10-05 14:24 ` Jiri Kosina
@ 2015-10-06 8:58 ` Luis de Bethencourt
0 siblings, 0 replies; 8+ messages in thread
From: Luis de Bethencourt @ 2015-10-06 8:58 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-kernel, linux-usb, linux-input
On 05/10/15 15:24, Jiri Kosina wrote:
> On Sat, 3 Oct 2015, Luis de Bethencourt wrote:
>
>>> But I am not really sure where you are seeing the bug (mapping to
>>> -EPERM) in this case? I think the only caller of hiddev_connect()
>>> should be hid_connect(), and the only thing that guy cares about
>>> whether individual callbacks succeed or fail, so that it sets
>>> hdev->clamed flags accordingly.
>>>
>>> Could you please be more specific about the -EPERM mapping you are
>>> talking about?
>>>
>> I agree with you. The only caller of hiddev_connect() only checks if the
>> callback succeded. It checks if the return < 0.
>> What I meant is that -1 means -EPERM. [0]
>
> I still don't understand what problem you are chasing here, sorry. EPERM
> is defined to be 1, yes. So are many other completely unrelated #defines.
>
>> This patch is purely about the correctness of using -ENOMEM. The word
>> "propagated" was not the best way to describe this problem. I could edit
>> the commit message if you would like.
>
> You seem to imply that someone might be interpreting that -1 as a define
> from errno.h. But that's not the case.
>
> Are you going to look at every 'return -1' occurence in the kernel and
> convert it to something else? That can keep you busy for quite some time:
>
> $ git grep 'return -1' | wc -l
> 9167
>
> The only cleanup I'd imagine at least remotely possible in this case would
> be to convert the ->connect() callbacks return bool.
>
This is a very good point. I will write a second version of the patch that
converts the return to a boolean.
Thanks for the review,
Luis
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-10-06 8:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-30 9:52 [PATCH] HID: hiddev: fix returned errno code in hiddev_connect() Luis de Bethencourt
2015-09-30 9:56 ` Luis de Bethencourt
2015-09-30 10:04 ` Sudip Mukherjee
2015-09-30 10:05 ` Luis de Bethencourt
2015-09-30 19:40 ` Jiri Kosina
2015-10-03 21:34 ` Luis de Bethencourt
2015-10-05 14:24 ` Jiri Kosina
2015-10-06 8:58 ` Luis de Bethencourt
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).