[PATCH] Staging: lustre: lproc_osc: Add check on a variable

[PATCH] Staging: lustre: lproc_osc: Add check on a variable

[Shivani Bhardwaj]

Variable rc is not tested for negative values and hence a check should
be included. Also, a check for variable val should be introduced.

Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
---
 drivers/staging/lustre/lustre/osc/lproc_osc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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

Re: [Outreachy kernel] [PATCH] Staging: lustre: lproc_osc: Add check on a variable

[Julia Lawall]

On Sun, 18 Oct 2015, Shivani Bhardwaj wrote:

> Variable rc is not tested for negative values and hence a check should
> be included. Also, a check for variable val should be introduced.
>
> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> ---
>  drivers/staging/lustre/lustre/osc/lproc_osc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c
> index cdc7f88..c4d44e7 100644
> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
> @@ -61,7 +61,9 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
>  	unsigned long val;
>
>  	rc = kstrtoul(buffer, 10, &val);
> -	if (rc < 0)
> +	if (rc)

If rc is never > 0, then why do you want to change rc < 0 to just rc?

julia

> +		return rc;
> +	if (val > 1)
>  		return -ERANGE;
>
>  	/* opposite senses */
> --
> 2.1.0
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20151018132030.GA30680%40ubuntu.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] Staging: lustre: lproc_osc: Add check on a variable

[Shivani Bhardwaj]

On Sun, Oct 18, 2015 at 6:54 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Sun, 18 Oct 2015, Shivani Bhardwaj wrote:
>
>> Variable rc is not tested for negative values and hence a check should
>> be included. Also, a check for variable val should be introduced.
>>
>> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
>> ---
>>  drivers/staging/lustre/lustre/osc/lproc_osc.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> index cdc7f88..c4d44e7 100644
>> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> @@ -61,7 +61,9 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
>>       unsigned long val;
>>
>>       rc = kstrtoul(buffer, 10, &val);
>> -     if (rc < 0)
>> +     if (rc)
>
> If rc is never > 0, then why do you want to change rc < 0 to just rc?
>
> julia
>

Because in case rc=0, it will be success and we don't need to return.
This patch is a fix up on earlier patch as suggested by Greg.

>> +             return rc;
>> +     if (val > 1)
>>               return -ERANGE;
>>
>>       /* opposite senses */
>> --
>> 2.1.0
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20151018132030.GA30680%40ubuntu.
>> For more options, visit https://groups.google.com/d/optout.
>>

Re: [Outreachy kernel] [PATCH] Staging: lustre: lproc_osc: Add check on a variable

[Julia Lawall]

On Sun, 18 Oct 2015, Shivani Bhardwaj wrote:

> On Sun, Oct 18, 2015 at 6:54 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Sun, 18 Oct 2015, Shivani Bhardwaj wrote:
> >
> >> Variable rc is not tested for negative values and hence a check should
> >> be included. Also, a check for variable val should be introduced.
> >>
> >> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> >> ---
> >>  drivers/staging/lustre/lustre/osc/lproc_osc.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c
> >> index cdc7f88..c4d44e7 100644
> >> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
> >> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
> >> @@ -61,7 +61,9 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
> >>       unsigned long val;
> >>
> >>       rc = kstrtoul(buffer, 10, &val);
> >> -     if (rc < 0)
> >> +     if (rc)
> >
> > If rc is never > 0, then why do you want to change rc < 0 to just rc?
> >
> > julia
> >
>
> Because in case rc=0, it will be success and we don't need to return.
> This patch is a fix up on earlier patch as suggested by Greg.

I don't follow.  When there was rc < 0, there would also not be a return.
There are three cases: rc < 0, rc = 0, rc > 0.  If you put rc < 0 you will
get a return on rc < 0 only.  And if you put rc you will get a return on
rc < 0 and rc > 0, but rc is never > 0 (you have said - I don't check), so
with rc you will also only get a return when rc < 0.  Putting rc is a bit
more concise.  But putting rc < 0 makes it more clear that what is going
on is an error check.  And I guess it is the code that was there
previously, so it is what the developer preferred,

julia

>
> >> +             return rc;
> >> +     if (val > 1)
> >>               return -ERANGE;
> >>
> >>       /* opposite senses */
> >> --
> >> 2.1.0
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> >> To post to this group, send email to outreachy-kernel@googlegroups.com.
> >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20151018132030.GA30680%40ubuntu.
> >> For more options, visit https://groups.google.com/d/optout.
> >>
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAKHNQQEJLf1LXOZr7KS%3DBdC8riOXdjg6jgfDMfV8OK2uXvpJww%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] Staging: lustre: lproc_osc: Add check on a variable

[Shivani Bhardwaj]

