Simple logging library in C

Question 1

This header implements a very simple set of C (only) functions for logging. This is part of a larger collection of utility functions aimed to be used during the development process, meaning that they are intended to provide quick, easy to use solutions that could be replaced in the production code if needed.

To ease with reading the code, I've removed the (rather verbose) comments with the documentation. You can find it in the README.md file here.

Beside the printf()-like functions, there are a couple of functions that may be used for testing and debugging purpose.

I'm very interested in any feedback you may have, especially in the area of usability.

#ifndef UTL_H
#define UTL_H
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <stdarg.h>
#include <stddef.h>
#include <time.h>
#include <ctype.h>
#ifndef UTL_NOLOG
#define logprintf(...) utl_log_printf(__VA_ARGS__)
#define logclose() utl_log_close("LOG STOP")
#define logopen(f,m) utl_log_open(f,m)
#ifndef NDEBUG 
#define logcheck(e) utl_log_check(!!(e),#e,__FILE__,__LINE__)
#define logassert(e) utl_log_assert(!!(e),#e,__FILE__,__LINE__)
#define logdebug logprintf
#else 
#define logcheck(e) utl_log_one()
#define logassert(e) ((void)0)
#define logdebug(...) ((void)0)
#endif 
#define _logprintf(...) ((void)0)
#define _logdebug(...) ((void)0)
#define _logcheck(...) utl_log_one()
#define _logassert(...) ((void)0)
#define _logopen(f,m) ((void)0)
#define _logclose() ((void)0)
void utl_log_close(char *msg);
void utl_log_open(char *fname, char *mode);
int utl_log_check(int res, char *test, char *file, int line);
void utl_log_assert(int res, char *test, char *file, int line);
void utl_log_printf(char *format, ...);
int utl_log_one(void);
#ifdef UTL_MAIN
static FILE *utl_log_file = NULL;
void utl_log_close(char *msg)
{
 if (msg) logprintf(msg);
 if (utl_log_file && utl_log_file != stderr) fclose(utl_log_file);
 utl_log_file = NULL;
}
void utl_log_open(char *fname, char *mode)
{
 char md[2];
 md[0] = (mode && *mode == 'w')? 'w' : 'a'; md[1] = '0円';
 utl_log_close(NULL);
 utl_log_file = fopen(fname,md);
 logprintf("LOG START");
}
void utl_log_printf(char *format, ...)
{
 va_list args;
 char log_tstr[32];
 time_t log_time;
 if (!utl_log_file) utl_log_file = stderr;
 time(&log_time);
 strftime(log_tstr,32,"%Y-%m-%d %X",localtime(&log_time));
 fprintf(utl_log_file,"%s ",log_tstr);
 va_start(args, format);
 vfprintf(utl_log_file, format, args);
 va_end(args);
 fputc('\n',utl_log_file);
 fflush(utl_log_file);
}
int utl_log_check(int res, char *test, char *file, int line)
{
 logprintf("CHK %s (%s) %s:%d", (res?"PASS":"FAIL"), test, file, line);
 return res;
}
void utl_log_assert(int res, char *test, char *file, int line)
{
 if (!utl_log_check(res,test,file,line)) {
 logprintf("CHK EXITING ON FAIL");
 logclose();
 exit(1);
 }
}
int utl_log_one() {return 1;} /* to avoid warnings */
#endif /* UTL_MAIN */
#endif /* UTL_NOLOG */
#endif /* UTL_H */

Question 2

Exit
There should never be a call to exit() in a library. If for example this code was included in a daemon or some other part of an operating system you would be shutting down the system without meaning to. The best you could do here is to require that a setjmp() command is used in the linking program and do a longjump() from the assert function. This allows the calling program to clean up and perform the correct actions in this case. A library will never know what other actions are necessary.

Incorrect Usage of Header Files
Header files should generally not have executable functions in them. Part of the reason is that if multiple files include the header then at link time the linker will complain about multiple definitions of the functions.

You seem to have worked around the multiple definition problem with complex #ifdef statements. It would be much simpler to have both a utl.h and and a utl.c. The utl.c file contains the executable functions and the utl.h file contains the function prototypes. This is a very common practice and could be considered a standard. This would also remove the requirement for most of the header files included by this header file.

If you are creating a library that may be used by multiple programs you should probably turn utl.c into either an archive library (libmylog.a) or a dynamically linked library (libmylog.so or libmylog.dll based on the operating system).

Question 3

Thanks pacmaninbw. I'll think about the exit(). As for the mixing of code in the header, I was looking for making very simple to add these functions to a project and I thought of giving the possibility of just including the .h file (with the additional requirement of compiling one of the source filese with the UTL_MAIN symbol defined. I will be very interested in understanding if this is something that could be seen as "easier to do" rather than compiling and linking against another file. Your suggestion seems to indicate that this might not be the case and I might reconsider. Thanks!

