Hi Richard, Richard Genoud <richard.gen...@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.gen...@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.gen...@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
