[PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Shivani Bhardwaj]

The variable rc is supposed to hold the value returned by function
kstrtoul which can either be 0, -EINVAL (-22) or -ERANGE (-34).
Therefore, the check must be performed on rc instead of val for error
values. Also, rc can never be greater than 0.

Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
---
Changes in v2:
	Update the commit message.

 drivers/staging/lustre/lustre/osc/lproc_osc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c
index 053d508..cdc7f88 100644
--- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
+++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
@@ -61,9 +61,7 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
 	unsigned long val;
 
 	rc = kstrtoul(buffer, 10, &val);
-	if (rc)
-		return rc;
-	if (val < 0 || val > 1)
+	if (rc < 0)
 		return -ERANGE;
 
 	/* opposite senses */
-- 
2.1.0

Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Arnd Bergmann]

On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:
> index 053d508..cdc7f88 100644
> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
> @@ -61,9 +61,7 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
>         unsigned long val;
>  
>         rc = kstrtoul(buffer, 10, &val);
> -       if (rc)
> -               return rc;
> -       if (val < 0 || val > 1)
> +       if (rc < 0)
>                 return -ERANGE;
>  
>         /* opposite senses */
> -- 
> 

I believe the intention of the code is to return -ERANGE if anything other
than 0 or 1 is stored here, which no longer works after your patch. The
range check in kstrtoul only ensures that there is a number that fits within
an 'unsigned long'.

	Arnd

Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Shivani Bhardwaj]

Shivani


On Fri, Oct 16, 2015 at 2:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:
>> index 053d508..cdc7f88 100644
>> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> @@ -61,9 +61,7 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
>>         unsigned long val;
>>
>>         rc = kstrtoul(buffer, 10, &val);
>> -       if (rc)
>> -               return rc;
>> -       if (val < 0 || val > 1)
>> +       if (rc < 0)
>>                 return -ERANGE;
>>
>>         /* opposite senses */
>> --
>>
>
> I believe the intention of the code is to return -ERANGE if anything other
> than 0 or 1 is stored here, which no longer works after your patch. The
> range check in kstrtoul only ensures that there is a number that fits within
> an 'unsigned long'.

This means it is never going to throw out of range error?
If that is the case why do we take the variable val as unsigned long?
It can be typecasted in the function kstrtoul. We can take it as
simple int. And still
if (rc)
     return rc;

statement is never going to be valid, is it?
Plus, why base 10? base 2 should work fine to check for 0 and 1.

And, if everything above is wrong then, since val is taken as unsigned
int it can again never be less than 0 so, shouldn't the check be made
only for val>1?

>
>         Arnd

Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Arnd Bergmann]

On Friday 16 October 2015 14:27:35 Shivani Bhardwaj wrote:
> On Fri, Oct 16, 2015 at 2:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:
> >> index 053d508..cdc7f88 100644
> >> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
> >> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
> >> @@ -61,9 +61,7 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
> >>         unsigned long val;
> >>
> >>         rc = kstrtoul(buffer, 10, &val);
> >> -       if (rc)
> >> -               return rc;
> >> -       if (val < 0 || val > 1)
> >> +       if (rc < 0)
> >>                 return -ERANGE;
> >>
> >>         /* opposite senses */
> >> --
> >>
> >
> > I believe the intention of the code is to return -ERANGE if anything other
> > than 0 or 1 is stored here, which no longer works after your patch. The
> > range check in kstrtoul only ensures that there is a number that fits within
> > an 'unsigned long'.
> 
> This means it is never going to throw out of range error?
> If that is the case why do we take the variable val as unsigned long?
> It can be typecasted in the function kstrtoul. We can take it as
> simple int. And still
> if (rc)
>      return rc;
> 
> statement is never going to be valid, is it?

A used may still write "aardvark" into the file, which would return
-EINVAL, and writing 999999999999999999 would return -ERANGE.

> Plus, why base 10? base 2 should work fine to check for 0 and 1.

Right, it doesn't matter here. 'base=0' (autodetect) would work too.

> And, if everything above is wrong then, since val is taken as unsigned
> int it can again never be less than 0 so, shouldn't the check be made
> only for val>1?

Yes, that would be correct.

	Arnd

Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Shivani Bhardwaj]

Shivani


