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.
> >
> > 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. However, maybe that actually
addresses some usecase, so probably would be better to leave it as it
was before.
> Setting "instances = 1" is bogus though.
>
> > }
> > @@ -742,7 +743,7 @@ module_init(iio_dummy_init);
> > */
> > static __exit void iio_dummy_exit(void)
> > {
> > - int i;
> > + unsigned int i;
>
> No.
>
> regards,
> dan carpenter
>
Thanks for all the comments. Will send out the updated patches soon.
BR,
Vladimirs
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
