Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1229860 > unrolled thread

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

Started byJavier Martinez Canillas <javier@osg.samsung.com>
First post2015-09-22 02:40 +0200
Last post2015-09-22 09:00 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging: dgap: fix returned errno code in dgap_parsefile() Javier Martinez Canillas <javier@osg.samsung.com> - 2015-09-22 02:40 +0200
    Re: [PATCH] staging: dgap: fix returned errno code in  dgap_parsefile() Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-09-22 07:00 +0200
      Re: [PATCH] staging: dgap: fix returned errno code in  dgap_parsefile() Javier Martinez Canillas <javier@osg.samsung.com> - 2015-09-22 08:40 +0200
        Re: [PATCH] staging: dgap: fix returned errno code in  dgap_parsefile() Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2015-09-22 09:00 +0200

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

FromJavier Martinez Canillas <javier@osg.samsung.com>
Date2015-09-22 02:40 +0200
Subject[PATCH] staging: dgap: fix returned errno code in dgap_parsefile()
Message-ID<qbjMK-64I-11@gated-at.bofh.it>
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2015-09-22 07:00 +0200
SubjectRe: [PATCH] staging: dgap: fix returned errno code in dgap_parsefile()
Message-ID<qbnQl-3r8-7@gated-at.bofh.it>
In reply to#1229860
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

FromJavier Martinez Canillas <javier@osg.samsung.com>
Date2015-09-22 08:40 +0200
SubjectRe: [PATCH] staging: dgap: fix returned errno code in dgap_parsefile()
Message-ID<qbpp7-5Qv-3@gated-at.bofh.it>
In reply to#1229904
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

FromSudip Mukherjee <sudipm.mukherjee@gmail.com>
Date2015-09-22 09:00 +0200
SubjectRe: [PATCH] staging: dgap: fix returned errno code in dgap_parsefile()
Message-ID<qbpIt-6cQ-1@gated-at.bofh.it>
In reply to#1229926
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web