Re: [Outreachy kernel] [PATCH] staging: greybus: loop variable to loop index

[Johan Hovold <johan@kernel.org>]

On Tue, Feb 21, 2017 at 06:31:28PM +0100, Julia Lawall wrote:
> On Tue, 21 Feb 2017, Gargi Sharma wrote:
> 
> > Change array index from the loop bound variable to loop index.
> > Once all the loopback devices have been found, free the pointer
> > array containing the dirent structures. The pointer to dirent
> > structures must be individually freed before freeing the pointer
> > array.
> >
> > Coccinelle Script:
> > @@
> > expression arr,ex1,ex2;
> > @@
> >
> > for(ex1 = 0; ex1 < ex2; ex1++) { <...
> >   arr[
> > - ex2
> > + ex1
> >   ]
> >   ...> }

Another good catch, but you should change the patch summary to something
more descriptive (and less detailed) like:

	staging: greybus: loopback_test: fix device-name leak

and possibly also make it more clear from the commit message that we've
been leaking these strings whenever there have been more than one
loopback device. (The memory is returned once the test program exits,
but still).

Care to send a v2 with those changes?

> > Signed-off-by: Gargi Sharma <gs051095@gmail.com>
> > ---
> >  drivers/staging/greybus/tools/loopback_test.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/greybus/tools/loopback_test.c b/drivers/staging/greybus/tools/loopback_test.c
> > index 18d7a3d..1c01833 100644
> > --- a/drivers/staging/greybus/tools/loopback_test.c
> > +++ b/drivers/staging/greybus/tools/loopback_test.c
> > @@ -636,7 +636,7 @@ int find_loopback_devices(struct loopback_test *t)
> >  	ret = 0;
> >  done:
> >  	for (i = 0; i < n; i++)
> > -		free(namelist[n]);
> > +		free(namelist[i]);
> 
> I guess this is a test, so no one ever tried running it :)

It is a test tool, but we've been using it quite a bit.

Thanks,
Johan