Error handling library

[Andrew Poelstra]

I hammered this out this morning to fix inconsistancies with the way my
programs handle errors. The code itself is fine, in that it compiles with
Richard Heathfield's gcc tags (plus -c because it doesn't have a main).

Any comments?

/* Start of header */
#ifndef _ERROR_H_
#define _ERROR_H_

#include <stdlib.h> /* Needed for size_t */

/* Diagnostic severity */
#define ADL_INFO 0
#define ADL_WARN 1
#define ADL_BENIGN 2
#define ADL_FATAL 3

struct error_t;
struct error_l;

int adl_set_buffer (size_t size);
int adl_register_error (char *file, char *error, int line, int fatal);
void adl_terminate (int display);

struct error_t
{
char *text;
char *file;
int line;
int severity;
};

struct error_l
{
struct error_t *err;
size_t ctr;
size_t max;
int wrap;
};

const char *adl_message[] = {"Info", "Warning", "Error", "Fatal error"};
struct error_l *adl_err = NULL;

#endif
/* End of header */

/* Start of source error.c */
#include "error.h"
#include <stdio.h>
#include <stdlib.h>


int adl_set_buffer (size_t size)
{
adl_err = malloc (sizeof *adl_err);
if (adl_err == NULL)
return -1;

adl_err->err = malloc (sizeof *adl_err->err * size);
adl_err->max = size;
adl_err->ctr = 0;

if (adl_err->err == NULL)
{
free (adl_err);
adl_err = NULL;
return -1;
}

return 0;
}


int adl_register_error (char *file, char *error, int line, int sev)
{
unsigned i = adl_err->ctr; /* This is here for readability only. */

if (adl_err == NULL)
return -1;

adl_err->err.text = error;
adl_err->err.file = file;
adl_err->err.line = line;
adl_err->err.severity = sev;

i++;

if (i > adl_err->max)
{
adl_err->ctr = 0;
adl_err->wrap = 1;
}
else
adl_err->ctr = i;

return 0;
}


void adl_terminate (int display)
{
unsigned ctr;
struct error_t *cur; /* Replaces adl_err->err[ctr] for readability. */

/* If we aren't writing anything to the screen, simply terminate. */
if (adl_err == NULL || !display)
exit (EXIT_FAILURE);

/* If we've wrapped, start immediately past last error. */
ctr = adl_err->wrap ? (adl_err->ctr + 1) : 0;

/* Loop ends when we reach last error */
while (ctr != adl_err->ctr)
{
cur = &adl_err->err[ctr];
printf ("%s: %s (line %d in %s).\n",
adl_message[cur->severity],
cur->text, cur->line, cur->file);

ctr++;
if (ctr == adl_err->max) /* We need this if we're wrapping*/
ctr = 0;
}
}
/* End of source */

 

[pete]

