On Thu, 19 Oct 2017 16:36:46 +0300
Dan Carpenter [off-list ref] wrote:

On Thu, Oct 19, 2017 at 01:56:32PM +0200, Michal Suchanek wrote:

quoted

I think a single cleanup section is better than many labels that
just avoid a single null check.
 

I am not a big advocate of churn, but one err style error handling is
really bug prone.

I'm dealing with static analysis so most of the bugs I see are error
handling bugs.  

But it looks like you do not deal much with actual code development
because then you would know that some of the changes proposed by the
static analysis lead to errors later when the code dynamically changes.

That's because error handling is hard to test but easy
for static analysis.  One err style error handling is the worst
because you get things like:

fail:
	kfree(foo->bar);
	kfree(foo);

Oops, foo->bar is a NULL dereference.  And generally, it's a bad thing
to free things that haven't been allocated so, for example, I see
refcounting bugs in error handling paths as well where we decrement
something that wasn't incremented.  Freeing everything is more
complicated than just freeing one specific thing the way standard
kernel error handling works.

It depends on the function in question. If it only allocates memory
which is not reference-counted and kfree() checks for the null in most
cases it is easier to do just one big cleanup. 

If it is more complex more labels are preferable.

quoted

As long as you can tell easily which resources were already
allocated and need to be freed it is saner to keep only one cleanup
section. 

Sure, if the function is simple and short then the error handling is
normally simple and short.  This is true for any style of error
handling.

quoted

If the code doing the allocation is changed in the future the single
cleanup can stay whereas multiple labels have to be rewritten
again.  

No, they don't unless you choose bad label names.  

You obviously miss the fact that resource allocation is not always
added at the end of the function.

You may need to reorder the code and hence the order of allocation or
add allocation at the start. Both of these cases break multiple labels
and special cases but work with one big cleanup just fine.

Thanks

Michal