diff mbox

[1/2] hid: usbhid: Return -ENOMEM instead of -1 for memory allocation failure

Message ID 1353478830-23788-1-git-send-email-sachin.kamat@linaro.org (mailing list archive)
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

Sachin Kamat Nov. 21, 2012, 6:20 a.m. UTC
Silences the following smatch warning:
drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
returning -1 instead of -ENOMEM is sloppy

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/hid/usbhid/hiddev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Jiri Kosina Nov. 22, 2012, 9:03 p.m. UTC | #1
On Wed, 21 Nov 2012, Sachin Kamat wrote:

> Silences the following smatch warning:
> drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
> returning -1 instead of -ENOMEM is sloppy
> 
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/hid/usbhid/hiddev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 14599e2..50e2ab8 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);

Well, this would make sense only if the callers would be actualling doing 
something useful with that return value. But the only check we are 
performing at callsites is non-zero test ...

Thanks,
Dmitry Torokhov Nov. 26, 2012, 6:05 p.m. UTC | #2
On Thu, Nov 22, 2012 at 10:03:44PM +0100, Jiri Kosina wrote:
> On Wed, 21 Nov 2012, Sachin Kamat wrote:
> 
> > Silences the following smatch warning:
> > drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
> > returning -1 instead of -ENOMEM is sloppy
> > 
> > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> > ---
> >  drivers/hid/usbhid/hiddev.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> > index 14599e2..50e2ab8 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);
> 
> Well, this would make sense only if the callers would be actualling doing 
> something useful with that return value. But the only check we are 
> performing at callsites is non-zero test ...

It is chicken and egg problem - callers can't use the result unless it
is meaningful and callee's do not bother to send anything meaningful
because nobody uses it...

If someone takes time to convert to proper return codes I think it would
be a good thing.

Thanks.
Jiri Kosina Nov. 26, 2012, 10:09 p.m. UTC | #3
On Mon, 26 Nov 2012, Dmitry Torokhov wrote:

> > > Silences the following smatch warning:
> > > drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
> > > returning -1 instead of -ENOMEM is sloppy
> > > 
> > > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> > > ---
> > >  drivers/hid/usbhid/hiddev.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> > > index 14599e2..50e2ab8 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);
> > 
> > Well, this would make sense only if the callers would be actualling doing 
> > something useful with that return value. But the only check we are 
> > performing at callsites is non-zero test ...
> 
> It is chicken and egg problem - callers can't use the result unless it
> is meaningful and callee's do not bother to send anything meaningful
> because nobody uses it...
> 
> If someone takes time to convert to proper return codes I think it would
> be a good thing.

Fully agree with you.

But I'd like to have both sides changed together once someone starts 
taking care of fixing it.

Thanks,
Sachin Kamat Nov. 27, 2012, 4:42 a.m. UTC | #4
Hi Jiri,

On 27 November 2012 03:39, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 26 Nov 2012, Dmitry Torokhov wrote:
>
>> > > Silences the following smatch warning:
>> > > drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
>> > > returning -1 instead of -ENOMEM is sloppy
>> > >
>> > > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> > > ---
>> > >  drivers/hid/usbhid/hiddev.c |    2 +-
>> > >  1 files changed, 1 insertions(+), 1 deletions(-)
>> > >
>> > > diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
>> > > index 14599e2..50e2ab8 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);
>> >
>> > Well, this would make sense only if the callers would be actualling doing
>> > something useful with that return value. But the only check we are
>> > performing at callsites is non-zero test ...
>>
>> It is chicken and egg problem - callers can't use the result unless it
>> is meaningful and callee's do not bother to send anything meaningful
>> because nobody uses it...
>>
>> If someone takes time to convert to proper return codes I think it would
>> be a good thing.
>
> Fully agree with you.
>
> But I'd like to have both sides changed together once someone starts
> taking care of fixing it.

The proposed change is done in the function hiddev_connect which
returns 0 on success.
The caller only checks if the return value is 0 or not
(!hdev->hiddev_connect(...)). In this case
what is the change that you would like to see on the caller side?

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
Jiri Kosina Nov. 27, 2012, 9:55 a.m. UTC | #5
On Tue, 27 Nov 2012, Sachin Kamat wrote:

> > Fully agree with you.
> >
> > But I'd like to have both sides changed together once someone starts
> > taking care of fixing it.
> 
> The proposed change is done in the function hiddev_connect which
> returns 0 on success.
> The caller only checks if the return value is 0 or not
> (!hdev->hiddev_connect(...)). In this case
> what is the change that you would like to see on the caller side?

A check whether the value returned by hiddev_connect() is error code 
(through IS_ERR_VALUE() or something similar), and continue propagating 
the error out of hid_connect().
diff mbox

Patch

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 14599e2..50e2ab8 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);