slightly OT: error handling conventions

[Matt Garman]

I was wondering what conventions people use for error handling in a
function while avoiding memory leaks at the same time.

I write a lot of functions that end up looking like the following:

int my_func() {
char *a, *b;
int x;

a = malloc(some_number);

x = another_function(a);
if (x == ERROR_CONDITION) {
free(a);
return ERROR;
}

x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) {
free(a);
free(b);
return ERROR;
}

free(a);
free(b);

return NO_ERROR;
}

As you can see, I essentially have a lot of redundant code. Plus,
this code could easily become a maintenance headache. Basically, I
often do a malloc(), perform some calculations or other
administrativia, maybe do some more malloc(), perform calculations,
etc. At each step along the way, I do error checking (I'm trying to
write robust code). If an error occurs, I need to exit the function,
but I also need to free any malloc'ed memory.

Is there a general strategy for dealing with this kind of scenario?
It seems like a goto might be useful here. Are there any other "nice"
conventions (readable, maintainable, etc)?

Thanks,
Matt

 

[Eric Sosman]

I was wondering what conventions people use for error handling in a
function while avoiding memory leaks at the same time.

I write a lot of functions that end up looking like the following:

int my_func() {
char *a, *b;
int x;

a = malloc(some_number);

x = another_function(a);
if (x == ERROR_CONDITION) {
free(a);
return ERROR;
}

x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) {
free(a);
free(b);
return ERROR;
}

free(a);
free(b);

return NO_ERROR;
}

As you can see, I essentially have a lot of redundant code. Plus,
this code could easily become a maintenance headache. Basically, I
often do a malloc(), perform some calculations or other
administrativia, maybe do some more malloc(), perform calculations,
etc. At each step along the way, I do error checking (I'm trying to
write robust code). If an error occurs, I need to exit the function,
but I also need to free any malloc'ed memory.

Is there a general strategy for dealing with this kind of scenario?
It seems like a goto might be useful here. Are there any other "nice"
conventions (readable, maintainable, etc)?

`goto' is a possibility. Nested `if' plus a variable to
keep track of the return status is another:

int my_func(void) {
char *a, *b;
int result = ERROR;
a = malloc(some_number);
if (a != NULL) { // a test you omitted
x = another_function(a);
if (x != ERROR_CONDITION) {
x = func_that_mallocs(&b);
if (x != ERROR_CONDITION) {
do_something(); // another omission
result = NO_ERROR;
}
free (b); // dubious, but mimics your code
}
free (a);
}
return result;
}

.... but this can become unwieldy. If there are a lot of things
to test, `goto' may be a good deal clearer:

int my_func(void) {
char *a = NULL, *b = NULL;
int result = ERROR;
a = malloc(some_number);
if (a == NULL) goto EXIT;
x = another_function(a);
if (x == ERROR_CONDITION) goto EXIT;
x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) goto EXIT;
do_something();
result = NO_ERROR;
EXIT:
free (b);
free (a);
return result;
}

Note that the initializations of `a' and `b' allow you to have
just a single exit-point instead of many.

If a function has very many such conditions, consider whether
it might make more sense to break it up into multiple smaller
functions, each able to clean up after itself and "pass the buck"
to the caller.

And then, of course, there's always `abort()' ...

 

[Ian Woods]

I was wondering what conventions people use for error handling in a
function while avoiding memory leaks at the same time.

I write a lot of functions that end up looking like the following:

int my_func() {
char *a, *b;
int x;

a = malloc(some_number);

x = another_function(a);
if (x == ERROR_CONDITION) {
free(a);
return ERROR;
}

x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) {
free(a);
free(b);
return ERROR;
}

free(a);
free(b);

return NO_ERROR;
}

As you can see, I essentially have a lot of redundant code. Plus,
this code could easily become a maintenance headache. Basically, I
often do a malloc(), perform some calculations or other
administrativia, maybe do some more malloc(), perform calculations,
etc. At each step along the way, I do error checking (I'm trying to
write robust code). If an error occurs, I need to exit the function,
but I also need to free any malloc'ed memory.

