Check all errors in code?

[lovecreatesbea...]

Do you check all error conditions for all library calls in you code?

Is it necessary to check all errors? Is it convenient to check all
errors (or how to make code clean and readable with mass of non
business logic related code block)?

The following code from net-snmp library calls atoi five times but
checks none. Do we code a wrapper my_atoi_with_error_check() around
atoi and call this new one everywhere else? Is it a good practice (If
it is, why ANSI atoi doesn't do it)?

/* <quote file=../net-snmp-5.4.1.1/snmplib/vacm.c> */

286 char *
287 _vacm_parse_config_access_common(struct vacm_accessEntry
**aptr, char *line)
288 {
289 struct vacm_accessEntry access;
290 char *cPrefix = (char *) &access.contextPrefix;
291 char *gName = (char *) &access.groupName;
292 size_t len;
293
294 access.status = atoi(line);
295 line = skip_token(line);
296 access.storageType = atoi(line);
297 line = skip_token(line);
298 access.securityModel = atoi(line);
299 line = skip_token(line);
300 access.securityLevel = atoi(line);
301 line = skip_token(line);
302 access.contextMatch = atoi(line);
303 line = skip_token(line);
304 len = sizeof(access.groupName);
305 line = read_config_read_octet_string(line, (u_char **)
&gName, &len);
306 len = sizeof(access.contextPrefix);
307 line = read_config_read_octet_string(line, (u_char **)
&cPrefix, &len);
308
309 *aptr = vacm_getAccessEntry(access.groupName,
310 access.contextPrefix,
311 access.securityModel,
312 access.securityLevel);
313 if (!*aptr)
314 *aptr = vacm_createAccessEntry(access.groupName,
315 access.contextPrefix,
316 access.securityModel,
317 access.securityLevel);
318 if (!*aptr)
319 return NULL;
320
321 (*aptr)->status = access.status;
322 (*aptr)->storageType = access.storageType;
323 (*aptr)->securityModel = access.securityModel;
324 (*aptr)->securityLevel = access.securityLevel;
325 (*aptr)->contextMatch = access.contextMatch;
326 return line;
327 }

/* </quote> */

 

[Ian Collins]

Do you check all error conditions for all library calls in you code?

Is it necessary to check all errors? Is it convenient to check all
errors (or how to make code clean and readable with mass of non
business logic related code block)?

The following code from net-snmp library calls atoi five times but
checks none. Do we code a wrapper my_atoi_with_error_check() around
atoi and call this new one everywhere else? Is it a good practice (If
it is, why ANSI atoi doesn't do it)?

There is no way to check the result of atoi, that's why it's better to
use strtol in most situations.

 

[lovecreatesbea...]

There is no way to check the result of atoi, that's why it's better to
use strtol in most situations.

I did not just mean check the return code from a function call.

Especially, I'm asking how to do complicated error check neatly.

 

[Ian Collins]

I did not just mean check the return code from a function call.

Especially, I'm asking how to do complicated error check neatly.

Use a language with exceptions

 

[lovecreatesbea...]

There is no way to check the result of atoi, that's why it's better to
use strtol in most situations.

I ever thought there're errno for atoi also. Thanks for reminder.

 

[Nick Keighley]

I did not just mean check the return code from a function call.

Especially, I'm asking how to do complicated error check neatly.

as someone else suggested, use exceptions.

Out of interest I've coded simple TCP/IP applications
in both C and Python. The Python versions were shorter
and easier to follow.

I somtimes use an exception like style in C

Rv read_box (Db_handle dbh, Box* box, Box_id box_id)
{
Rv rv;
int i;

for (i = 1; i < SHELF_COUNT; i++)
{
if ((rv = read_shelf (dbh, box->shelf, box_id, i)) != OK)
return rv;
}

return OK;
}

This was wrappered in macros so the code looked more like:

Rv read_box (Db_handle dbh, Box* box, Box_id box_id)
{
int i;

for (i = 1; i < SHELF_COUNT; i++)
{
read_shelf (dbh, box->shelf, box_id, i);
CHECK_ERROR;
}

return OK;
}

It was reasonably easy to follow and reasonably
easy to verify that all the error checking was done.


--
Nick Keighley

"Object-oriented programming is an exceptionally bad idea
that could only have originated in California."
Dijkstra

 

[lovecreatesbea...]

(e-mail address removed) said:


It has been said that you should never check for a condition you don't know
how to handle. But apart from that, check everything.

Sometimes, I omit some error check to come up with a can-work module
rapidly. Those error code will be supplied after some bug report
thrown out. This approach helps to catch up the schedule.

Is it useful to know whether your program worked? If not, then the program
itself is probably not all that useful.

Thanks for being patient.

Feel free to explain how you'd check atoi. Once you've failed to do that,
you will be in a position to explain why the code is broken.

That snippet is radomly selected. They even do not check the malloc
for three times, see Line 55, 59 and 64 below.

Dude, was it you who wrote atoi piece plus this one?


/* <quote file=../net-snmp-5.4.1.1/agent/auto_nlist.c> */
34 long
35 auto_nlist_value(const char *string)
36 {
37 struct autonlist **ptr, *it = 0;
38 int cmp;
39
40 if (string == 0)
41 return 0;
42
43 ptr = &nlists;
44 while (*ptr != 0 && it == 0) {
45 cmp = strcmp((*ptr)->symbol, string);
46 if (cmp == 0)
47 it = *ptr;
48 else if (cmp < 0) {
49 ptr = &((*ptr)->left);
50 } else {
51 ptr = &((*ptr)->right);
52 }
53 }
54 if (*ptr == 0) {
55 *ptr = (struct autonlist *) malloc(sizeof(struct
autonlist));
56 it = *ptr;
57 it->left = 0;
58 it->right = 0;
59 it->symbol = (char *) malloc(strlen(string) + 1);
60 strcpy(it->symbol, string);
61 /*
62 * allocate an extra byte for inclusion of a preceding
'_' later
63 */
64 it->nl[0].n_name = (char *) malloc(strlen(string) +
2);
65 #if defined(aix4) || defined(aix5)
66 strcpy(it->nl[0].n_name, string);
67 #else
68 sprintf(it->nl[0].n_name, "_%s", string);
69 #endif
70 it->nl[1].n_name = 0;
71 init_nlist(it->nl);
72 #if !(defined(aix4) || defined(aix5))
73 if (it->nl[0].n_type == 0) {
74 strcpy(it->nl[0].n_name, string);
75 init_nlist(it->nl);
76 }
77 #endif
78 if (it->nl[0].n_type == 0) {
79 if (!
netsnmp_ds_get_boolean(NETSNMP_DS_APPLICATION_ID,
80
NETSNMP_DS_AGENT_NO_ROOT_ACCESS)) {
81 snmp_log(LOG_ERR, "nlist err: neither %s nor _
%s found.\n",
82 string, string);
83 }
84 return (-1);
85 } else {
86 DEBUGMSGTL(("auto_nlist:auto_nlist_value", "found
symbol %s at %x.\n",
87 it->symbol, it->nl[0].n_value));
88 return (it->nl[0].n_value);
89 }
90 } else
91 return (it->nl[0].n_value);
92 }

/* </quote> */

 

[Nick Keighley]

Sometimes, I omit some error check to come up with a can-work module
rapidly. Those error code will be supplied after some bug report
thrown out. This approach helps to catch up the schedule.

not in my experience! I'd much rather put the checks in at the
beginning.
And if you had the checks you wouldn't need to debug- your program
would tell you what was wrong.

<snip>

 

[lovecreatesbea...]

not in my experience! I'd much rather put the checks in at the
beginning.
And if you had the checks you wouldn't need to debug- your program
would tell you what was wrong.

Yes, your way is right way.

 

[James Kuyper]

....
Sometimes, I omit some error check to come up with a can-work module
rapidly. Those error code will be supplied after some bug report
thrown out. This approach helps to catch up the schedule.

Only in the short term (and not even necessarily in the short term). In
the long run, you'll spend more time dealing with the bug reports and
fixing the problem than it would have taken to do it right in the first
place. Even in the short term, there's a good chance that your can-work
module won't work, and that you'll end up missing your schedule anyway
because of a bug that would have been much easier to track down if you'd
included proper error checking in the first place.

For many years, I held the opinion that if I adequately warn my boss of
the danger of skipping testing for errors, I've met my responsibilities.
If he responds by ordering me to go ahead and skip testing despite those
dangers, anything that goes wrong because of that is his responsibility,
not mine. I quickly learned, the hard way, that if I want to take that
attitude, I should always insist on getting such orders in writing. It
took me much longer to learn (again, the hard way) that I should never
obey such orders, period. I don't refuse the orders, I just ignore them.

 

[James Kuyper]

Do you check all error conditions for all library calls in you code?

For any function call that can fail, I make sure that my code is written
to deal with the possibility that it did fail. That usually means
checking for failure with every function call. However, for things like
writing to a stream, I can do a lot of function calls that write to the
file, and then use ferror() once to see if any of them failed. I almost
never check the return value of printf(), for precisely this reason.

What makes that approach feasible is the fact that, for my typical
program, it doesn't matter much which write to the stream failed; if any
of them failed, it's a fatal error, and there's only a minor cost to
failing late rather than failing immediately. For many people's
programs, that is not true - when writing to a pipe or writing to a
device, it's sometimes important to react immediately to any failure.

This approach is NOT appropriate when reading data from a stream. If the
data was not read in, the contents of the buffer I read it into are
unpredictable, and almost certainly not what the rest of my code expects
it to be. I need to choose an alternate path through my code as soon as
I detect the error.

Is it necessary to check all errors?

No, but it's needed more often than some lazy programmers think it is.
In the long run, it's easier to make a habit of checking every call that
could fail, than to try to figure out for each call whether or not it
really needs to be checked. If you follow the second approach, you'll
almost certainly find yourself debugging a mysterious error that will
end up tracing back to the unnoticed failure of a function call.

... Is it convenient to check all
errors (or how to make code clean and readable with mass of non
business logic related code block)?

No, it is not convenient. There's a real danger of making your code look
like it's main purpose is to produce error messages. I once had a
subordinate who handed me a test plan for a function which triggered
every single error message the function could generate, but which didn't
have even a single test case that didn't generate an error message. His
test plan didn't bother checking whether the program actually performed
the task it was supposed to perform when given non-erroneous input!

 

[James Kuyper]

Not if I don't have any alternative action that I want to perform.
If printf("hello world\n") returns EOF,
then I really don't have any alternative action for that.

Even if all other modes of reporting an error are shut down, you can
always exit with an exit status of EXIT_FAILURE. That method of
reporting might be disabled too, but there's no way from within portable
code to determine that, so you might as well try.

 

[Richard Tobin]

Even if all other modes of reporting an error are shut down, you can
always exit with an exit status of EXIT_FAILURE.

What if exit() unexpectedly returns?

-- Richard

 

[Andrew Poelstra]

I did not just mean check the return code from a function call.

Especially, I'm asking how to do complicated error check neatly.

I use goto.

if(!(mem1 = malloc(sizeof *mem1)) ||
!(mem2 = malloc(sizeof *mem2)) ||
!(mem3 = malloc(sizeof *mem3)))
{
err_code = OUT_OF_MEMORY;
goto fail;
}

if(!(infile = fopen("./in.txt", "r")) ||
!(outfile = fopen("./out.txt", "w")))
{
err_code = FILE_ERROR;
goto fail;
}

/* logic below */

return 0;

fail:
free(mem1); /* These would be initialized to NULL
free(mem2); in case their malloc() or fopen()
free(mem3); never gets called. */
fclose(infile);
fclose(outfile);
return err_code;

 

[CBFalconer]

Do you check all error conditions for all library calls in you code?

Is it necessary to check all errors? Is it convenient to check all
errors (or how to make code clean and readable with mass of non
business logic related code block)?

The following code from net-snmp library calls atoi five times but
checks none. Do we code a wrapper my_atoi_with_error_check() around
atoi and call this new one everywhere else? Is it a good practice
(If it is, why ANSI atoi doesn't do it)?

Don't use atoi. Use one of strtod, strtold, strtou, which have
good error detection facilities. atoi is only there to handle old
code, written before the strto functions were available.

atoi doesn't do it because revising it would strand the old
software.

 

[Richard Tobin]

What if exit() unexpectedly returns?

[/QUOTE]

Call it again. In fact, call it in a loop. The program is bound to notice
eventually.

What if the loop unexpectedly terminates... oh never mind.

-- Richard

 

[James Kuyper]

What if exit() unexpectedly returns?

I'd expect a smiley on such a statement. In the absence of one, I'll
treat it seriously. Many of the standard library routines are defined in
a way that allows them to fail even if used perfectly correctly, and
provide mechanisms whereby such failures can be detected. Those
mechanisms should be used. However,

7.20.4.3p6: "The exit function cannot return to its caller."

The possibility of exit() returning to its caller is not worth worrying
about.

I won't say it can't happen, but worrying about the possibility is no
more reasonable than worrying about the possibility that any other
feature of C has been implemented incorrectly.

 

[Ben Bacarisse]

I use goto.

if(!(mem1 = malloc(sizeof *mem1)) ||
!(mem2 = malloc(sizeof *mem2)) ||
!(mem3 = malloc(sizeof *mem3)))
{
err_code = OUT_OF_MEMORY;
goto fail;
}

if(!(infile = fopen("./in.txt", "r")) ||
!(outfile = fopen("./out.txt", "w")))
{
err_code = FILE_ERROR;
goto fail;
}

/* logic below */

return 0;

fail:
free(mem1); /* These would be initialized to NULL
free(mem2); in case their malloc() or fopen()
free(mem3); never gets called. */
fclose(infile);
fclose(outfile);

fclose(NULL) is undefined. These two need to be protected with an if
each. Such a shame that there is no symmetry with malloc/free but
that is how the world is!

 

[lovecreatesbea...]

Thanks Ian and Richard for mentioning the weakness of atoi also.

Don't use atoi.  Use one of strtod, strtold, strtou, which have
good error detection facilities.  atoi is only there to handle old
code, written before the strto functions were available.

atoi doesn't do it because revising it would strand the old
software.

Yes, thanks for this information.

 

[lovecreatesbea...]

Only in the short term (and not even necessarily in the short term). In
the long run, you'll spend more time dealing with the bug reports and
fixing the problem than it would have taken to do it right in the first
place. Even in the short term, there's a good chance that your can-work
module won't work, and that you'll end up missing your schedule anyway
because of a bug that would have been much easier to track down if you'd
included proper error checking in the first place.

For many years, I held the opinion that if I adequately warn my boss of
the danger of skipping testing for errors, I've met my responsibilities.
If he responds by ordering me to go ahead and skip testing despite those
dangers, anything that goes wrong because of that is his responsibility,
not mine. I quickly learned, the hard way, that if I want to take that
attitude, I should always insist on getting such orders in writing. It
took me much longer to learn (again, the hard way) that I should never
obey such orders, period. I don't refuse the orders, I just ignore them.

(My boss and ex-bosses don't know about C programming and don't care
about C programming. I'm supposed to show them some instant
achievement same as other humble programmers in China do. My bosses do
the same instant things to the clients.)

Thanks you for the advice, it helps me.