On 10/14/2018 12:52 AM, Dan Carpenter wrote:
On Sat, Oct 13, 2018 at 11:18:20AM -0700, Shannon Nelson wrote:
On 10/13/2018 3:26 AM, Dan Carpenter wrote:
Smatch complains that "val" would be uninitialized if kstrtoul() fails.
Fixes: 9a08862a5d2e ("vDSO for sparc")
Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
---
arch/sparc/vdso/vma.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/sparc/vdso/vma.c b/arch/sparc/vdso/vma.c
index f51595f861b8..5eaff3c1aa0c 100644
--- a/arch/sparc/vdso/vma.c
+++ b/arch/sparc/vdso/vma.c
@@ -262,7 +262,9 @@ static __init int vdso_setup(char *s)
unsigned long val;
err = kstrtoul(s, 10, &val);
+ if (err)
+ return err;
vdso_enabled = val;
- return err;
+ return 0;
}
__setup("vdso=", vdso_setup);
This is probably fine, but it might be cleaner as
err = kstrtoul(s, 10, &val);
if (!err)
vdso_enabled = val;
return err;
I always tell people to do failure handling instead of success handling.
And also I tell people not to make the last if statement in a function
a special case. Like you see this a lot right:
ret = frob();
if (ret)
return ret;
ret = frob();
if (ret)
return ret;
ret = frob();
if (!ret)
*p = whatever;
return ret;
If you do failure handling then the success path is at indent level one
and failure handling is two tabs. But if you mix it up it's not as
clear. I have never studied this, but my impression from looking at
static analysis over the years is that success handling is error prone.
regards,
dan carpenter
(driving right into the bikeshed...)
Yes, and that's the way I would normally go, especially if there was
more to that little function. But in this case with such a short little
function, I hate seeing it messed up with unnecessary lines.
Anyway, enough with the noise, either way is fine.
sln