Is there a general strategy for dealing with this kind of scenario?
It seems like a goto might be useful here. Are there any other "nice"
conventions (readable, maintainable, etc)?

Thanks,
Matt

For code where I have multiple resources to find, use, then release I
often use the idea of a 'runlevel'. Basically, it's this:

int do_stuff(char * iname, char * oname) {
int rl=0; /* the 'runlevel' variable */
int rv=ERROR; /* the error return value */
FILE * inf;
FILE * outf;
void * mem;

inf=fopen(iname,"r");
if (!inf) goto on_err;
else rl++; /* opened a resource, go up a level */

outf=fopen(oname,"w");
if (!outf) goto on_err;
else rl++; /* opened a resource, go up a level */

mem=malloc(WHATEVER_SIZE);
if (!mem) goto on_err;
else rl++; /* opened a resource, go up a level */

/* do stuff with inf, outf and mem */

rv=0; /* the success return value */

on_err:
switch(rl) {
case 3: free(mem);
case 2: fclose(outf);
case 1: fclose(inf);
case 0: /* nothing to clean up */
}

return rv;
}

There are plenty of ways this can be improved, but I find this particular
method to be easy to code and easy to analyse. I wouldn't expect most
people to just 'see' what was going on, so it's likely to be a pain for
someone else to maintain - at least until they're familiar with the idea.

Ian Woods

 

[Sheldon Simms]

I write a lot of functions that end up looking like the following:

int my_func() {
char *a, *b;
int x;

a = malloc(some_number);

Check to see that malloc succeeded

x = another_function(a);
if (x == ERROR_CONDITION) {
free(a);
return ERROR;
}

x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) {
free(a);
free(b);
return ERROR;
}

free(a);
free(b);

return NO_ERROR;
}

As you can see, I essentially have a lot of redundant code.

Is there a general strategy for dealing with this kind of scenario?
It seems like a goto might be useful here. Are there any other "nice"
conventions (readable, maintainable, etc)?

I think using goto is a reasonable (readable, maintainable, etc.)
alternative for situations like this. However, I would make sure that
func_that_mallocs() cleans up after itself if it fails instead of
requiring its caller to do it.

-Sheldon

 

[Jack Klein]

I was wondering what conventions people use for error handling in a
function while avoiding memory leaks at the same time.

I write a lot of functions that end up looking like the following:

int my_func() {
char *a, *b;
int x;

a = malloc(some_number);

x = another_function(a);
if (x == ERROR_CONDITION) {
free(a);
return ERROR;
}

x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) {
free(a);
free(b);
return ERROR;
}

free(a);
free(b);

return NO_ERROR;
}

As you can see, I essentially have a lot of redundant code. Plus,
this code could easily become a maintenance headache. Basically, I
often do a malloc(), perform some calculations or other
administrativia, maybe do some more malloc(), perform calculations,
etc. At each step along the way, I do error checking (I'm trying to
write robust code). If an error occurs, I need to exit the function,
but I also need to free any malloc'ed memory.

Is there a general strategy for dealing with this kind of scenario?
It seems like a goto might be useful here. Are there any other "nice"
conventions (readable, maintainable, etc)?

Thanks,
Matt

I may get blasted for this, but I've always liked this one:

int my_func()
{
char *a = NULL;
char *b = NULL;
FILE *fin = NULL;
FILE *fout = NULL;
int x = SUCCESS:

do
{
a = malloc(...);
b = malloc(...);
if (!a || !b)
{
x = OUT_OF_MEMORY;
break;
}

fin = fopen(...);
fout = fopen(...);
if (!fin || !fout)
{
x = FILE_OPEN_ERROR;
break;
}

/* do the real work */
} while (0);

free(a);
free(b);
if (fin) fclose(fin);
if (fout) fclose(fout);
return x;
}

Of course the do { ... } while (0) with breaks is just a disguised
goto, as indeed are all flow control structures, but inside a block
like this you know they all go to the same place, you don't have to
mentally check the goto labels to see if they are all the same.

As an aside, I have often wished that fclose(NULL) was defined as a
no-op, as is free(NULL), for the same reason of symmetry. Then you
could always fclose() the pointer returned by fopen() whether
successful or not, just as you can always free() the pointer returned
by *alloc(), successful or not.

