[PATCH] staging: dgap: fix returned errno code in dgap_parsefile()

[PATCH] staging: dgap: fix returned errno code in dgap_parsefile()

[Javier Martinez Canillas]

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:

dgap_parsefile() warn: returning -1 instead of -ENOMEM is sloppy

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/staging/dgap/dgap.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/dgap/dgap.c b/drivers/staging/dgap/dgap.c
index 303d97023ccb..e17bde7bf416 100644
--- a/drivers/staging/dgap/dgap.c
+++ b/drivers/staging/dgap/dgap.c
@@ -642,7 +642,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 
@@ -861,7 +861,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = TNODE;
@@ -883,7 +883,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = CUNODE;
@@ -914,7 +914,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = LNODE;
@@ -933,7 +933,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = CNODE;
@@ -975,7 +975,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = MNODE;
@@ -1054,7 +1054,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = PNODE;
@@ -1076,7 +1076,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = JNODE;
@@ -1098,7 +1098,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = ANODE;
@@ -1120,7 +1120,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = INTRNODE;
@@ -1141,7 +1141,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = TSNODE;
@@ -1163,7 +1163,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = CSNODE;
@@ -1185,7 +1185,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = BSNODE;
@@ -1207,7 +1207,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = USNODE;
@@ -1229,7 +1229,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = FSNODE;
@@ -1251,7 +1251,7 @@ static int dgap_parsefile(char **in)
 
 			p->next = kzalloc(sizeof(struct cnode), GFP_KERNEL);
 			if (!p->next)
-				return -1;
+				return -ENOMEM;
 
 			p = p->next;
 			p->type = VSNODE;
-- 
2.4.3

Re: [PATCH] staging: dgap: fix returned errno code in dgap_parsefile()

[Sudip Mukherjee]

On Tue, Sep 22, 2015 at 02:39:36AM +0200, Javier Martinez Canillas 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.
Just a little doubt. caller means the function which is calling this
dgap_parsefile() or you meant the user?
The function which is calling this dgap_parsefile() is just checking if
it has received 0 or something else. Something else is error and it
rerturns -EINVAL for all types of error (ofcourse that is also wrong).
So the user will see -EINVAL for all types of error in dgap_parsefile().

regards
sudip

Re: [PATCH] staging: dgap: fix returned errno code in dgap_parsefile()

[Javier Martinez Canillas]

Hello Sudip,

On 09/22/2015 06:52 AM, Sudip Mukherjee wrote:
> On Tue, Sep 22, 2015 at 02:39:36AM +0200, Javier Martinez Canillas 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.
> Just a little doubt. caller means the function which is calling this
> dgap_parsefile() or you meant the user?

I meant whatever function calls dgap_parsefile(), which currently is
only dgap_firmware_load().

> The function which is calling this dgap_parsefile() is just checking if
> it has received 0 or something else. Something else is error and it
> rerturns -EINVAL for all types of error (ofcourse that is also wrong).
> So the user will see -EINVAL for all types of error in dgap_parsefile().
>

Yes, I also verified what dgap_firmware_load() does with the returned error
code to make sure that it was safe to do this change without affecting the
rest of the driver.

But I believe the patch and what the commit message says is true regardless
of the fact that the caller is just checking for != 0. dgap_firmware_load()
stills gets a wrong error condition whether it's checking it or not.
 
> regards
> sudip
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH] staging: dgap: fix returned errno code in dgap_parsefile()

[Sudip Mukherjee]

On Tue, Sep 22, 2015 at 08:38:43AM +0200, Javier Martinez Canillas wrote:
> Hello Sudip,
> 
> On 09/22/2015 06:52 AM, Sudip Mukherjee wrote:
> > On Tue, Sep 22, 2015 at 02:39:36AM +0200, Javier Martinez Canillas 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.
> > Just a little doubt. caller means the function which is calling this
> > dgap_parsefile() or you meant the user?
<snip>
> 
> But I believe the patch and what the commit message says is true regardless
> of the fact that the caller is just checking for != 0. dgap_firmware_load()
> stills gets a wrong error condition whether it's checking it or not.
Yes. I just had a doubt what you meant by caller. If user then I would
have said that "patch is correct but commit message is not". :)

regards
sudip