On Thu, Oct 19, 2017 at 04:16:37PM +0200, Michal Suchanek wrote:

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

quoted

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.

This is silly...  Anyway, my record is there in git.  I mostly send
bugfixes, and not cleanups.  In terms of patches that are merged, I
probably have introduced one or two runtime bugs per year over the past
decade.

quoted

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.

No.  Just always do it standard way.  If there is only one error
condition just free it directly:

	if (invalid) {
		free(foo);
		return -EINVAL;
	}

But once you add a second error condition then use gotos.  There is no
reason to deviate from the standard.

If it is more complex more labels are preferable.

quoted

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.

No, in my example, the new allocation was added to the *start* not the
end.  It doesn't matter though, standard error handling works the same
either way.

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.

No, You don't need to re-order the labels or change them.  The label
name says what is freed.  The order is the reverse order from how they
were allocated.  It's very simple.  You just have to keep track of the
most recently allocated thing:

	foo = alloc();
	if (!foo)
		return -ENOMEM;

	bar = alloc();
	if (!bar)
		goto free_foo;

	baz = alloc();
	if (!baz)
		goto free_bar;

You can tell this code doesn't leak just from looking at the label
names.

regards,
dan carpenter