Checking return values for errors, a matter of style?

[Johan Tibell]

I've written a piece of code that uses sockets a lot (I know that
sockets aren't portable C, this is not a question about sockets per
se). Much of my code ended up looking like this:

if (function(socket, args) == -1) {
perror("function");
exit(EXIT_FAILURE);
}

I feel that the ifs destroy the readability of my code. Would it be
better to declare an int variable (say succ) and use the following
structure?

int succ;

succ = function(socket, args);
if (succ == -1) {
perror("function");
exit(EXIT_FAILURE);
}

What's considered "best practice" (feel free to substitute with: "what
do most good programmers use")?

 

[Richard Heathfield]

Johan Tibell said:

I've written a piece of code that uses sockets a lot (I know that
sockets aren't portable C, this is not a question about sockets per
se). Much of my code ended up looking like this:

if (function(socket, args) == -1) {
perror("function");
exit(EXIT_FAILURE);
}

I feel that the ifs destroy the readability of my code. Would it be
better to declare an int variable (say succ) and use the following
structure?

int succ;

succ = function(socket, args);
if (succ == -1) {
perror("function");
exit(EXIT_FAILURE);
}

What's considered "best practice" (feel free to substitute with: "what
do most good programmers use")?

My normal practice is to use an int to catch the return value so that I can
inspect it. My library routines - at least, the ones that don't return
pointers - return int, with one particular value indicating success, and
any value other than that indicating the reason for the error. There's no
point in my wondering /why/ a function failed if I can't be bothered to
store the error code returned by that function!

But there are times when I use the other way - classic example would be an
fgets loop:

while(fgets(buffer, sizeof buffer, stdin) != NULL)
{
dosomethingwith(buffer);
}

 

[Frederick Gotham]

Johan Tibell posted:

I've written a piece of code that uses sockets a lot (I know that
sockets aren't portable C, this is not a question about sockets per
se). Much of my code ended up looking like this:

if (function(socket, args) == -1) {
perror("function");
exit(EXIT_FAILURE);
}

I feel that the ifs destroy the readability of my code. Would it be
better to declare an int variable (say succ) and use the following
structure?

int succ;

succ = function(socket, args);
if (succ == -1) {
perror("function");
exit(EXIT_FAILURE);
}

What's considered "best practice" (feel free to substitute with: "what
do most good programmers use")?

Have you considered something like:

void Assure(int const i)
{
if(-1 == i)
{
perror("function");
exit(EXIT_FAILURE);
}
}

And then putting something like the following in your code:

Assure( function(socket,args) );

 

[jacob navia]

Johan Tibell a écrit :

I've written a piece of code that uses sockets a lot (I know that
sockets aren't portable C, this is not a question about sockets per
se). Much of my code ended up looking like this:

if (function(socket, args) == -1) {
perror("function");
exit(EXIT_FAILURE);
}

I feel that the ifs destroy the readability of my code. Would it be
better to declare an int variable (say succ) and use the following
structure?

int succ;

succ = function(socket, args);
if (succ == -1) {
perror("function");
exit(EXIT_FAILURE);
}

What's considered "best practice" (feel free to substitute with: "what
do most good programmers use")?

Using the second form allows you to easily see the return value in
the debugger. To see the error code using the first form you would
have to follow the machine in machine code and read the return
register... not so easy in many debuggers.

 

[Eric Sosman]

Johan Tibell wrote On 07/31/06 17:20,:

I've written a piece of code that uses sockets a lot (I know that
sockets aren't portable C, this is not a question about sockets per
se). Much of my code ended up looking like this:

if (function(socket, args) == -1) {
perror("function");
exit(EXIT_FAILURE);
}

I feel that the ifs destroy the readability of my code. Would it be
better to declare an int variable (say succ) and use the following
structure?

int succ;

succ = function(socket, args);
if (succ == -1) {
perror("function");
exit(EXIT_FAILURE);
}

What's considered "best practice" (feel free to substitute with: "what
do most good programmers use")?

I don't see much difference between these two styles.
They take about the same amount of verbiage, and are equally
readable. I think I'd prefer the second style (slightly) if
the returned value has a significance beyond just success vs.
failure or if the function call is long and involved; I'd favor
the first style if the returned value is strictly a yes/no
status and the function call is fairly brief. It's not worth
making a fetish of, though.