--
Jack Klein
Home: http://JK-Technology.Com
FAQs for
comp.lang.c http://www.eskimo.com/~scs/C-faq/top.html
comp.lang.c++ http://www.parashift.com/c++-faq-lite/
alt.comp.lang.learn.c-c++ ftp://snurse-l.org/pub/acllc-c++/faq

 

[CBFalconer]

I was wondering what conventions people use for error handling in
a function while avoiding memory leaks at the same time.

I write a lot of functions that end up looking like the following:

int my_func() {
char *a, *b;
int x;

a = malloc(some_number);

x = another_function(a);
if (x == ERROR_CONDITION) {
free(a);
return ERROR;
}

x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) {
free(a);
free(b);
return ERROR;
}

free(a);
free(b);

return NO_ERROR;
}

As you can see, I essentially have a lot of redundant code. Plus,
this code could easily become a maintenance headache. Basically, I
often do a malloc(), perform some calculations or other
administrativia, maybe do some more malloc(), perform calculations,
etc. At each step along the way, I do error checking (I'm trying to
write robust code). If an error occurs, I need to exit the function,
but I also need to free any malloc'ed memory.

Is there a general strategy for dealing with this kind of scenario?
It seems like a goto might be useful here. Are there any other "nice"
conventions (readable, maintainable, etc)?

I would write that as:

int my_func()
{
char *a, *b;
int x;
int err;

a = b = NULL;
if (!(a = malloc(some_number))) /* you omitted this test */
err = NOMEM;
else if (ERROR_CONDITION == (x = another_function(a)))
err = ERROR;
else if (ERROR_CONDITION == (x = func_that_mallocs(&b)))
err = ERROR;
else
err = NO_ERROR;
free(a);
free(b)
return err;
}

 

[Richard Heathfield]

Ian Woods wrote:

8> For code where I have multiple resources to find, use, then release I

often use the idea of a 'runlevel'. Basically, it's this:

int do_stuff(char * iname, char * oname) {
int rl=0; /* the 'runlevel' variable */
int rv=ERROR; /* the error return value */
FILE * inf;
FILE * outf;
void * mem;

inf=fopen(iname,"r");
if (!inf) goto on_err;
else rl++; /* opened a resource, go up a level */

on_err:
switch(rl) {
case 3: free(mem);
case 2: fclose(outf);
case 1: fclose(inf);
case 0: /* nothing to clean up */
}

return rv;
}

There are plenty of ways this can be improved, but I find this particular
method to be easy to code and easy to analyse. I wouldn't expect most
people to just 'see' what was going on, so it's likely to be a pain for
someone else to maintain - at least until they're familiar with the idea.

I can certainly see what's going on, and I find your approach interesting,
but I do have a question. I know you're not a fan of spaghetti programming.
Here, though, there is little spaghetti to speak of, since you use goto to
jump to the end of the function (straight to the error-handling, that is).
This is the only use of goto I've ever seriously contemplated in C (because
it's not exactly spaghetti - or rather, it's more like rigid, uncooked
spaghetti, straight off the tree), but in fact I've always found other ways
to do it.

My question is this: Have you /considered/ other possibilities, especially
w.r.t. maintenance issues, or are you content with the goto in this
context? That is, have you explored this runlevel idea, /sans/ goto? If so,
what were your findings?

[Personal aside: Ian, I haven't forgotten, honest! I hope to get my email
fixed up any month now.]

 

[Richard Heathfield]

I was wondering what conventions people use for error handling in a
function while avoiding memory leaks at the same time.

I write a lot of functions that end up looking like the following:

int my_func() {
char *a, *b;
int x;

a = malloc(some_number);

x = another_function(a);
if (x == ERROR_CONDITION) {
free(a);
return ERROR;
}

x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) {
free(a);
free(b);
return ERROR;
}

free(a);
free(b);

return NO_ERROR;
}

I've rewritten this in "my" style. See what you think.

