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@xxxxxxxxx>
> > > > ---
> > > > 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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