If my program had a lot of these and I got tired of
typing the boilerplate over and over (probably misspelling
EXIT_FALIURE a few times), I'd write myself a tiny wrapper
along the lines of

static void crash(const char *message) {
if (message != NULL)
perror (message);
exit (EXIT_FAILURE);
}

Then the "down in the trenches" code becomes

if (function(socket, args) == -1)
crash ("function");

succ = function(socket, args);
if (succ == -1)
crash ("function");

Let's see: Expending five lines on the wrapper function saves
me two lines each time I use it, so I'm ahead of the game as
soon as I've checked for my third error ;-) More importantly,
the smaller "footprint" of the error-handling code lets the
reader scan it with perhaps a little more ease.

 

[Keith Thompson]

Johan Tibell a écrit :

Using the second form allows you to easily see the return value in
the debugger. To see the error code using the first form you would
have to follow the machine in machine code and read the return
register... not so easy in many debuggers.

That could be a valid approach, depending on your development style
and environment.

Personally, I don't use debuggers very often, so it usually wouldn't
occur to me to distort my code to make it easier to use in a debugger.
But if you find it useful, go for it.

 

[Keith Thompson]

I've written a piece of code that uses sockets a lot (I know that
sockets aren't portable C, this is not a question about sockets per
se). Much of my code ended up looking like this:

if (function(socket, args) == -1) {
perror("function");
exit(EXIT_FAILURE);
}

I feel that the ifs destroy the readability of my code. Would it be
better to declare an int variable (say succ) and use the following
structure?

int succ;

succ = function(socket, args);
if (succ == -1) {
perror("function");
exit(EXIT_FAILURE);
}

What's considered "best practice" (feel free to substitute with: "what
do most good programmers use")?

I find both forms about equally readable.

If you're only going to refer to the function's result once, as you do
here, there's no real need to store the value in a variable. If
you're going to refer to it more than once, a variable is helpful,
perhaps essential. For example:

int result;

result = function(socket, args);
if (result != 0) {
fprintf(stderr,
"function returned %d, errno = %d\n",
result,
errno);
exit(EXIT_FAILURE);
}

But if the function returns only a simple success/failure code, and
the detailed information is elsewhere, this probably isn't necessary.

 

[jacob navia]

Keith Thompson a écrit :

Personally, I don't use debuggers very often, so it usually wouldn't
occur to me to distort my code to make it easier to use in a debugger.

Ahhh... You do not use debuggers very often?

Mmmm... Well, they are great tools. I use them very often,
actually I spend most of the time either in the editor
or in the debugger.

The only time when I did not use a debugger was when I was writing
the debugger for lcc-win32. My debugger wasn't then able
to debug itself so I had to develop it without any help, what made
things considerably more difficult...

But if you find it useful, go for it.

Yes, I use them very often.

 

[Andrew Poelstra]

I've written a piece of code that uses sockets a lot (I know that
sockets aren't portable C, this is not a question about sockets per
se). Much of my code ended up looking like this:

if (function(socket, args) == -1) {
perror("function");
exit(EXIT_FAILURE);
}

I feel that the ifs destroy the readability of my code. Would it be
better to declare an int variable (say succ) and use the following
structure?

int succ;

succ = function(socket, args);
if (succ == -1) {
perror("function");
exit(EXIT_FAILURE);
}

What's considered "best practice" (feel free to substitute with: "what
do most good programmers use")?

It really depends on what your target is. In most of my code, I'm
thinking of removing all the checks for stuff like malloc(10) because
there is only a 0.02% chance that I'll be porting my code to a
non-desktop machine where memory is scarce.

OTOH, if you work for a company that deals in all sorts of systems, it
may be worthwhile to have a meaningful error message printed in the case
of problems.

It's not so much a matter of style as it is a matter of practibility:
your code must be robust, but it should also be easy to read. Nowadays
programs targeted at home computers or servers can assume that you'll
have a 99.99% success rate on a functioning system when allocating
memory < 1Kb.

 

[Keith Thompson]

It really depends on what your target is. In most of my code, I'm
thinking of removing all the checks for stuff like malloc(10) because
there is only a 0.02% chance that I'll be porting my code to a
non-desktop machine where memory is scarce.