int my_func(void)
{
int rc = SUCCESS;
char *a = malloc(some_number);
char *b = NULL;

if(a != NULL)
{
rc = another_function(a);
if(SUCCESS == rc)
{
rc = func_that_mallocs(&b);
if(SUCCESS == rc)
{
rc = DoStuff(a, b);
free(b);
}
}
free(a);
}
return rc;
}

 

[Sheldon Simms]

My question is this: Have you /considered/ other possibilities, especially
w.r.t. maintenance issues, or are you content with the goto in this
context? That is, have you explored this runlevel idea, /sans/ goto? If so,
what were your findings?

Sorry, I'm not Ian, but I would like to ask a question of my own:

What would be the advantage of eliminating goto in a situation
like this?

I once got involved in a large, er, "discussion" on the German C
group about goto. No matter what situation was presented, the
majority of those participating agreed that the use of goto was
an abomination that should be avoided at all costs. Many of the
same people also claimed to refuse to use break and continue.

Their reasoning for this was hard for me to understand, since I
lacked the cultural background, but apparently some influential
professor had some ideas about program design which led most of
them to require all programs to be designed by drawing boxes
inside of boxes. Control was only permitted to flow into the
top of a box or out of the bottom of a box.

This was all nice, and I understood it to be a graphical approach
to structured programming, but what I didn't understand was the
attitude that *absolutely no* deviation from this schema was to
be tolerated.

-Sheldon

 

[Christian Bau]

I once got involved in a large, er, "discussion" on the German C
group about goto. No matter what situation was presented, the
majority of those participating agreed that the use of goto was
an abomination that should be avoided at all costs. Many of the
same people also claimed to refuse to use break and continue.

Their reasoning for this was hard for me to understand, since I
lacked the cultural background, but apparently some influential
professor had some ideas about program design which led most of
them to require all programs to be designed by drawing boxes
inside of boxes. Control was only permitted to flow into the
top of a box or out of the bottom of a box.

If anybody asks for advice whether to use goto or not, you should tell
them not to use it. People who ask for advice shouldn't use goto.

At some point they will not ask for advice anymore, and at some point
they will either start thinking for themselves or they will not. If they
think for themselves, they will use goto when it is appropriate and
avoid it when it is inappropriate, and if they are smart they will know
when it is appropriate. If they don't think for themselves, they will
still insist that goto is always evil.

Usual indication of "not thinking" is when you present an example that
is more readable because it uses goto, and they insist that it is worse
because it uses goto. So the conclusion that they draw is that using
"goto" is bad because you use "goto".

 

[Nick Keighley]

Sorry, I'm not Ian, but I would like to ask a question of my own:

What would be the advantage of eliminating goto in a situation
like this?

I once got involved in a large, er, "discussion" on the German C
group about goto. No matter what situation was presented, the
majority of those participating agreed that the use of goto was
an abomination that should be avoided at all costs. Many of the
same people also claimed to refuse to use break and continue.

Their reasoning for this was hard for me to understand, since I
lacked the cultural background, but apparently some influential
professor had some ideas about program design which led most of
them to require all programs to be designed by drawing boxes
inside of boxes. Control was only permitted to flow into the
top of a box or out of the bottom of a box.

This was all nice, and I understood it to be a graphical approach
to structured programming, but what I didn't understand was the
attitude that *absolutely no* deviation from this schema was to
be tolerated.

"goto considered harmful"
http://www.acm.org/classics/oct95/

"structured programming with goto"
http://pplab.snu.ac.kr/courses/PL2001/papers/p261-knuth.pdf

then google "heathfield goto addict"

