* Re: [PATCH] sparc: vDSO: Silence an uninitialized variable warning
2018-10-13 10:26 [PATCH] sparc: vDSO: Silence an uninitialized variable warning Dan Carpenter
@ 2018-10-13 18:18 ` Shannon Nelson
2018-10-14 7:52 ` Dan Carpenter
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Shannon Nelson @ 2018-10-13 18:18 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] sparc: vDSO: Silence an uninitialized variable warning
2018-10-13 10:26 [PATCH] sparc: vDSO: Silence an uninitialized variable warning Dan Carpenter
2018-10-13 18:18 ` Shannon Nelson
@ 2018-10-14 7:52 ` Dan Carpenter
2018-10-14 22:44 ` Shannon Nelson
2018-10-18 4:55 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2018-10-14 7:52 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] sparc: vDSO: Silence an uninitialized variable warning
2018-10-13 10:26 [PATCH] sparc: vDSO: Silence an uninitialized variable warning Dan Carpenter
2018-10-13 18:18 ` Shannon Nelson
2018-10-14 7:52 ` Dan Carpenter
@ 2018-10-14 22:44 ` Shannon Nelson
2018-10-18 4:55 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: Shannon Nelson @ 2018-10-14 22:44 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] sparc: vDSO: Silence an uninitialized variable warning
2018-10-13 10:26 [PATCH] sparc: vDSO: Silence an uninitialized variable warning Dan Carpenter
` (2 preceding siblings ...)
2018-10-14 22:44 ` Shannon Nelson
@ 2018-10-18 4:55 ` David Miller
3 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2018-10-18 4:55 UTC (permalink / raw)
To: kernel-janitors
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.
^ permalink raw reply [flat|nested] 5+ messages in thread