I hammered this out this morning
to fix inconsistancies with the way my
programs handle errors. The code itself is fine,
in that it compiles with
Richard Heathfield's gcc tags
(plus -c because it doesn't have a main).

Any comments?

/* Start of header */
#ifndef _ERROR_H_
#define _ERROR_H_

N869
7.1.3 Reserved identifiers
[#1] Each header declares or defines all identifiers listed
in its associated subclause, and optionally declares or
defines identifiers listed in its associated future library
directions subclause and identifiers which are always
reserved either for any use or for use as file scope
identifiers.
-- All identifiers that begin with an underscore and
either an uppercase letter or another underscore are
always reserved for any use.

#include <stdlib.h> /* Needed for size_t */

You can use <stddef.h> instead of <stdlib.h>,
if you want more minimalism.

 

[Andrew Poelstra]

I hammered this out this morning
to fix inconsistancies with the way my
programs handle errors. The code itself is fine,
in that it compiles with
Richard Heathfield's gcc tags
(plus -c because it doesn't have a main).

Any comments?

/* Start of header */
#ifndef _ERROR_H_
#define _ERROR_H_

N869
7.1.3 Reserved identifiers
[#1] Each header declares or defines all identifiers listed
in its associated subclause, and optionally declares or
defines identifiers listed in its associated future library
directions subclause and identifiers which are always
reserved either for any use or for use as file scope
identifiers.
-- All identifiers that begin with an underscore and
either an uppercase letter or another underscore are
always reserved for any use.

I knew the double-underscore rule. I didn't know about the underscore-
capital rule. Oops.

You can use <stddef.h> instead of <stdlib.h>,
if you want more minimalism.

Thanks!

 

[pete]

I hammered this out this morning
to fix inconsistancies with the way my
programs handle errors. The code itself is fine,
in that it compiles with
Richard Heathfield's gcc tags
(plus -c because it doesn't have a main).

Any comments?

/* Start of header */
#ifndef _ERROR_H_
#define _ERROR_H_

N869
7.1.3 Reserved identifiers
[#1] Each header declares or defines all identifiers listed
in its associated subclause, and optionally declares or
defines identifiers listed in its associated future library
directions subclause and identifiers which are always
reserved either for any use or for use as file scope
identifiers.
-- All identifiers that begin with an underscore and
either an uppercase letter or another underscore are
always reserved for any use.

I knew the double-underscore rule. I didn't know about the underscore-
capital rule. Oops.

I'm currently using this form: H_ERROR_H
for header guards.

It's easier to not create any identifiers with a leading
underscore than it is to memorize
the complete set of rules regarding leading underscores.

 

[Bill Pursell]

I hammered this out this morning to fix inconsistancies with the way my
programs handle errors. The code itself is fine, in that it compiles with
Richard Heathfield's gcc tags (plus -c because it doesn't have a main).

Any comments?

Couple of thoughts:

It's not clear what is meant by "ADL". I'm guessing
it's an abbreviation of "additional". A comment clarifying
that would be nice.

I found it surprising that adl_set_buffer() called malloc.
"set" doesn't imply "create" to me.

You might want to flush the output at the end of
adl_terminate.

 

[Andrew Poelstra]

Couple of thoughts:

It's not clear what is meant by "ADL". I'm guessing
it's an abbreviation of "additional". A comment clarifying
that would be nice.

Did I forget that? It means Andrew's Diagnostic Library.

I found it surprising that adl_set_buffer() called malloc.
"set" doesn't imply "create" to me.

It "set"s the buffer for the program. I might change it to
"init".

You might want to flush the output at the end of
adl_terminate.

Doesn't the \n do that for me? That is, even if there
is unflushed stuff in the buffer, won't it be printed
along with the first diagnostic?

 

[Michael Mair]

I hammered this out this morning to fix inconsistancies with the way my
programs handle errors. The code itself is fine, in that it compiles with
Richard Heathfield's gcc tags (plus -c because it doesn't have a main).

Any comments?

/* Start of header */
#ifndef _ERROR_H_
#define _ERROR_H_

#include <stdlib.h> /* Needed for size_t */

/* Diagnostic severity */
#define ADL_INFO 0
#define ADL_WARN 1
#define ADL_BENIGN 2
#define ADL_FATAL 3

ADL? AdvancedDiagnosticsLibrary?
Make it clear in a comment or similar.

Note: I'd have not only a "severity" but a "visibility"
axis -- this gives you a chance to hide causes of fatal
errors or unnecessary infos from the user in "normal" mode
and see what is really wrong in "show all" mode.

Consequently, all "internal fatals" must be replaced by
a default "external fatal"; "internal errors" should also
be mapped to a default "external fatal" -- otherwise, the
user does not know that there was an error.
I'd not make the errors "wrap around" but have a maximal
error threshold which leads to a fatal and instant orderly
termination of the program.

struct error_t;
struct error_l;

int adl_set_buffer (size_t size);
int adl_register_error (char *file, char *error, int line, int fatal);
void adl_terminate (int display);

struct error_t
{
char *text;
char *file;
int line;
int severity;
};

struct error_l
{
struct error_t *err;
size_t ctr;
size_t max;
int wrap;
};

error_t and error_l are not exactly identifiers that tell
the whole story -- err_text and err_list (if I guessed correctly)
are "better" in this respect.

const char *adl_message[] = {"Info", "Warning", "Error", "Fatal error"};
struct error_l *adl_err = NULL;

This does not belong in the header.
extern const char **adl_message;
extern struct error_l *adl_err;
an appropriate .c file parts would be better -- and I am not
sure why these should be visible outside (I'd not even show
struct error_l to the user).

#endif
/* End of header */

/* Start of source error.c */
#include "error.h"
#include <stdio.h>
#include <stdlib.h>


int adl_set_buffer (size_t size)
{

}


int adl_register_error (char *file, char *error, int line, int sev)
{

i++;

if (i > adl_err->max)
{
adl_err->ctr = 0;
adl_err->wrap = 1;
}
else
adl_err->ctr = i;

I'd rather see an expansion than a wrap; the first error may be
the one which tells most -- overwriting it is not exactly smart.<snip>

Cheers
Michael

 

[goose]

I hammered this out this morning to fix inconsistancies with the way my
programs handle errors. The code itself is fine, in that it compiles with
Richard Heathfield's gcc tags (plus -c because it doesn't have a main).

Any comments?

/* Start of header */
#ifndef _ERROR_H_
#define _ERROR_H_

Aother poster here brought this up; I'd suggest
just prefix H_ ont your header names for this; its much
easier than memorising the rules for #define.

#include <stdlib.h> /* Needed for size_t */

/* Diagnostic severity */
#define ADL_INFO 0
#define ADL_WARN 1
#define ADL_BENIGN 2
#define ADL_FATAL 3

1. I don't really know why this is #defined; for a type
that needs to hold a limited range of values, C provides
enum (although I suppose that its a matter of taste?)
which lets the compiler catch out-of-range usage.

For example, if the above was an enum:
the compiler will prevent a caller from passing 5 to
register_error for the argument fatal/sev (see point #2).
Since you never check that "severity" is in range before
you use it as an index into an array, you run the very real
risk of a programmer checking the prototype of the function
and assuming that a higher int means "more dangerous error"
(true up to a point with your values) and merely upping the
int value of the argument without realising that there is
a limit (or even #defined values).

struct error_t;
struct error_l;

int adl_set_buffer (size_t size);
int adl_register_error (char *file, char *error, int line, int fatal);

2. The prototype for register_error differs in the name of
the last argument; I doubt that any harm will come from this
but you make the maintainers life harder (eg. automated methods
for finding a prototype from a function definition or vice
versa may not work as they differ).

void adl_terminate (int display);

struct error_t
{
char *text;
char *file;
int line;
int severity;
};

struct error_l
{
struct error_t *err;
size_t ctr;
size_t max;
int wrap;
};

const char *adl_message[] = {"Info", "Warning", "Error", "Fatal error"};

3. I'd change this (together with defining an enum above) so that
the lookup table and the range are are textually close together
with a comment warning that one is used as an index into the array
(which is the other one). I'll put an example down below at the end
of this post. On a related note, do you think that defining an array
in a header is a good idea? All the modules which include this header
will have their own copy of the array but only this module will
ever use its copy of the array.

struct error_l *adl_err = NULL;

#endif
/* End of header */

/* Start of source error.c */
#include "error.h"

3.5 (bit of a nitpick only) It's generally accepted to
put standard headers first, then system headers and then
local (to the project) headers.

#include <stdio.h>
#include <stdlib.h>


int adl_set_buffer (size_t size)
{
adl_err = malloc (sizeof *adl_err);
if (adl_err == NULL)
return -1;

adl_err->err = malloc (sizeof *adl_err->err * size);
adl_err->max = size;
adl_err->ctr = 0;

if (adl_err->err == NULL)
{
free (adl_err);
adl_err = NULL;
return -1;
}

return 0;
}

4. Seeing as how you are only returning one of two values
in this function, you might make that clear to the maintainer
by making return type of this function a bool. You would
lose nothing but would gain some clarity for the reader.

int adl_register_error (char *file, char *error, int line, int sev)
{
unsigned i = adl_err->ctr; /* This is here for readability only. */

if (adl_err == NULL)
return -1;

adl_err->err.text = error;
adl_err->err.file = file;
adl_err->err.line = line;
adl_err->err.severity = sev;

i++;

if (i > adl_err->max)
{
adl_err->ctr = 0;
adl_err->wrap = 1;
}
else
adl_err->ctr = i;

return 0;
}

5. See point #4 above.

void adl_terminate (int display)

6. This is not properly named (much like your other functions
as was pointed out by other posters) because it won't
always terminate, see? Why not make two different functions,
one that does termination and one that does display?

{
unsigned ctr;
struct error_t *cur; /* Replaces adl_err->err[ctr] for readability. */

/* If we aren't writing anything to the screen, simply terminate. */
if (adl_err == NULL || !display)
exit (EXIT_FAILURE);

/* If we've wrapped, start immediately past last error. */
ctr = adl_err->wrap ? (adl_err->ctr + 1) : 0;

/* Loop ends when we reach last error */
while (ctr != adl_err->ctr)
{
cur = &adl_err->err[ctr];
printf ("%s: %s (line %d in %s).\n",
adl_message[cur->severity],
cur->text, cur->line, cur->file);

ctr++;
if (ctr == adl_err->max) /* We need this if we're wrapping*/
ctr = 0;
}
}
/* End of source */

Right. Seeing that a lookup table for user messages is something
that you will likely need in most non-trivial projects, you can
try something like this (I'm using your library as an example):


Warning: I've typeed this straight into my web-browser,
it has not been compiled or tested in any way.

/* ------------- start Header file ------------- */
#ifndef H_ERROR
#define H_ERROR

enum error_type_t {
ADL_INFO = 0,
ADL_WARN,
ADL_BENIGN,
ADL_FATAL,
/* New error types can be added here only
* if the corresponding entry in adl_reror_lookup()
* is modified as well. For every entry here, there
* must be one in adl_error_lookup() as well.
*/
ADL_RANGE
};

char *adl_error_lookup (error_type_t err);

#endif
/* ------------- end Header file ------------- */

/* ------------- start Source file ------------- */
/* First include all your standard headers here */

/* Include the system headers here */

#include "error.h"

char *adl_error_lookup (error_type_t err)
{
static char *message[] = {
"Info",
"Warning",
"Error",
"Fatal error",
/* New entries must be added in here. For every entry
* in here there must a corresponding entry in the
* definition of error_type_t.
*/
"Unknown",
};

return message[err];
}

/* ------------- end Source file ------------- */

See, the advantage here is we don't even need to check that
the argument in adl_error_lookup() is within the range of
the messages in the array, the compiler will do that auto-
matically for us. Also note the comment I've placed at the
two points so that the maintainer *knows* that he must
modify something else when he adds new messages or new error
types.

You can modify this to easily fit something else; I'm
using this to store *all* messages directed at the user
for an embedded application so that I can easily swap
languages while running (an array of languages, each
consisting of the above array of messages; array of
languages is indexed with a variable "curr_lang" which is
of an enum type with range of only the languages supported).

With care (and a filesystem on your platform), you can
have the above method change languages with the languages
stored on your filesystem instead of in memory.


HTH
goose

 

[Andrew Poelstra]

1. I don't really know why this is #defined; for a type
that needs to hold a limited range of values, C provides
enum (although I suppose that its a matter of taste?)
which lets the compiler catch out-of-range usage.

I've been using #define's by habit, simply because they're
very easy to vertically align (and convert to 0, 0x1, 0x2,
0x4, 0x8i, 0x10... if I want to be able to OR multiple values
together). I'll switch this one to enum, and hopefully
remember to do so in the future.

2. The prototype for register_error differs in the name of
the last argument; I doubt that any harm will come from this
but you make the maintainers life harder (eg. automated methods
for finding a prototype from a function definition or vice
versa may not work as they differ).

As you might have guessed, the function originally took a binary
fatal/nonfatal argument, and I soon realized that I would need a
few more in-between levels.

const char *adl_message[] = {"Info", "Warning", "Error", "Fatal error"};

3. I'd change this (together with defining an enum above) so that
the lookup table and the range are are textually close together
with a comment warning that one is used as an index into the array
(which is the other one). I'll put an example down below at the end
of this post.

I noted your example, and will take it into account. I originally didn't
do stuff like that because the entire library was only about 100 lines
and maintaining it wouldn't be difficult.

Now I've noted a ton of new features I need to add (clearing the buffer,
allowing infinite buffers, displaying to an arbitrary stream, ...)

On a related note, do you think that defining an array
in a header is a good idea? All the modules which include this header
will have their own copy of the array but only this module will
ever use its copy of the array.

I've noted your static function below. I wasn't sure how to avoid that
problem, because a regular function would allocate memory /every time
it is called/. I didn't think of static, though. Thanks!

3.5 (bit of a nitpick only) It's generally accepted to
put standard headers first, then system headers and then
local (to the project) headers.

I generally avoid system headers in libraries (optimially, I'd like to
be able to port without any changes). I'll move the headers, though,
because maintainability is a large concern in such an extensible
library.

4. Seeing as how you are only returning one of two values
in this function, you might make that clear to the maintainer
by making return type of this function a bool. You would
lose nothing but would gain some clarity for the reader.

I would need to define my own bool type, wouldn't I? (Or I
could use C99 and hope for the best).

6. This is not properly named (much like your other functions
as was pointed out by other posters) because it won't
always terminate, see? Why not make two different functions,
one that does termination and one that does display?

It was supposed to always terminate. I forgot the exit(0) at the end
of the display code. I will separate the functions, though.

Thanks for all of your comments and suggestions, goose. I'll be sure
to implement them as soon as possible.

 

[Bill Pursell]

Doesn't the \n do that for me? That is, even if there
is unflushed stuff in the buffer, won't it be printed
along with the first diagnostic?

I'm not certain that calling fprintf with a '\n' is
guaranteed to flush the buffer. My thinking
is that error conditions are rare (hopefully),
and the diagnostic is important, so it's
better safe than sorry.

 

[pete]

Bill Pursell wrote:

I'm not certain that calling fprintf with a '\n' is
guaranteed to flush the buffer.

It isn't.

N869
7.19.3 Files

[#3] When a stream is unbuffered, characters are intended to
appear from the source or at the destination as soon as
possible. Otherwise characters may be accumulated and
transmitted to or from the host environment as a block.
When a stream is fully buffered, characters are intended to
be transmitted to or from the host environment as a block
when a buffer is filled. When a stream is line buffered,
characters are intended to be transmitted to or from the
host environment as a block when a new-line character is
encountered. Furthermore, characters are intended to be
transmitted as a block to the host environment when a buffer
is filled, when input is requested on an unbuffered stream,
or when input is requested on a line buffered stream that
requires the transmission of characters from the host
environment. Support for these characteristics is
implementation-defined, and may be affected via the setbuf
and setvbuf functions.

 

[Mark McIntyre]

I'm not certain that calling fprintf with a '\n' is
guaranteed to flush the buffer.

Only if it is line buffered or unbuffered - see 7.19.3(3). This
typically only applies to stdout.
--
Mark McIntyre

"Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it."
--Brian Kernighan