Cleanup patterns

[MQ]

Hi all

I am just wondering how most people implement cleanup in C functions.
In particular, if the function opens a number of resources, these need
to be released properly should an error occur at any point in the
function (as well as at the end if successful). C++ has exceptions,
the only way I can see to do this neatly in C is to use goto
statements. Is my method of implementing cleanup good, or are their
better ways. Here is an example, which uses the global errno to store
the error.

#define CLEANUP(err) ({errno = (err); goto cleanup})

int example_functio n()
{
SOMETYPE * a,b,c;
errno = 0;

if(!(a = malloc(sizeof(a ))))
CLEANUP(ENOMEM) ;

if(!(b = malloc(sizeof(b ))))
CLEANUP(ENOMEM) ;

if(!(c = malloc(sizeof(c ))))
CLEANUP(ENOMEM) ;

/* do something here */

cleanup:
if(a)
free(a);
if(b);
free(b);
if(c)
free(c);
if(errno)
return -1;
return 0;
}

[Tom St Denis]

Re: Cleanup patterns


MQ wrote:

Hi all
>
I am just wondering how most people implement cleanup in C functions.
In particular, if the function opens a number of resources, these need
to be released properly should an error occur at any point in the
function (as well as at the end if successful). C++ has exceptions,
the only way I can see to do this neatly in C is to use goto
statements. Is my method of implementing cleanup good, or are their
better ways. Here is an example, which uses the global errno to store
the error.
>
#define CLEANUP(err) ({errno = (err); goto cleanup})

Don't use macros with jumps and what not.

int example_functio n()
{
SOMETYPE * a,b,c;

b and c are not pointers [or at least not at the same level as a]

errno = 0;
>
if(!(a = malloc(sizeof(a ))))
CLEANUP(ENOMEM) ;
>
if(!(b = malloc(sizeof(b ))))
CLEANUP(ENOMEM) ;
>
if(!(c = malloc(sizeof(c ))))
CLEANUP(ENOMEM) ;

I'd do simply

a = malloc(sizeof(* a));
if (a == NULL) { errno = outofmem; goto ERR_A; }
b = ...
if (b == NULL) { errno = outofmem; goto ERR_B; }
....

Then at the end have

errno = 0;

ERR_C:
free(b);
ERR_B:
free(a);
ERR_A:
return errno;
}

Makes the code easy to follow and doesn't involve nasty if's all over
the place (e.g. if you avoided goto all together..)

Tom

[Eric Sosman]

Re: Cleanup patterns

MQ wrote:

Hi all
>
I am just wondering how most people implement cleanup in C functions.
In particular, if the function opens a number of resources, these need
to be released properly should an error occur at any point in the
function (as well as at the end if successful). C++ has exceptions,
the only way I can see to do this neatly in C is to use goto
statements.

In C, it's typical to use nested if's, and/or carefully
initialized variables, and/or decomposition into multiple
functions, and/or goto.

Is my method of implementing cleanup good, or are their
better ways. Here is an example, which uses the global errno to store
the error.

Your method is Bad(tm). For starters, it won't compile.
That could be fixed by changing the macro definition, but then
you'd be invoking undefined behavior if either of the first two
malloc() calls failed. Also, Standard C doesn't define ENOMEM,
and `errno!=0' is not a valid test for failure.

All of these problems are easily fixed or worked around, but
consider: You've advanced CLEANUP as, essentially, a way to help
write better programs. Since it demonstrably has *not* helped
you to write a better program, perhaps it's time to question its
effectiveness, no?

#define CLEANUP(err) ({errno = (err); goto cleanup})
>
int example_functio n()
{
SOMETYPE * a,b,c;
errno = 0;
>
if(!(a = malloc(sizeof(a ))))
CLEANUP(ENOMEM) ;
>
if(!(b = malloc(sizeof(b ))))
CLEANUP(ENOMEM) ;
>
if(!(c = malloc(sizeof(c ))))
CLEANUP(ENOMEM) ;
>
/* do something here */
>
cleanup:
if(a)
free(a);
if(b);
free(b);
if(c)
free(c);
if(errno)
return -1;
return 0;
}

--
Eric Sosman
esosman@acm-dot-org.invalid

[MQ]

Re: Cleanup patterns


Eric Sosman wrote:

MQ wrote:
>

Hi all

I am just wondering how most people implement cleanup in C functions.
In particular, if the function opens a number of resources, these need
to be released properly should an error occur at any point in the
function (as well as at the end if successful). C++ has exceptions,
the only way I can see to do this neatly in C is to use goto
statements.

>
In C, it's typical to use nested if's, and/or carefully
initialized variables, and/or decomposition into multiple
functions, and/or goto.
>

Is my method of implementing cleanup good, or are their
better ways. Here is an example, which uses the global errno to store
the error.

>
Your method is Bad(tm). For starters, it won't compile.
That could be fixed by changing the macro definition, but then
you'd be invoking undefined behavior if either of the first two
malloc() calls failed. Also, Standard C doesn't define ENOMEM,
and `errno!=0' is not a valid test for failure.