On Fri, Oct 16, 2015 at 3:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 16 October 2015 14:27:35 Shivani Bhardwaj wrote:
>> On Fri, Oct 16, 2015 at 2:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:
>> >> index 053d508..cdc7f88 100644
>> >> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> >> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> >> @@ -61,9 +61,7 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
>> >>         unsigned long val;
>> >>
>> >>         rc = kstrtoul(buffer, 10, &val);
>> >> -       if (rc)
>> >> -               return rc;
>> >> -       if (val < 0 || val > 1)
>> >> +       if (rc < 0)
>> >>                 return -ERANGE;
>> >>
>> >>         /* opposite senses */
>> >> --
>> >>
>> >
>> > I believe the intention of the code is to return -ERANGE if anything other
>> > than 0 or 1 is stored here, which no longer works after your patch. The
>> > range check in kstrtoul only ensures that there is a number that fits within
>> > an 'unsigned long'.
>>
>> This means it is never going to throw out of range error?
>> If that is the case why do we take the variable val as unsigned long?
>> It can be typecasted in the function kstrtoul. We can take it as
>> simple int. And still
>> if (rc)
>>      return rc;
>>
>> statement is never going to be valid, is it?
>
> A used may still write "aardvark" into the file, which would return
> -EINVAL, and writing 999999999999999999 would return -ERANGE.
>
>> Plus, why base 10? base 2 should work fine to check for 0 and 1.
>
> Right, it doesn't matter here. 'base=0' (autodetect) would work too.
>
>> And, if everything above is wrong then, since val is taken as unsigned
>> int it can again never be less than 0 so, shouldn't the check be made
>> only for val>1?
>
> Yes, that would be correct.
>
>         Arnd
>
Fine. I'll make this change.
Thank you.

Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Shivani Bhardwaj]

Hi Arnd! I still have a doubt.
if (rc)
     return rc;

still doesn't make any sense, right?

On Fri, Oct 16, 2015 at 4:32 PM, Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> Shivani
>
>
> On Fri, Oct 16, 2015 at 3:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Friday 16 October 2015 14:27:35 Shivani Bhardwaj wrote:
>>> On Fri, Oct 16, 2015 at 2:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> > On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:
>>> >> index 053d508..cdc7f88 100644
>>> >> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
>>> >> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>>> >> @@ -61,9 +61,7 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
>>> >>         unsigned long val;
>>> >>
>>> >>         rc = kstrtoul(buffer, 10, &val);
>>> >> -       if (rc)
>>> >> -               return rc;
>>> >> -       if (val < 0 || val > 1)
>>> >> +       if (rc < 0)
>>> >>                 return -ERANGE;
>>> >>
>>> >>         /* opposite senses */
>>> >> --
>>> >>
>>> >
>>> > I believe the intention of the code is to return -ERANGE if anything other
>>> > than 0 or 1 is stored here, which no longer works after your patch. The
>>> > range check in kstrtoul only ensures that there is a number that fits within
>>> > an 'unsigned long'.
>>>
>>> This means it is never going to throw out of range error?
>>> If that is the case why do we take the variable val as unsigned long?
>>> It can be typecasted in the function kstrtoul. We can take it as
>>> simple int. And still
>>> if (rc)
>>>      return rc;
>>>
>>> statement is never going to be valid, is it?
>>
>> A used may still write "aardvark" into the file, which would return
>> -EINVAL, and writing 999999999999999999 would return -ERANGE.
>>
>>> Plus, why base 10? base 2 should work fine to check for 0 and 1.
>>
>> Right, it doesn't matter here. 'base=0' (autodetect) would work too.
>>
>>> And, if everything above is wrong then, since val is taken as unsigned
>>> int it can again never be less than 0 so, shouldn't the check be made
>>> only for val>1?
>>
>> Yes, that would be correct.
>>
>>         Arnd
>>
> Fine. I'll make this change.
> Thank you.

Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Arnd Bergmann]

On Sunday 18 October 2015 16:01:48 Shivani Bhardwaj wrote:
> Hi Arnd! I still have a doubt.
> if (rc)
>      return rc;
> 
> still doesn't make any sense, right?

I don't understand what you mean.

> On Fri, Oct 16, 2015 at 4:32 PM, Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> > On Fri, Oct 16, 2015 at 3:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >>>
> >>> This means it is never going to throw out of range error?
> >>> If that is the case why do we take the variable val as unsigned long?
> >>> It can be typecasted in the function kstrtoul. We can take it as
> >>> simple int. And still
> >>> if (rc)
> >>>      return rc;
> >>>
> >>> statement is never going to be valid, is it?
> >>
> >> A used may still write "aardvark" into the file, which would return
> >> -EINVAL, and writing 999999999999999999 would return -ERANGE.

Anything wrong with my explanation here?

	Arnd

Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Shivani Bhardwaj]