(and Richard, don't let this goto your head...)


--
Nick Keighley

"I've been on the wagon now for more than a decade.
Not a single goto in all that time. I just don't
need them any more. I don't even use break or continue
now, except on social occasions of course.
And I don't get carried away."

 

[Dan Pop]

It seems like a goto might be useful here.

A goto is useful any time it helps simplifying the program structure,
especially reducing the number of indentation levels (but not at the cost
of increasing the complexity of the program control structure, i.e.
generating spaghetti code).

Premature function termination (when cleanup is needed) is a typical case
of goto usage.

Dan

 

[David Rubin]

I was wondering what conventions people use for error handling in a
function while avoiding memory leaks at the same time.

I write a lot of functions that end up looking like the following:

int my_func() {
char *a, *b;
int x;

a = malloc(some_number);

x = another_function(a);
if (x == ERROR_CONDITION) {
free(a);
return ERROR;
}

x = func_that_mallocs(&b);
if (x == ERROR_CONDITION) {
free(a);
free(b);
return ERROR;
}

return NO_ERROR;
}

Here's one more refactored implementation:

int
my_func(void)
{
char *a = 0;
char *b = 0;
int r = NO_ERROR;

if((a=malloc(some_number)) == 0){
return ERROR;
}

if(another_function(a) == ERROR_CONDITION
|| func_that_mallocs(&b) == ERROR_CONDITION){
r = ERROR;
}

free(a);
free(b);
return r;
}

 

[Erik Domstad]

I've rewritten this in "my" style. See what you think.

int my_func(void)
{
int rc = SUCCESS;
char *a = malloc(some_number);
char *b = NULL;

if(a != NULL)
{
rc = another_function(a);
if(SUCCESS == rc)
{
rc = func_that_mallocs(&b);
if(SUCCESS == rc)
{
rc = DoStuff(a, b);
free(b);
}
}
free(a);
}
return rc;
}

I often write like this. It involves a few redundant tests of rc
but it is not nested as deep. I even check rc the first time even though
I know it's SUCCESS. This way it's easy to uncomment or move blocks of code.

Perhaps not very elegant but I think it's quite readable and easy to maintain.

int my_func(void)
{
int rc = SUCCESS;
char *a = NULL;
char *b = NULL;

if (SUCCESS == rc) {
a = malloc(some_number)
if (NULL == a)
rc = FAILURE;
}

if(SUCCESS == rc)
rc = another_function(a);

if(SUCCESS == rc)
rc = func_that_mallocs(&b);

if(SUCCESS == rc)
rc = DoStuff(a, b);

if (NULL != b)
free(b);

if (NULL != a)
free(a);

return rc;
}

 

[Ed Morton]

A goto is useful any time it helps simplifying the program structure,

I agree in theory, but I've yet to see an example that wouldn't be more
easily understandable using variables. e.g. elsethread a program
structure like this was proposed (the only proposal I saw that used goto):

void foo()
int reached = 0;
if (a() == 3) {
goto cleanup;
} else {
reached++;
}
if (b() == 4) {
goto cleanup;
} else {
reached++;
}
if (c() < 0) {
goto cleanup;
} else {
reached++;
}

cleanup:
switch (reached) {
case 3: undoc();
case 2: undob();
case 1: undoa();
}
}

Yes, we could all figure out what this does, especially if we were
familiar with this style of coding, but why is a() returning 3
apparently an error? Ditto for b() returning 4? Again, if the values
tested for are obvious like NULL then again we can figure it out, but
why write this type of code forcing future developers enhancing or
debugging it to have to figure things out (i.e. synthesize the
abstraction) when you can write something like this in the first place:

void foo()
enum { INIT, GOT_RSRCS, SENT_MSG, GOT_RSP } rslt = INIT;

if (a() != 3) {
rslt = GOT_RSRCS;
}
if (rslt == GOT_RSRCS) {
if (b() != 4) {
rslt = SENT_MSG;
}
}
if (rslt == SENT_MSG) {
if (c() >= 0) {
rslt = GOT_RSP;
}
}

cleanup:
switch (rslt) {
case GOT_RSP: undoc();
case SENT_MSG: undob();
case GOT_RSRCS: undoa();
}
}

It's now immediately clear to next poor sap who has to work on this WHY
we're going to cleanup. Bear in mind that same poor sap could have
several hundred other lines of code to try to read and understand while
trying to fix a bug that's going to cause the loss of a
multi-million-dollar contract if not fixed by Friday!

Among others, I've read the Knuth and Djikstra articles about use of
gotos and I've yet to see an example of a reason to use them other than
a very slight increase in execution efficiency (which is, of course,
good enough reason in sme situations) but I'm open to persuasion.

Ed.

 

[Dan Pop]

