[PATCH] sparc: vDSO: Silence an uninitialized variable warning

[PATCH] sparc: vDSO: Silence an uninitialized variable warning

[Dan Carpenter]

Smatch complains that "val" would be uninitialized if kstrtoul() fails.

Fixes: 9a08862a5d2e ("vDSO for sparc")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 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);
-- 
2.18.0

Re: [PATCH] sparc: vDSO: Silence an uninitialized variable warning

[Shannon Nelson]

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@oracle.com>
> ---
>   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;

sln

Re: [PATCH] sparc: vDSO: Silence an uninitialized variable warning

[Dan Carpenter]

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@oracle.com>
> > ---
> >   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

Re: [PATCH] sparc: vDSO: Silence an uninitialized variable warning

[Shannon Nelson]

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@oracle.com>
>>> ---
>>>    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

Re: [PATCH] sparc: vDSO: Silence an uninitialized variable warning

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Sat, 13 Oct 2018 13:26:53 +0300

> Smatch complains that "val" would be uninitialized if kstrtoul() fails.
> 
> Fixes: 9a08862a5d2e ("vDSO for sparc")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

So much bike shedding in this thread ;-)

Applied, thanks Dan.