OTOH, if you work for a company that deals in all sorts of systems, it
may be worthwhile to have a meaningful error message printed in the case
of problems.

It's not so much a matter of style as it is a matter of practibility:
your code must be robust, but it should also be easy to read. Nowadays
programs targeted at home computers or servers can assume that you'll
have a 99.99% success rate on a functioning system when allocating
memory < 1Kb.

Why would you want to settle for 99.99% when you can get 100%?

It takes only one failure to ruin your whole day.

 

[Andrew Poelstra]

Why would you want to settle for 99.99% when you can get 100%?

It takes only one failure to ruin your whole day.

Because that tiny percentage is the difference between
p = malloc (sizeof *p * q);

and
p = malloc (sizeof *p * q);
if (rv = !!p)
{
/* Rest of function here. */
}

Those ifs nest up and it becomes a pain to manage them. I prefer not to
have multiple returns because they annoy me; if I want to know what a
function returns, I like to skip to the end. If I see a "return 0" I
know that I either have a foolproof function, or I have to go and find
other return values elsewhere in the code. Better to have a
return rv; /* rv is 0 on success, or -1 on error (bad memory or file) */

Another idea would be to create a varadic function that checks multiple
memory allocations:
if (chk_mem (p, q, r, s, t) && chk_file (inf, outf))
{
/* Rest of function is happy. */
}

Hmm. I'm not sure whether to be sad that my readability argument is
falling apart, or happy that I'm learning better code design.


Also, I have an diagnostic library that will send errors to any stream
specified. So, if a user has a problem with a 12-byte allocation and the
rest of the system is for some reason working fine, I can tell him to
run myprog --debug and send me the output of myprog.log.

There's an extra step in there where he's emailing me a log, but
otherwise the communication would be the same; I'd have to tell
him to increase his swap space or to run fewer programs. (I'm
assuming here that a bad allocation will give me a nice GPF or
segfault; this isn't always true and is one of the most dangerous
assumptions I tend to make.)

 

[Keith Thompson]

[...]
Why would you want to settle for 99.99% when you can get 100%?

It takes only one failure to ruin your whole day.

Because that tiny percentage is the difference between
p = malloc (sizeof *p * q);

and
p = malloc (sizeof *p * q);
if (rv = !!p)
{
/* Rest of function here. */
}

Those ifs nest up and it becomes a pain to manage them. I prefer not to
have multiple returns because they annoy me; if I want to know what a
function returns, I like to skip to the end. If I see a "return 0" I
know that I either have a foolproof function, or I have to go and find
other return values elsewhere in the code. Better to have a
return rv; /* rv is 0 on success, or -1 on error (bad memory or file) */

[...]

If you just abort the program on a malloc() failure, there's no reason
for the level of nesting to get out of hand.

p = malloc(sizeof *p * q);
if (p == NULL) {
fprintf(stderr, "malloc failed\n");
exit(EXIT_FAILURE);
}
/* Rest of function here */

It's not a *great* way to handle the error, but it's better than
ignoring it.

Or, as I suggested, write a wrapper:

void *my_malloc(size_t size)
{
void *result = malloc(size);
if (result == NULL) {
fprintf(stderr, "malloc failed\n");
exit(EXIT_FAILURE);
}
else {
return result;
}
}

....

p = my_malloc(sizeof *p * q);

The alternative is to ignore allocation failures and risk undefined
behavior.

 

[Bill Pursell]

Also, I have an diagnostic library that will send errors to any stream
specified. So, if a user has a problem with a 12-byte allocation and the
rest of the system is for some reason working fine, I can tell him to
run myprog --debug and send me the output of myprog.log.

This won't work at all. If I run your program and get a segfault
because of a memory error, when I run your diagnostic there's
a very good chance that the problem won't re-occur. It will
end up being an error that happens .01% of the time, but
never when I'm running the diagnostic tool! Pretty quickly,
I'll just decide that your code is unstable and I'll stop using
it. And if I ever look through the source and see unchecked
mallocs, I'll stop using it immediately! Putting a wrapper
around malloc ( "xmalloc" and "Malloc" are the two common
names I've seen, "my_malloc" has been mentioned in this
thread) that aborts on error is a much better approach, if
for no other reason than keeping your users happy.
At run time,I'd much rather see:
"Out of memory", than "Segmentation fault".

 

