On Tue, 2008-11-18 at 08:19 +0100, Henning Rogge wrote:
> Am Tuesday 18 November 2008 04:34:20 schrieb Alina Friedrichsen:
> > I think "goto" is the cleanest way to make a destruktor for a function in
> > C. See the Linux kernel.
> "goto" and "clean" in the same sentence ? There is something wrong with 
> this... ^^

Now that you asked;-)
Not to 100%. And yes, the students in mine 1st year (read: officially
they are beginners and some/most of them really are) operating system
lecture are also explicitly not allowed to use "goto".

For the advanced programmers:
It is somewhat awkward and quite error prone to have a (user-space)
function (without any "goto" and/or labels) like
----  snip  ----
int x(...)
{
[...]
	x = malloc(a);
[...]
	if(some_error) {
		free(x);
		return -1;
	}
[...]
	y = malloc(b);
[...]
	if(some_error) {
		free(x);
		free(y);
		return -1;
	}
[...]
	z = malloc(c);
[...]
	if(some_error) {
		free(x);
		free(y);
		free(z);
		return -1;
	}
[...]
	free(x);
	free(y);
	free(z);
	return 0;
}
----  snip  ----
And it's not only "malloc()" as in the above example which wants to be
paired with "free()" but could be "open()" and "close()", "socket()" and
"close()", etc.
To keep it simpler, I just used "malloc()".
If the "malloc()"ed data should survive the end of the function, you
remove the final "free()" block

For a concrete example: E.g. ipc_init() (in src/ipc_frontend.c) has 3-4
such exit paths - not only 3 as above - with exact the same situation.
So the example of 3 is more the lower limit .....
BTW no, I won't rewrite that everywhere.

One alternative version of the above using the (presumbly evil) "goto"
is
----  snip  ----
int x(...)
{
	int rv = -1;
[...]
	x = malloc(a);
[...]
	if(some_error) {
		goto freex_and_out;
	}
[...]
	y = malloc(b);
[...]
	if(some_error) {
		goto freexy_and_out;
	}
[...]
	z = malloc(c);
[...]
	if(some_error) {
		goto freexyz_and_out;
	}
[...]
	rv = 0;
freexyz_and_out:
	free(z);
freexy_and_out:
	free(y);
freex_and_out:
	free(x);
	return rv;
}
----  snip  ----
If the "malloc()"ed data should survive the end of the function, you
s/rv = 0/return 0/, s/return rv/return -1/ and kill the definition of
"rv".
Please note that we have only forward jumps (and no backward jumps - so
no great danger of spaghetti code).

The advantages:
- Less code a run-time reduces the stress on the i-cache (and may be
  irrelevant in user-space, in the kernel it is not that simple).
But most important
- It's much less error prone as one doesn't have to "copy" the cleanup
  logic into various exit paths and have it correct.
  And the problem is not the initial development but the maintenance
  over time where the code is changed, enhanced (if you need an
  additional "malloc()", etc.
  grep in the hg/CVS log and grep for "leak" - I fixed several file
  descriptor leaks (Ok, they happened just in obscure error cases like
  "bind() fails").

The above can easily be rewritten with cascaded if()s and have only one
"free()" per "malloc()".
But either you get a lot of indentation (and remember: the Linux kernel
has 8 chars/1 tab per level and having too many indentation levels is
also not really liked) which makes the code quite unreadable or
you introduce additional functions (to avoid the "too many indentation
levels" problem) just separates code which simply belong together and
reduces readability also (for no really good use except to follow some
formal "law" on coding style) - and also remember that source code
*much* more often read (so it needs to be understood) than written or
changed.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services