Question 4

About exit(1). It seems that assert() (whose behaviour I'm trying to mimic) calls abort(). How would you consider this?

Question 5

About assert, I think you might want to look at this stackoverflow question stackoverflow.com/questions/8114008/…. If I were writing a daemon or other operating system library I would only compile with asserts enabled in unit testing. I always turn off asserts in production code.

Question 6

I agree, that's why logassert() disappears when compiled with NDEBUG defined (as assert() does).

Question 7

Unclear why code used a fixed Y-M-D order yet locale time representation with "%X". Suggest ISO 8601 and use "%04Y-%02m-%02dT%02H:%02D:%02SZ" and use gmtime().
Design lacks error checking. I'd expect void utl_log_printf(char *format, ...) and other functions to return int and each I/O function called should have its return value check and possibly the function should return early on error.
Design. If looking to detect development issues: flush before printing the ... variables, which being unqualified data, more likely to fail and code wants to get as much of the message logged as it can.
```
fflush(log_str); // add
va_start(args, format);
```

Minor:

Agree with @pacmaninbw points.

Why code 32 in 2 places. State the size once. Even better as a #define.

char log_tstr[32];
// strftime(log_tstr,32,"%Y-%m-%d %X",localtime(&log_time));
strftime(log_tstr, sizeof log_tstr,"%Y-%m-%d %X",localtime(&log_time));

Code does a lot of #define with identifiers beginning with _ like #define _logprintf(.... Those are reserved by the C implementation. "All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces." C11dr 7.1.3 3
Use const. If a function does not change referenced data, declare the functor with const. This allows code to pass const pointers and may allow the compiler to employ some optimizations otherwise not allowed.
```
// utl_log_close(char *msg)
utl_log_close(const char *msg)
```
~~(削除) Should test for the existence of w in the string, not if it is the first one in md[0] = (mode && *mode == 'w')? 'w' : 'a'; md[1] = '0円'; (削除ここまで)~~ a or w or others must be the first letter. Cancel this point.
32 is stingy. For me, I'd used the worst case that could be returned from strftime() and ISO 8601 which may be in the 40s or so (think 64-bit int year). If not certain, I would estimate worst case and double it.
```
// char log_tstr[32];
char log_tstr[100];
```
Error checking lacking on time(); and strftime().

Avoid 2 statements on a line.

char md[2];
// md[0] = (mode && *mode == 'w')? 'w' : 'a'; md[1] = '0円';
md[0] = (mode && *mode == 'w')? 'w' : 'a';
md[1] = '0円';

Good usage of surrounding user strings with printable text.

// ( )
logprintf("CHK %s (%s) %s:%d", (res?"PASS":"FAIL"), test, file, line);

FYI: Pedantic note: __LINE__ is a integer constant. It is not limited to the range of int. Perhaps use unsigned long line
The type used in assert(scalar expression) is scalar, not int. Consider the following. Calling utl_log_assert(int res0x100000000, ... may result in a test fail. Instead, use _Bool
```
assert(0x100000000); // 8 zeros
void utl_log_assert(_Bool res,
```
Unneeded #include in .h portion. Only #include files in the .h file that the .h file needs. Include in the .c file, the ones it needs.

Question 8

Thanks for you comments, chux. I had completely missed the %X and the other points in managing the time string. I don't quite get your point 3, could you clarify?

Question 9

Another question. Related to 14, consider that utl_log_assert is only to be called via log_assert() which means that the !!(x) is applied to its first param. This allows for things like logassert(p) where p is a pointer. Do you think point 14 is still relevant?

Question 10

8: My intention was to mimic the fopen() behaviour where the w (or a) should be the first character. Did I miss something?

Question 11

@Remo.D I agree with you - answer updated.

Question 12

@Remo D. "Arithmetic types and pointer types are collectively called scalar types." C11 §6.2.5 21, so #14 applies to pointers too should code call utl_log_assert(() directly.

pacmaninbw ♦pacmaninbw 26.2k13 gold badges47 silver badges113 bronze badges · Answer 1 · 2016-09-03 14:12:26Z

Exit
There should never be a call to exit() in a library. If for example this code was included in a daemon or some other part of an operating system you would be shutting down the system without meaning to. The best you could do here is to require that a setjmp() command is used in the linking program and do a longjump() from the assert function. This allows the calling program to clean up and perform the correct actions in this case. A library will never know what other actions are necessary.

Incorrect Usage of Header Files
Header files should generally not have executable functions in them. Part of the reason is that if multiple files include the header then at link time the linker will complain about multiple definitions of the functions.

You seem to have worked around the multiple definition problem with complex #ifdef statements. It would be much simpler to have both a utl.h and and a utl.c. The utl.c file contains the executable functions and the utl.h file contains the function prototypes. This is a very common practice and could be considered a standard. This would also remove the requirement for most of the header files included by this header file.

If you are creating a library that may be used by multiple programs you should probably turn utl.c into either an archive library (libmylog.a) or a dynamically linked library (libmylog.so or libmylog.dll based on the operating system).

Thanks pacmaninbw. I'll think about the exit(). As for the mixing of code in the header, I was looking for making very simple to add these functions to a project and I thought of giving the possibility of just including the .h file (with the additional requirement of compiling one of the source filese with the UTL_MAIN symbol defined. I will be very interested in understanding if this is something that could be seen as "easier to do" rather than compiling and linking against another file. Your suggestion seems to indicate that this might not be the case and I might reconsider. Thanks!
About exit(1). It seems that assert() (whose behaviour I'm trying to mimic) calls abort(). How would you consider this?
About assert, I think you might want to look at this stackoverflow question stackoverflow.com/questions/8114008/…. If I were writing a daemon or other operating system library I would only compile with asserts enabled in unit testing. I always turn off asserts in production code.
I agree, that's why logassert() disappears when compiled with NDEBUG defined (as assert() does).

chux chux 36.4k2 gold badges43 silver badges96 bronze badges · Answer 2 · 2016-09-04 05:37:19Z

Unclear why code used a fixed Y-M-D order yet locale time representation with "%X". Suggest ISO 8601 and use "%04Y-%02m-%02dT%02H:%02D:%02SZ" and use gmtime().
Design lacks error checking. I'd expect void utl_log_printf(char *format, ...) and other functions to return int and each I/O function called should have its return value check and possibly the function should return early on error.
Design. If looking to detect development issues: flush before printing the ... variables, which being unqualified data, more likely to fail and code wants to get as much of the message logged as it can.
```
fflush(log_str); // add
va_start(args, format);
```

Minor:

Agree with @pacmaninbw points.

Why code 32 in 2 places. State the size once. Even better as a #define.

char log_tstr[32];
// strftime(log_tstr,32,"%Y-%m-%d %X",localtime(&log_time));
strftime(log_tstr, sizeof log_tstr,"%Y-%m-%d %X",localtime(&log_time));

Code does a lot of #define with identifiers beginning with _ like #define _logprintf(.... Those are reserved by the C implementation. "All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces." C11dr 7.1.3 3
Use const. If a function does not change referenced data, declare the functor with const. This allows code to pass const pointers and may allow the compiler to employ some optimizations otherwise not allowed.
```
// utl_log_close(char *msg)
utl_log_close(const char *msg)
```
~~(削除) Should test for the existence of w in the string, not if it is the first one in md[0] = (mode && *mode == 'w')? 'w' : 'a'; md[1] = '0円'; (削除ここまで)~~ a or w or others must be the first letter. Cancel this point.
32 is stingy. For me, I'd used the worst case that could be returned from strftime() and ISO 8601 which may be in the 40s or so (think 64-bit int year). If not certain, I would estimate worst case and double it.
```
// char log_tstr[32];
char log_tstr[100];
```
Error checking lacking on time(); and strftime().

Avoid 2 statements on a line.

char md[2];
// md[0] = (mode && *mode == 'w')? 'w' : 'a'; md[1] = '0円';
md[0] = (mode && *mode == 'w')? 'w' : 'a';
md[1] = '0円';

Good usage of surrounding user strings with printable text.

// ( )
logprintf("CHK %s (%s) %s:%d", (res?"PASS":"FAIL"), test, file, line);

FYI: Pedantic note: __LINE__ is a integer constant. It is not limited to the range of int. Perhaps use unsigned long line
The type used in assert(scalar expression) is scalar, not int. Consider the following. Calling utl_log_assert(int res0x100000000, ... may result in a test fail. Instead, use _Bool
```
assert(0x100000000); // 8 zeros
void utl_log_assert(_Bool res,
```
Unneeded #include in .h portion. Only #include files in the .h file that the .h file needs. Include in the .c file, the ones it needs.

Thanks for you comments, chux. I had completely missed the %X and the other points in managing the time string. I don't quite get your point 3, could you clarify?
Another question. Related to 14, consider that utl_log_assert is only to be called via log_assert() which means that the !!(x) is applied to its first param. This allows for things like logassert(p) where p is a pointer. Do you think point 14 is still relevant?
8: My intention was to mimic the fopen() behaviour where the w (or a) should be the first character. Did I miss something?
@Remo D. "Arithmetic types and pointer types are collectively called scalar types." C11 §6.2.5 21, so #14 applies to pointers too should code call utl_log_assert(() directly.

Stack Exchange Network

Simple logging library in C

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions