[PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix memory leak

[Miquel Raynal]

Hi Richard,

Richard Genoud <richard.genoud at posteo.net> wrote on Thu, 15 Oct 2020
18:29:45 +0200:

> Hi Miquel !
> Thanks for your feedback.
> 
> Le 15/10/2020 à 15:54, Miquel Raynal a écrit :
> > Hi Richard,
> > 
> > Richard Genoud <richard.genoud at posteo.net> wrote on Wed, 14 Oct 2020
> > 10:06:11 +0200:
> >   
> >> pos_list wasn't freed on every error
> >>
> >> Signed-off-by: Richard Genoud <richard.genoud at posteo.net>  
> > 
> > Same comment here (and probably after as well) as in patch 05/17, not
> > sure this is actually relevant for the community but I prefer this:
> > 
> > 	bar = malloc();
> > 	...
> > 	if (ret)
> > 		goto free_bar;
> > 
> > 	foo = malloc();
> > 	...
> > 	if (ret)
> > 		goto free foo;
> > 
> > 	...
> > 
> > 	foo:
> > 		kfree(foo);
> > 	bar:
> > 		kfree(bar);
> > 
> > than:
> > 
> > 	foo = NULL;
> > 	bar = NULL;
> > 
> > 	...
> > 	if (ret)
> > 		goto out;
> > 	...
> > 	if (ret)
> > 		goto out;
> > 	...
> > 		out:
> > 	if (ret)
> > 		kfree(...)  
> 
> I guess it's a coding habit.
> I personnaly prefer the later because I think it's less error-prone :
> When moving code aroung, we don't have to move the labels and rename
> the gotos.
> Ex:
> Let's say we have this code:
> 	bar = malloc();
> 	...
> 	if (ret)
> 		goto free_bar;
> 
> 	foo = malloc();
> 	...
> 	if (ret)
> 		goto free_foo;
> 	ret = init_somthing();
> 	if (ret)
> 		goto free_foo;
> 	ret = dummy()
> 	if (ret)
> 		goto free_foo;
> 
> 	...
> 
> 	foo:
> 		kfree(foo);
> 	bar:
> 		kfree(bar);
> 
> And, we want to move, for whatever reason, init_something() and dummy()
> before the foo allocation. We will have to change the code to:
> 
> 	bar = malloc();
> 	...
> 	if (ret)
> 		goto free_bar;
> 	ret = init_somthing();
> 	if (ret)
> 		goto free_bar; // not free_foo anymore !
> 	ret = dummy()
> 	if (ret)
> 		goto free_bar; // ditto
> 
> 	foo = malloc();
> 	...
> 	if (ret)
> 		goto free_foo;
> 	...
> 
> 	foo:
> 		kfree(foo);
> 	bar:
> 		kfree(bar);
> 
> Worse, if we have to exchange bar and foo allocation, we'll also have
> to exchange the deallocation of foo and bar and change all gotos beneath :
> 	foo = malloc();
> 	...
> 	if (ret)
> 		goto free_foo;
> 
> 	bar = malloc();
> 	...
> 	if (ret)
> 		goto free_bar;
> 
> 	ret = init_somthing();
> 	if (ret)
> 		goto free_foo; // not free_foo anymore
> 	ret = dummy()
> 	if (ret)
> 		goto free_foo; //ditto
> 
> 
> 	...
> 
> // oops ! we have to exchange that !
> 	foo:
> 		kfree(foo);
> 	bar:
> 		kfree(bar);
> 
> 
> That's why I prefer only one label and setting NULL.
> If I didn't convince you, I'll change it back to multiple labels :)

You are right it involves less changes when editing the code. But
on the other hand it is so often written like [my proposal], that it
almost becomes a coding style choice I guess. Anyway, I don't have a
strong opinion on this so I'll let you choose the best approach from
your point of view, unless you get other comments sharing my thoughts.

Thanks anyway for the cleanup :)

Cheers,
Miquèl