Staging: lustre: lpoc_osc: Change the variable to be checked

PATCH In review Shivani Bhardwaj 2 participants 10 messages Oct 16, 2015 – Oct 18, 2015 Staging

Share & tools

Shivani Bhardwaj

author

2015-10-16 08:32 UTC #0 lore raw

PATCH v2 1/1 Staging: lustre: lpoc_osc: Change the variable to be checked

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 <[email protected]>

---
Changes in v2:
Update the commit message.

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

drivers/staging/lustre/lustre/osc/lproc_osc.c +1 -3

index 053d508..cdc7f88 100644
@@ -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 */

Signature · 1 line

2.1.0

Arnd Bergmann

2015-10-16 08:41 UTC #1 lore raw

Quoted 6 lines · On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:

index 053d508..cdc7f88 100644

drivers/staging/lustre/lustre/osc/lproc_osc.c static ssize_t active_store(struct kobject *kobj, struct attribute *attr, · line 67

Quoted 10 lines

drivers/staging/lustre/lustre/osc/lproc_osc.c +1 -3

@@ -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

Shivani Bhardwaj

author

2015-10-16 08:57 UTC #2 lore raw

Shivani

Quoted 14 lines · Arnd Bergmann

Quoted 13 lines · On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:

index 053d508..cdc7f88 100644

drivers/staging/lustre/lustre/osc/lproc_osc.c +1 -3

@@ -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;

Quoted 8 lines

drivers/staging/lustre/lustre/osc/lproc_osc.c static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
Quoted 3 lines
drivers/staging/lustre/lustre/osc/lproc_osc.c +0 -1
        /* 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?

Quoted 2 lines

Arnd

Arnd Bergmann

2015-10-16 09:55 UTC #3 lore raw

Quoted 24 lines · On Friday 16 October 2015 14:27:35 Shivani Bhardwaj wrote:

Quoted 14 lines · Arnd Bergmann
Quoted 13 lines · On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:
index 053d508..cdc7f88 100644
drivers/staging/lustre/lustre/osc/lproc_osc.c +1 -3
@@ -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;
 
Quoted 8 lines
drivers/staging/lustre/lustre/osc/lproc_osc.c static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
Quoted 3 lines
drivers/staging/lustre/lustre/osc/lproc_osc.c +0 -1
        /* 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'.

Quoted 8 lines

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.

Quoted 1 line

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.

Quoted 3 lines

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

Shivani Bhardwaj

author

2015-10-16 11:02 UTC #4 lore raw

Shivani

Quoted 41 lines · Arnd Bergmann

Quoted 24 lines · On Friday 16 October 2015 14:27:35 Shivani Bhardwaj wrote:
Quoted 14 lines · Arnd Bergmann
Quoted 13 lines · On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:
index 053d508..cdc7f88 100644
drivers/staging/lustre/lustre/osc/lproc_osc.c +1 -3
@@ -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;
 
Quoted 8 lines
drivers/staging/lustre/lustre/osc/lproc_osc.c static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
Quoted 3 lines
drivers/staging/lustre/lustre/osc/lproc_osc.c +0 -1
        /* 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'.
Quoted 8 lines
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.
Quoted 1 line
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.

Quoted 8 lines

Quoted 3 lines
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.

Shivani Bhardwaj

author

2015-10-18 10:31 UTC #5 lore raw

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

     return rc;

still doesn't make any sense, right?

Quoted 55 lines · Shivani Bhardwaj

Shivani
Quoted 41 lines · Arnd Bergmann
Quoted 24 lines · On Friday 16 October 2015 14:27:35 Shivani Bhardwaj wrote:
Quoted 14 lines · Arnd Bergmann
Quoted 13 lines · On Friday 16 October 2015 14:02:18 Shivani Bhardwaj wrote:
index 053d508..cdc7f88 100644
drivers/staging/lustre/lustre/osc/lproc_osc.c +1 -3
@@ -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;
 
Quoted 8 lines
drivers/staging/lustre/lustre/osc/lproc_osc.c static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
Quoted 3 lines
drivers/staging/lustre/lustre/osc/lproc_osc.c +0 -1
        /* 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'.
Quoted 8 lines
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.
Quoted 1 line
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.
Quoted 8 lines
Quoted 3 lines
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.

Arnd Bergmann

2015-10-18 12:54 UTC #6 lore raw

Quoted 5 lines · 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.

Quoted 10 lines · Shivani Bhardwaj

Quoted 9 lines · Arnd Bergmann
Quoted 8 lines
Quoted 8 lines
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;

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 1 line
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

Shivani Bhardwaj

author

2015-10-18 13:02 UTC #7 lore raw

Quoted 19 lines · Arnd Bergmann

Quoted 5 lines · 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.
Quoted 10 lines · Shivani Bhardwaj
Quoted 9 lines · Arnd Bergmann
Quoted 8 lines
Quoted 8 lines
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;

Quoted 8 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
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.

Arnd Bergmann

2015-10-18 13:12 UTC #8 lore raw

Quoted 25 lines · On Sunday 18 October 2015 18:32:06 Shivani Bhardwaj wrote:

Quoted 24 lines · Arnd Bergmann
Quoted 5 lines · 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.
Quoted 14 lines · Shivani Bhardwaj
Quoted 13 lines · Arnd Bergmann
Quoted 12 lines
Quoted 9 lines
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.

Quoted 7 lines

Quoted 3 lines
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

Shivani Bhardwaj

author

2015-10-18 13:14 UTC #9 lore raw

Quoted 30 lines · Arnd Bergmann

Quoted 29 lines · On Sunday 18 October 2015 18:32:06 Shivani Bhardwaj wrote:
Quoted 27 lines · Arnd Bergmann
Quoted 5 lines · 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.
Quoted 10 lines · Shivani Bhardwaj
Quoted 9 lines · Arnd Bergmann
Quoted 8 lines
Quoted 8 lines
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;
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
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

Quoted 7 lines

Quoted 3 lines
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.