Hi Eric

I did throw this together rather quickly and no doubt there will be a
number of errors, but after posting I fixed these problems and it does
compile fine. Ignoring this, I just want to know if the general
structure of the code is good practise. You should be able to
ascertain what I am trying to achieve...


MQ.

[Eric Sosman]

Re: Cleanup patterns

MQ wrote:

Eric Sosman wrote:
>

>>MQ wrote:
>>
>>

>>>Hi all
>>>
>>>I am just wondering how most people implement cleanup in C functions.
>>>In particular, if the function opens a number of resources, these need
>>>to be released properly should an error occur at any point in the
>>>function (as well as at the end if successful). C++ has exceptions,
>>>the only way I can see to do this neatly in C is to use goto
>>>statements .

>>
> In C, it's typical to use nested if's, and/or carefully
>>initialized variables, and/or decomposition into multiple
>>functions, and/or goto.
>>
>>

>>>Is my method of implementing cleanup good, or are their
>>>better ways. Here is an example, which uses the global errno to store
>>>the error.

>>
> Your method is Bad(tm). For starters, it won't compile.
>>That could be fixed by changing the macro definition, but then
>>you'd be invoking undefined behavior if either of the first two
>>malloc() calls failed. Also, Standard C doesn't define ENOMEM,
>>and `errno!=0' is not a valid test for failure.

>
>
Hi Eric
>
I did throw this together rather quickly and no doubt there will be a
number of errors, but after posting I fixed these problems and it does
compile fine. Ignoring this, I just want to know if the general
structure of the code is good practise. You should be able to
ascertain what I am trying to achieve...

It appears you are trying to achieve what I described as
typical: "nested if's, and/or carefully initialized variables,
and/or decomposition into multiple functions, and/or goto."
My original question remains: Given that the attempt led you
into four different errors (and perhaps more in the "fixed"
version we haven't seen yet), do you still think you're going
about things in the best way possible? Note that "it does
compile fine" and "it is correct" are not the same thing!

--
Eric Sosman
esosman@acm-dot-org.invalid

[CBFalconer]

Re: Cleanup patterns

MQ wrote:

>
I am just wondering how most people implement cleanup in C
functions. In particular, if the function opens a number of
resources, these need to be released properly should an error
occur at any point in the function (as well as at the end if
successful). C++ has exceptions, the only way I can see to do
this neatly in C is to use goto statements. Is my method of
implementing cleanup good, or are their better ways. Here is an
example, which uses the global errno to store the error.
>
#define CLEANUP(err) ({errno = (err); goto cleanup})
>
int example_functio n()
{
SOMETYPE * a,b,c;
errno = 0;
>
if(!(a = malloc(sizeof(a ))))
CLEANUP(ENOMEM) ;
>
if(!(b = malloc(sizeof(b ))))
CLEANUP(ENOMEM) ;
>
if(!(c = malloc(sizeof(c ))))
CLEANUP(ENOMEM) ;
>
/* do something here */
>
cleanup:
if(a)
free(a);
if(b);
free(b);
if(c)
free(c);
if(errno)
return -1;
return 0;
}

I would implement that as:

int example(void)
{
SOMETYPE *a, *b, *c;

errno = 0;
a = b = c = NULL;
if (!(a = malloc(sizeof *a))) errno = ENOMEM;
else if (!(b = malloc(sizeof *b))) errno = ENOMEM;
else if (!(c = malloc(sizeof *c))) errno = ENOMEM
else {
/* dosomething here */
}
free(a); free(b); free(c);
return -(0 != errno);
}

Notice the absence of obfuscating macros. Also the routine no
longer frees undefined pointers or non-pointers. No goto needed,
although I do not hesitate to use them when appropriate. I would
normally choose different return values.

--
Chuck F (cbfalconer at maineline dot net)
Available for consulting/temporary embedded and systems.
<http://cbfalconer.home .att.net>

[MQ]

Re: Cleanup patterns

It appears you are trying to achieve what I described as
typical: "nested if's, and/or carefully initialized variables,
and/or decomposition into multiple functions, and/or goto."
My original question remains: Given that the attempt led you
into four different errors (and perhaps more in the "fixed"
version we haven't seen yet), do you still think you're going
about things in the best way possible?

Umm, well, that was my question originally. I am looking for feedback.
And the question is more generic than the example above. I want to
know *generically* what are the best methods for implementing cleanup
in C functions. I can say for sure that *something* needs to be done,
as a function that holds potentially numerous resources cannot clean up
at the point of error, otherwise most of the code will be cleanup
stuff, and obscure the real task that the code is doing.

MQ.

[Richard Heathfield]

Re: Cleanup patterns

MQ said:

Hi all
>
I am just wondering how most people implement cleanup in C functions.

I can't speak for most people, but I do it like this:

Attempt to acquire resource
If attempt succeeded
Use resource
Release resource
Else
Handle error
Endif

This code structure is recursively extensible, and functions are a fabulous
mechanism for keeping the indent level down.

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999

http://www.cpax.org.uk

email: rjh at the above domain, - www.

[Chris Dollin]

Re: Cleanup patterns

MQ wrote:

Hi all
>
I am just wondering how most people implement cleanup in C functions.
In particular, if the function opens a number of resources, these need
to be released properly should an error occur at any point in the
function (as well as at the end if successful). C++ has exceptions,
the only way I can see to do this neatly in C is to use goto
statements. Is my method of implementing cleanup good, or are their
better ways. Here is an example, which uses the global errno to store
the error.
>
#define CLEANUP(err) ({errno = (err); goto cleanup})
>
int example_functio n()
{
SOMETYPE * a,b,c;
errno = 0;
>
if(!(a = malloc(sizeof(a ))))
CLEANUP(ENOMEM) ;
>
if(!(b = malloc(sizeof(b ))))
CLEANUP(ENOMEM) ;
>
if(!(c = malloc(sizeof(c ))))
CLEANUP(ENOMEM) ;
>
/* do something here */
>
cleanup:
if(a)
free(a);
if(b);
free(b);
if(c)
free(c);
if(errno)
return -1;
return 0;
}

This one I'd write (fixing a couple of things along the way) as:

int exampleFunction ()
{
SomeType *a = malloc( sizeof (*a) );
SomeType *b = malloc( sizeof (*b) );
SomeType *c = malloc( sizeof (*c) );
int status = a && b && c ? 0 : -1;
if (status == 0)
{
/* do what must be done */
}
free( a ), free( b ), free( c );
return status;
}

I don't think your example is complex enough to demonstrate whatever
value your macro might have.

--
Chris "Magenta - the best colour of sound" Dollin
"No-one here is exactly what he appears." G'kar, /Babylon 5/

[Eric Sosman]

Re: Cleanup patterns

CBFalconer wrote:

MQ wrote:
>

>>I am just wondering how most people implement cleanup in C
>>functions. In particular, if the function opens a number of
>>resources, these need to be released properly should an error
>>occur at any point in the function (as well as at the end if
>>successful) . C++ has exceptions, the only way I can see to do
>>this neatly in C is to use goto statements. Is my method of
>>implementin g cleanup good, or are their better ways. Here is an
>>example, which uses the global errno to store the error.
>>
>>#define CLEANUP(err) ({errno = (err); goto cleanup})
>>
>>int example_functio n()
>>{
> SOMETYPE * a,b,c;
> errno = 0;
>>
> if(!(a = malloc(sizeof(a ))))
> CLEANUP(ENOMEM) ;
>>
> if(!(b = malloc(sizeof(b ))))
> CLEANUP(ENOMEM) ;
>>
> if(!(c = malloc(sizeof(c ))))
> CLEANUP(ENOMEM) ;
>>
> /* do something here */
>>
>>cleanup:
> if(a)
> free(a);
> if(b);
> free(b);
> if(c)
> free(c);
> if(errno)
> return -1;
> return 0;
>>}

>
>
I would implement that as:
>
int example(void)
{
SOMETYPE *a, *b, *c;
>
errno = 0;
a = b = c = NULL;
if (!(a = malloc(sizeof *a))) errno = ENOMEM;
else if (!(b = malloc(sizeof *b))) errno = ENOMEM;
else if (!(c = malloc(sizeof *c))) errno = ENOMEM

Missing a semicolon here, right after the possibly
undefined identifier ENOMEM.

else {
/* dosomething here */
}
free(a); free(b); free(c);
return -(0 != errno);

Note that free() is permitted to set errno to a non-zero
value. So might /* dosomething here */ if it were not a
comment.

errno is not a good holder for program state, because
it's "transient: " most library functions can change it at
will, whether or not any error has occurred. Park a value
in errno, and it may be "gone in sixty seconds."

--
Eric Sosman
esosman@acm-dot-org.invalid

[bert]

Re: Cleanup patterns

MQ wrote:

Hi all
>
I am just wondering how most people implement cleanup in C functions.
In particular, if the function opens a number of resources, these need
to be released properly should an error occur at any point in the
function (as well as at the end if successful). C++ has exceptions,
the only way I can see to do this neatly in C is to use goto
statements. Is my method of implementing cleanup good, or are their
better ways.

I went through several phases of this problem,
and ended up quite satisfied with the following
as a general scheme.

1. Define a filescope structure which can hold
all pointers, error indicators etcetera relevant
to the function.

2. In the function, declare an instance of the
structure; initialise it with NULLs; fill it up
as resource allocation proceeds; overwrite
fields in it with NULL after freeing any resource.

3. On a failure, exit with a statement such as:
return tidy_exit (this_error, alloc_struct);

It is a simple exercise to write the function
tidy_exit( ) so that it makes use of the data
in its second parameter as required, and
finally returns a copy of its first parameter.

There were some times when it seemed a bit
like overkill, but (a) it was arbitrarily extendable,
and (b) it kept the main function looking clean.
--

[Michael]

Re: Cleanup patterns

I am just wondering how most people implement cleanup in C functions.

In particular, if the function opens a number of resources, these need
to be released properly should an error occur at any point in the
function (as well as at the end if successful). C++ has exceptions,
the only way I can see to do this neatly in C is to use goto
statements. Is my method of implementing cleanup good, or are their
better ways. Here is an example, which uses the global errno to store
the error.
>
#define CLEANUP(err) ({errno = (err); goto cleanup})
>
int example_functio n()
{
SOMETYPE * a,b,c;
errno = 0;
>
if(!(a = malloc(sizeof(a ))))
CLEANUP(ENOMEM) ;
>
if(!(b = malloc(sizeof(b ))))
CLEANUP(ENOMEM) ;
>
if(!(c = malloc(sizeof(c ))))
CLEANUP(ENOMEM) ;
>
/* do something here */
>
cleanup:
if(a)
free(a);
if(b);
free(b);
if(c)
free(c);
if(errno)
return -1;
return 0;
}

I must be the only one who thinks this is good code. I once worked on
a project where we used a style like this and had orders of magnitude
fewer resource leaks than any other project our company had done using
if/then/else. (We were writing a server on Linux that had to process
tons of transactions and stay up for months, so even a small memory
leak would quickly add up and kill it.) The way we did it is a little
subtle, and requires programmer discipline. Basically, every function
that allocates resources must follow certain rules. The good thing is
that it's fairly easy to check whether a given function does, and if
not, to correct it.

Note: what follows is a rough idea, and may suffer the usual lack of
semicolons and stuff - details are left as an exercise to the reader.
In other words, treat the following as pseudo-code.

/* Rule 1: define a couple macros like this so you can use them like
statements
with semicolons, i.e., their syntax isn't too strange when you use
them (macro
is strange, though). We passed around an error message structure,
so our macros
were a little more complex than the version here with errno. */
#define GOTO_IF_BAD_ALL OC(mem, label) \
do {\
if (mem == NULL) { \
errno = ENOMEM; \
goto label;\
}\
while (0)

int example_functio n(SOMESTRUCT* s)
{
SOMETYPE *a;
SOMETYPE *b;
SOMETYPE *c;
RETURNTYPE r;

/* Rule 2: initialize everything to NULL, and the error to some
generic error condition */
a = NULL;
b = NULL;
c = NULL;
errno = UNDEFINED_ERROR ;
/* Note: if the function ever returns UNDEFINED_ERROR , that
indicates a logic error
in the function. (Actual errors, as well as successful
completion, should set the
error code to a real value.) */

/* Rule 3: whenever you allocate, use the macro. */
a = malloc(sizeof(a ));
GOTO_IF_BAD_ALL OC(a, cleanup);

b = malloc(sizeof(b ));
GOTO_IF_BAD_ALL OC(b, cleanup);

/* Rule 3b: check all function call returns too, with an analogous
macro. */
r = call_some_funct ion(a, b);
GOTO_IF_CALL_RE TURNS_ERROR(r, cleanup);
/* Depending how you do it, this macro might need an extra
parameter with error
code, string message, or both. */

c = malloc(sizeof(c ));
GOTO_IF_BAD_ALL OC(c, cleanup);
s->someField = c;
c = NULL; /* We no longer own it. */
/* Rule 4: when you pass ownership of a pointer, set it to NULL,
and add a comment
as above if there's even a question that it might be confusing
to some developer
in the future. */

/* do something here */

/* Rule 5: set error code to success right before the cleanup
routine. */
errno = SUCCESS;

/* Rule 6: have a global cleanup routine that all paths go through. */
cleanup:
/* Rule 7: check each owned pointer. If non-NULL, free it. */
if(a)
free(a);
if(b);
free(b);
if(c)
free(c);

/* Rule 8: return value depends on errno. (You could map to 0/1 or
something too,
as long as it depends on error code.) */
return errno;
}

This example shows the major rules, and looks fairly yucky until you
get used to it. However, it scales well if you have some complex
logic, nested ifs, etc. (e.g., if you only allocate c in some set of
circumstances). The basic idea is that every potentially unsafe action
(allocation, function call, etc.) is followed immediately by a macro
checking its return value and jumping to cleanup if there is an error.

Further, it's easy to check whether a given variable may be leaked.
You just check that all the rules are followed for that given variable.
By iterating over all variables this way, you get a robust function.

Like I said, it took a while (probably a week) to get used to this
coding style. But then we were golden, and spent very little time
debugging resource leaks.

Michael

[Bill Reid]

Re: Cleanup patterns


Richard Heathfield <rjh@see.sig.in validwrote in message
news:uf-dndTD3dU9pfDYnZ 2dnUVZ8qWdnZ2d@ bt.com...

MQ said:

I am just wondering how most people implement cleanup in C functions.

>
I can't speak for most people, but I do it like this:
>
Attempt to acquire resource
If attempt succeeded
Use resource
Release resource
Else
Handle error
Endif
>
This code structure is recursively extensible, and functions are a

fabulous

mechanism for keeping the indent level down.
>

Well, no, except for all trivial examples that aren't speed-sensitive.

For more complex functions, you'll quite often have contigent
dependancies for each resource acquisition that don't fit neatly into
the "nest", and the overhead of the function calling mechanism
means that functions are not infinitely or always a "fabulous
mechanism" for something as silly as "keeping the indent
level down" (unless you like your "C" code to run as slow
as "Java"!).

For me, the answer is whatever works best for the particular
situation; I do tend to use a few basic "patterns" quite often (and
sorry, I do use "goto"s a lot for these purposes when warranted),
but I think the real mistake here in this thread is to find a
"one size fits all" solution.

---
William Ernest Reid

[Richard Heathfield]

Re: Cleanup patterns

Bill Reid said:

>
Richard Heathfield <rjh@see.sig.in validwrote in message
news:uf-dndTD3dU9pfDYnZ 2dnUVZ8qWdnZ2d@ bt.com...

<snip>

>>
>This code structure is recursively extensible, and functions are a
>fabulous mechanism for keeping the indent level down.
>>

Well, no, except for all trivial examples that aren't speed-sensitive.

Not so far, not for me anyway. If this method ever turns out to be too slow,
I'll call you. In the meantime, I'll keep the code clear and simple.

<snip>

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999

http://www.cpax.org.uk

email: rjh at the above domain, - www.

[MQ]

Re: Cleanup patterns


Bill Reid wrote:

For more complex functions, you'll quite often have contigent
dependancies for each resource acquisition that don't fit neatly into
the "nest", and the overhead of the function calling mechanism
means that functions are not infinitely or always a "fabulous
mechanism" for something as silly as "keeping the indent
level down" (unless you like your "C" code to run as slow
as "Java"!).

These are my greatest concerns, as I am writing file system driver code
that needs to be fast efficient.