From mboxrd@z Thu Jan  1 00:00:00 1970
From: Miquel Raynal <miquel.raynal@bootlin.com>
Date: Thu, 15 Oct 2020 18:38:51 +0200
Subject: [PATCH 06/17] fs/squashfs: sqfs_read_directory_table: fix
 memory leak
In-Reply-To: <6d4eecef-5d8f-37a2-a1df-1103aaab52d5@posteo.net>
References: <20201014080622.14970-1-richard.genoud@posteo.net>
 <20201014080622.14970-7-richard.genoud@posteo.net>
 <20201015155412.2830559d@xps13>
 <6d4eecef-5d8f-37a2-a1df-1103aaab52d5@posteo.net>
Message-ID: <20201015183851.01e8082f@xps13>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Richard,

Richard Genoud <richard.genoud@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@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@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