| Message ID | 1353478830-23788-1-git-send-email-sachin.kamat@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
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,
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.
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,
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
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 --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);
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(-)