Received-SPF: neutral (google.com: 212.227.126.131 is neither permitted nor denied by best guess record for domain of arnd@arndb.de) client-ip=212.227.126.131;
From: Arnd Bergmann <arnd@arndb.de>
To: outreachy-kernel@googlegroups.com
Cc: Shivani Bhardwaj <shivanib134@gmail.com>
Subject: Re: [Outreachy kernel] [PATCH v2] Staging: lustre: lpoc_osc: Change the variable to be checked
Date: Fri, 16 Oct 2015 11:55:36 +0200
Message-ID: <9667273.8SQsnSMdnh@wuerfel>
User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; )
In-Reply-To: <CAKHNQQFXZ2rqkbig2hwPSd=nHg-OZt3c6JiaGAHao6mkXPsUQQ@mail.gmail.com>
References: <20151016083218.GA2815@ubuntu> <4275207.TTHHeQE8pW@wuerfel> <CAKHNQQFXZ2rqkbig2hwPSd=nHg-OZt3c6JiaGAHao6mkXPsUQQ@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

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