I agree in theory, but I've yet to see an example that wouldn't be more
easily understandable using variables. e.g. elsethread a program
structure like this was proposed (the only proposal I saw that used goto):

void foo()

Where is the brace?

int reached = 0;
if (a() == 3) {
goto cleanup;
} else {
reached++;
}
if (b() == 4) {
goto cleanup;
} else {
reached++;
}
if (c() < 0) {
goto cleanup;
} else {
reached++;
}

cleanup:
switch (reached) {
case 3: undoc();
case 2: undob();
case 1: undoa();
}
}

Yes, we could all figure out what this does, especially if we were
familiar with this style of coding, but why is a() returning 3
apparently an error? Ditto for b() returning 4? Again, if the values
tested for are obvious like NULL then again we can figure it out, but
why write this type of code forcing future developers enhancing or
debugging it to have to figure things out (i.e. synthesize the
abstraction) when you can write something like this in the first place:

void foo()
Ditto.

enum { INIT, GOT_RSRCS, SENT_MSG, GOT_RSP } rslt = INIT;

if (a() != 3) {
rslt = GOT_RSRCS;
}
if (rslt == GOT_RSRCS) {
if (b() != 4) {
rslt = SENT_MSG;
}
}
if (rslt == SENT_MSG) {
if (c() >= 0) {
rslt = GOT_RSP;
}
}

cleanup:
switch (rslt) {
case GOT_RSP: undoc();
case SENT_MSG: undob();
case GOT_RSRCS: undoa();
}
}

In many cases, it is relatively easy to determine what needs to be
undone without explicitly keeping track of it. This leads to the much
simpler:

void foo()
{
if (a() != OK) goto cleanup;
if (b() != OK) goto cleanup;
c();

cleanup:
if (donec) undoc();
if (doneb) undob();
if (donea) undoa();
}

Note that this is pseudocode, the done* things being expressions, rather
than variables and the undo* things being cleanup actions, rather than
actual function calls.

And given the triviality of the example, it can be rewritten without goto
and without nested if's:

a() == OK && b() == OK && c() ;

but this is not the point. I'd rather use the goto version, BTW ;-)

The things that usually need cleanup in a standard C program are
dynamically allocated buffers and open streams. If the corresponding
pointers are initialised to NULL, no check whatsoever is needed for
freeing the buffers, an unconditional free() call is doing the job,
only FILE pointers need to be checked and fclose() called if they are
non-null.

Similar tricks can be done for other resources that need to be released
by non-portable C programs, in order to simplify the overall code
structure. If all the gotos in a function have the same destination,
few people have a valid reason for complaining.

Dan

 

[Sheldon Simms]

"goto considered harmful"
http://www.acm.org/classics/oct95/

"structured programming with goto"
http://pplab.snu.ac.kr/courses/PL2001/papers/p261-knuth.pdf

Yes yes, thanks. I've read them before.

But Dijkstra isn't God, and "Go To Considered Harmful" isn't
holy scripture. As for Knuth, he's *for* goto, as long as
it's properly used.

So once again, what would be the advantage of eliminating
goto in the code under discussion (which I reinsert below)?

 

[Sheldon Simms]

Usual indication of "not thinking" is when you present an example that
is more readable because it uses goto, and they insist that it is worse
because it uses goto.

What they will insist is that the version without goto is more
readable. And the reason they will give for that is precisely that
there is no goto. Since "more readable" is a subjective judgement,
it's impossible to argue the point, so I don't try anymore.

 

[CBFalconer]

.... snip ...

If anybody asks for advice whether to use goto or not, you should tell
them not to use it. People who ask for advice shouldn't use goto.

Excellent. The last sentence is sig material.

 

[CBFalconer]

.... snip ...

I've rewritten this in "my" style. See what you think.

int my_func(void)
{
int rc = SUCCESS;
char *a = malloc(some_number);
char *b = NULL;

if(a != NULL)
{
rc = another_function(a);
if(SUCCESS == rc)
{
rc = func_that_mallocs(&b);
if(SUCCESS == rc)

You have a hidden assumption here, i.e. that failure does not
malloc(&b).