[Bill Pursell]

Johan Tibell posted, w.r.t. checking return values:


Have you considered something like:

void Assure(int const i)
{
if(-1 == i)
{
perror("function");
exit(EXIT_FAILURE);
}
}

That's nice. How about:

#define Assure(x) if (!(x)) {fprintf(stderr, "In %s:%s " #x "
failed\n",\
__FILE__, __func__); exit(EXIT_FAILURE);}

And then putting something like the following in your code:

Assure( function(socket,args) );

Which allows slightly more flexibility:
Assure (function(a,b,c) == expected_value);

 

[Richard Heathfield]

Andrew Poelstra said:

It's not so much a matter of style as it is a matter of practibility:
your code must be robust, but it should also be easy to read.

Your code is not going to be robust if it doesn't check whether a request
for an external resource was successful. I agree it should be easy to read,
but that doesn't mean leaving the code out!

Nowadays
programs targeted at home computers or servers can assume that you'll
have a 99.99% success rate on a functioning system when allocating
memory < 1Kb.

Programmers who make such an assumption should not be writing for the home
market or the server market. They should be writing in crayon on droolproof
paper.

 

[Haroon Shafiq]

I've written a piece of code that uses sockets a lot (I know that [...]

What's considered "best practice" (feel free to substitute with: "what
do most good programmers use")?

Following style makes it clear from reading point of view,

/********/
#define RET_CODE_SUCCESS 1
#define RET_CODE_FAILURE -1

....

if (RET_CODE_FAILURE == function(socket, args) {
perror("function");
exit(EXIT_FAILURE);
}
/********/

As far as storing the return value in a variable is concerned, if you
are not going to use it, there is no need to store it. If you want to
check for the returned value during debugging, you can step-in to that
function.

 

[Haroon Shafiq]

[...]


Because that tiny percentage is the difference between
p = malloc (sizeof *p * q);

and
p = malloc (sizeof *p * q);
if (rv = !!p)
{
/* Rest of function here. */
}

Those ifs nest up and it becomes a pain to manage them.

Your customer pulling his hair on a segmentation fault while doing an
intense calculation or running a major server is even a bigger pain,
for him and for your repute.

Another idea would be to create a varadic function that checks multiple
memory allocations:
if (chk_mem (p, q, r, s, t) && chk_file (inf, outf))
{
/* Rest of function is happy. */
}

What if you want to do some thing if memory allocation fails at one
point and some thing else if it fails at some other point?

Hmm. I'm not sure whether to be sad that my readability argument is
falling apart, or happy that I'm learning better code design.

Better code design and readability, IMHO, complement each other.

There's an extra step in there where he's emailing me a log, but
otherwise the communication would be the same; I'd have to tell
him to increase his swap space or to run fewer programs.

One of the worst pieces of advice I have ever heard. Do you work for,
ahem, M$?

 

[Andrew Poelstra]

Andrew Poelstra said:

Your code is not going to be robust if it doesn't check whether a request
for an external resource was successful. I agree it should be easy to read,
but that doesn't mean leaving the code out!

What exactly /would/ be the way to do such a thing? I ask you because
you don't like multiple returns or the break statement, both of which
would be a typical response.

Programmers who make such an assumption should not be writing for the home
market or the server market. They should be writing in crayon on droolproof
paper.

Being as every other post was pretty much exactly as insulting as this,
I'd say that I wasn't not wrong on any minor point! I'm glad that I haven't
had the chance to make these foolhardy changes to my actual code yet.

I've written a new interface to my error library so that it will be able
to handle memory failures gracefully, log to a runtime-determined file,
check for bad files or memory, and ensure that a proper message reaches
the user if it can't go on.

 

[Andrew Poelstra]

I wasn't not wrong on any minor point!

Seems it's not just the 2AM folks getting their "not"s mixed up! I meant
"I wasn't wrong on some minor point!"

 

[Al Balmer]

It's not so much a matter of style as it is a matter of practibility:
your code must be robust, but it should also be easy to read. Nowadays
programs targeted at home computers or servers can assume that you'll
have a 99.99% success rate on a functioning system when allocating
memory < 1Kb.

You've forgotten the rule of computing probability - if there's one
chance in a million, it happens every second.

You must have written some of the code for Windows.