On Sun, Oct 18, 2015 at 7:02 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Sun, 18 Oct 2015, Shivani Bhardwaj wrote:
>
>> On Sun, Oct 18, 2015 at 6:54 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> > On Sun, 18 Oct 2015, Shivani Bhardwaj wrote:
>> >
>> >> Variable rc is not tested for negative values and hence a check should
>> >> be included. Also, a check for variable val should be introduced.
>> >>
>> >> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
>> >> ---
>> >>  drivers/staging/lustre/lustre/osc/lproc_osc.c | 4 +++-
>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/staging/lustre/lustre/osc/lproc_osc.c b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> >> index cdc7f88..c4d44e7 100644
>> >> --- a/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> >> +++ b/drivers/staging/lustre/lustre/osc/lproc_osc.c
>> >> @@ -61,7 +61,9 @@ static ssize_t active_store(struct kobject *kobj, struct attribute *attr,
>> >>       unsigned long val;
>> >>
>> >>       rc = kstrtoul(buffer, 10, &val);
>> >> -     if (rc < 0)
>> >> +     if (rc)
>> >
>> > If rc is never > 0, then why do you want to change rc < 0 to just rc?
>> >
>> > julia
>> >
>>
>> Because in case rc=0, it will be success and we don't need to return.
>> This patch is a fix up on earlier patch as suggested by Greg.
>
> I don't follow.  When there was rc < 0, there would also not be a return.
> There are three cases: rc < 0, rc = 0, rc > 0.  If you put rc < 0 you will
> get a return on rc < 0 only.  And if you put rc you will get a return on
> rc < 0 and rc > 0, but rc is never > 0 (you have said - I don't check), so
> with rc you will also only get a return when rc < 0.  Putting rc is a bit
> more concise.  But putting rc < 0 makes it more clear that what is going
> on is an error check.  And I guess it is the code that was there
> previously, so it is what the developer preferred,
>
> julia
>

What you say is perfectly fine. But, earlier code mentioned 'if (rc)'
only [developer version]. I changed that to 'if(rc<0)' along with some
other changes that I got wrong. I've been asked to provide a fix up.
If you say, I can change it to rc<0. Yes, it would be more precise.
I've consulted Arnd about the positive values. It is never going to
happen. Please suggest?

>>
>> >> +             return rc;
>> >> +     if (val > 1)
>> >>               return -ERANGE;
>> >>
>> >>       /* opposite senses */
>> >> --
>> >> 2.1.0
>> >>
>> >> --
>> >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> >> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20151018132030.GA30680%40ubuntu.
>> >> For more options, visit https://groups.google.com/d/optout.
>> >>
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/CAKHNQQEJLf1LXOZr7KS%3DBdC8riOXdjg6jgfDMfV8OK2uXvpJww%40mail.gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
>>

Re: [Outreachy kernel] [PATCH] Staging: lustre: lproc_osc: Add check on a variable

[Julia Lawall]

> > I don't follow.  When there was rc < 0, there would also not be a return.
> > There are three cases: rc < 0, rc = 0, rc > 0.  If you put rc < 0 you will
> > get a return on rc < 0 only.  And if you put rc you will get a return on
> > rc < 0 and rc > 0, but rc is never > 0 (you have said - I don't check), so
> > with rc you will also only get a return when rc < 0.  Putting rc is a bit
> > more concise.  But putting rc < 0 makes it more clear that what is going
> > on is an error check.  And I guess it is the code that was there
> > previously, so it is what the developer preferred,
> >
> > julia
> >
>
> What you say is perfectly fine. But, earlier code mentioned 'if (rc)'
> only [developer version]. I changed that to 'if(rc<0)' along with some
> other changes that I got wrong. I've been asked to provide a fix up.
> If you say, I can change it to rc<0. Yes, it would be more precise.
> I've consulted Arnd about the positive values. It is never going to
> happen. Please suggest?

OK, thanks for the explanation.  If the code was originally if (rc), it
would probably be reasonable to leave it as is.  I was thrown off by the
fact that there was a - on rc < 0 in the proposed patch.  Did Greg apply
an incorrect one that you now have to fix up?

julia

Re: [Outreachy kernel] [PATCH] Staging: lustre: lproc_osc: Add check on a variable

[Shivani Bhardwaj]

On Sun, Oct 18, 2015 at 7:49 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> > I don't follow.  When there was rc < 0, there would also not be a return.
>> > There are three cases: rc < 0, rc = 0, rc > 0.  If you put rc < 0 you will
>> > get a return on rc < 0 only.  And if you put rc you will get a return on
>> > rc < 0 and rc > 0, but rc is never > 0 (you have said - I don't check), so
>> > with rc you will also only get a return when rc < 0.  Putting rc is a bit
>> > more concise.  But putting rc < 0 makes it more clear that what is going
>> > on is an error check.  And I guess it is the code that was there
>> > previously, so it is what the developer preferred,
>> >
>> > julia
>> >
>>
>> What you say is perfectly fine. But, earlier code mentioned 'if (rc)'
>> only [developer version]. I changed that to 'if(rc<0)' along with some
>> other changes that I got wrong. I've been asked to provide a fix up.
>> If you say, I can change it to rc<0. Yes, it would be more precise.
>> I've consulted Arnd about the positive values. It is never going to
>> happen. Please suggest?
>
> OK, thanks for the explanation.  If the code was originally if (rc), it
> would probably be reasonable to leave it as is.  I was thrown off by the
> fact that there was a - on rc < 0 in the proposed patch.  Did Greg apply
> an incorrect one that you now have to fix up?
>
> julia

Yes, that's what exactly happened.