Re: [PATCH 2/2] staging: iio_simple_dummy: zero check param

[Vladimirs Ambrosovs <rodriguez.twister@gmail.com>]

On Thu, May 28, 2015 at 09:59:34AM +0300, Dan Carpenter wrote:
> On Thu, May 28, 2015 at 01:12:40AM +0300, Vladimirs Ambrosovs wrote:
> > On Wed, May 27, 2015 at 11:25:07AM +0300, Dan Carpenter wrote:
> > > On Wed, May 27, 2015 at 01:19:58AM +0300, Vladimirs Ambrosovs wrote:
> > > > Check for zero was added to the module parameter "instances" to
> > > > avoid the allocation of array of zero values. Although it is a valid call,
> > > > we don't want to allocate ZERO_SIZE_PTR, so need to disallow this case.
> > > > The type of variables which are compared to "instances" were also changed
> > > > to unsigned int so that no compiler complaints occur.
> > > 
> > > Which compiler is that?
> > > 
> > > You should get a different compiler if you compiler complains about
> > > stupid stuff like that.  Making everything unsigned int is a common
> > > cause of problems.  I fixed or reported several of those bugs yesterday.
> > > 
> > > "instances" should be unsigned int, though, you're correct about that.
> > > 
> > Mine is fine - not complaining ;). 
> > 
> > Got your point, although, in some cases, I think, these warnings not a
> > stupid stuff, and could get some junior out of trouble.
> > 
> > But anyway, will keep in mind to stay away from unsigned ints. 
> 
> It's not a matter of staying away from unsigned ints, it's that some
> people make everything unsigned by default.  That causes problems for
> two reasons.  1) The kernel uses negative error codes.  2) int is the
> default datatype when you want a "number" in C.  If you want a special
> number then you make it unsigned int, u32, or unsigned long or whatever.
> All those types mean something.  An unsigned int and a u32 are the same
> to a computer but to a human they mean something different.  People who
> use complicated datatypes all the time instead of just plain old int are
> making the code complicated.
> 
> 
Thanks for the explanation.

> > > > 
> > > > Signed-off-by: Vladimirs Ambrosovs <rodriguez.twister@gmail.com>
> > > > ---
> > > >  drivers/staging/iio/iio_simple_dummy.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> > > > index 88fbb4f..2744a1b 100644
> > > > --- a/drivers/staging/iio/iio_simple_dummy.c
> > > > +++ b/drivers/staging/iio/iio_simple_dummy.c
> > > > @@ -30,7 +30,7 @@
> > > >   * dummy devices are registered.
> > > >   */
> > > >  static unsigned instances = 1;
> > > > -module_param(instances, int, 0);
> > > > +module_param(instances, uint, 0);
> > > >  
> > > >  /* Pointer array used to fake bus elements */
> > > >  static struct iio_dev **iio_dummy_devs;
> > > > @@ -706,9 +706,10 @@ static void iio_dummy_remove(int index)
> > > >   */
> > > >  static __init int iio_dummy_init(void)
> > > >  {
> > > > -	int i, ret;
> > > > +	unsigned int i;
> > > > +	int ret;
> > > 
> > > No.
> > > 
> > > >  
> > > > -	if (instances > 10) {
> > > > +	if (instances == 0 || instances > 10) {
> > > >  		instances = 1;
> > > >  		return -EINVAL;
> > > 
> > > Allocating zero size arrays is a totally valid thing the kernel and it
> > > doesn't cause a problem unless there are other existing serious bugs in
> > > the code.  In this case instances == 0 is fine.
> > > 
> > Sorry, got a bit confused - is it fine to be in the code, or the 0
> > value is valid, and shouldn't be checked for? The idea behind this
> > change was not the allocation of zero size array, but the
> > use of the module with 0 instances.
> 
> The changelog specifically mentioned a ZERO_SIZE_ARRAY.  If you had
> said, "It doesn't make sense to load a module with 0 instances" then I
> would have allowed the patch.  I don't care if you make this change or
> not, but the changelog had wrong motivation.
> 
Fair enough.

> regards,
> dan carpenter

BR,
Vladimirs