On Sun, Oct 18, 2015 at 6:24 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday 18 October 2015 16:01:48 Shivani Bhardwaj wrote:
>> Hi Arnd! I still have a doubt.
>> if (rc)
>>      return rc;
>>
>> still doesn't make any sense, right?
>
> I don't understand what you mean.
>
>> On Fri, Oct 16, 2015 at 4:32 PM, Shivani Bhardwaj <shivanib134@gmail.com> wrote:
>> > On Fri, Oct 16, 2015 at 3:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >>>
>> >>> This means it is never going to throw out of range error?
>> >>> If that is the case why do we take the variable val as unsigned long?
>> >>> It can be typecasted in the function kstrtoul. We can take it as
>> >>> simple int. And still
>> >>> if (rc)
>> >>>      return rc;
>> >>>
>> >>> statement is never going to be valid, is it?
>> >>
>> >> A used may still write "aardvark" into the file, which would return
>> >> -EINVAL, and writing 999999999999999999 would return -ERANGE.
>
> Anything wrong with my explanation here?
>
>         Arnd

According to your explanation, -EINVAL or -ERANGE if failure or 0 if success.
if(rc)     ->   if rc happens to be true which means >0.
This is never going to happen. Right?
So, this

if(rc)
     return rc;

stays useless. Doesn't it?
I'm just asking to avoid introducing bugs if I have misunderstood the thing.

Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Arnd Bergmann]

On Sunday 18 October 2015 18:32:06 Shivani Bhardwaj wrote:
> On Sun, Oct 18, 2015 at 6:24 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Sunday 18 October 2015 16:01:48 Shivani Bhardwaj wrote:
> >> Hi Arnd! I still have a doubt.
> >> if (rc)
> >>      return rc;
> >>
> >> still doesn't make any sense, right?
> >
> > I don't understand what you mean.
> >
> >> On Fri, Oct 16, 2015 at 4:32 PM, Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> >> > On Fri, Oct 16, 2015 at 3:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> >>>
> >> >>> This means it is never going to throw out of range error?
> >> >>> If that is the case why do we take the variable val as unsigned long?
> >> >>> It can be typecasted in the function kstrtoul. We can take it as
> >> >>> simple int. And still
> >> >>> if (rc)
> >> >>>      return rc;
> >> >>>
> >> >>> statement is never going to be valid, is it?
> >> >>
> >> >> A used may still write "aardvark" into the file, which would return
> >> >> -EINVAL, and writing 999999999999999999 would return -ERANGE.
> >
> > Anything wrong with my explanation here?
> >
> >         Arnd
> 
> According to your explanation, -EINVAL or -ERANGE if failure or 0 if success.
> if(rc)     ->   if rc happens to be true which means >0.
> This is never going to happen. Right?

No. 'if (rc)' means 'if (rc != 0)', so the condition is also true when rc<0.

	Arnd

Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked

[Shivani Bhardwaj]

On Sun, Oct 18, 2015 at 6:42 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sunday 18 October 2015 18:32:06 Shivani Bhardwaj wrote:
>> On Sun, Oct 18, 2015 at 6:24 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Sunday 18 October 2015 16:01:48 Shivani Bhardwaj wrote:
>> >> Hi Arnd! I still have a doubt.
>> >> if (rc)
>> >>      return rc;
>> >>
>> >> still doesn't make any sense, right?
>> >
>> > I don't understand what you mean.
>> >
>> >> On Fri, Oct 16, 2015 at 4:32 PM, Shivani Bhardwaj <shivanib134@gmail.com> wrote:
>> >> > On Fri, Oct 16, 2015 at 3:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> >>>
>> >> >>> This means it is never going to throw out of range error?
>> >> >>> If that is the case why do we take the variable val as unsigned long?
>> >> >>> It can be typecasted in the function kstrtoul. We can take it as
>> >> >>> simple int. And still
>> >> >>> if (rc)
>> >> >>>      return rc;
>> >> >>>
>> >> >>> statement is never going to be valid, is it?
>> >> >>
>> >> >> A used may still write "aardvark" into the file, which would return
>> >> >> -EINVAL, and writing 999999999999999999 would return -ERANGE.
>> >
>> > Anything wrong with my explanation here?
>> >
>> >         Arnd
>>
>> According to your explanation, -EINVAL or -ERANGE if failure or 0 if success.
>> if(rc)     ->   if rc happens to be true which means >0.
>> This is never going to happen. Right?
>
> No. 'if (rc)' means 'if (rc != 0)', so the condition is also true when rc<0.
>
>         Arnd

OK. Thanks.