Lightweight logging library in C

I want to get some practice in writing C libraries and so started with something I can use straight away, a logging library. It is very basic and only has two levels: log everything and log only errors. Use logd (log debug) to log any low level thing and if log everything level set, that item will be logged. Use loge to log important stuff that will always be logged.

Is there critical functionality missing here? Any bugs? Any usability issues? Style issues?

log.h:

/* lightweight logging library */
#ifndef LOG_H_
#define LOG_H_

#ifdef __cplusplus
extern "C" {
#endif

typedef enum {
    LOG_ERRORS,
    LOG_EVERYTHING
} log_level;

/* configuration */
void log_init(const char* path, const char* filename, const char* file_extension);
void set_log_level(log_level level);
log_level get_log_level();

/* logging functions */
void logd(const char* format, ...); /* debug log */
void loge(const char* format, ...); /* error log */

/* cleanup / ancillary */
void flush_log();
void close_log();

#ifdef __cplusplus
}
#endif

#endif /* LOG_H_ */

log.c:

```
#include
#include
#include
#include
#include
#include

#include "log.h"

#ifdef __linux__
#define SEPARATOR ('/')
#elif _WIN32 / includes windows 64 bit /
#define SEPARATOR ('\\')
#else
#error Platform not supported
#endif

#define MAX_PATH_LENGTH 512
#define MAX_FILENAME 256
#define MAX_FILE_EXTN 20
#define LOG_FILENAME "program"

static log_level loglevel = LOG_ERRORS;
static char logpath[MAX_PATH_LENGTH] = { 0 };
static char filename[MAX_FILENAME] = LOG_FILENAME;
static char file_extn[MAX_FILE_EXTN] = "log";
static FILE* fp = NULL;

static const char* get_log_filename() {
return filename;
}

static void set_log_filename(const char* name) {
if (name && *name)
strncpy(filename, name, MAX_FILENAME);
}

static void set_path(const char* path) {
int len;
if (path && *path != '\0') {
strncpy(logpath, path, MAX_PATH_LENGTH);
len = strlen(logpath);
if

Bug: Use of sprintf in construct_full_path does not check length

It's possible for strlen(logpath) + strlen(filename) + strlen(append) + strlen(".log") to be longer than MAX_PATH_LENGTH; in this case, your call to sprintf will smash memory beyond the end of fullpath. You should at least use snprintf to avoid out-of-bounds accesses; preferably, you would ensure that the sizes all add up, by either:

• Allocating enough memory statically (currently, logpath could be large enough to take up the whole of fullpath), or

• Allocating memory dynamically to ensure the buffer will be large enough.

Bug: Check if(logpath) in get_path is always true

logpath is a statically-allocated array. It will never be a null pointer, so this condition will always be true (high compiler warning levels should catch this).

Usability issue: SEPARATOR

There are lots of platforms which are not windows or linux; exploding on those platforms because you can't work out what the path separator should be seems excessive.

I would suggest wrapping that whole #ifdef ... #elif ... #else block in something like #ifndef SEPARATOR; that way, you can define the SEPARATOR macro using a compiler flag, and you don't have to care that you can't auto-detect this. If you were going to do that, I'd also suggest renaming SEPARATOR to PATH_SEPARATOR, as it has a kind of global scope (it's also a clearer name).

Portability issue: #include only works on windows

Strictly, I think that backslashes appearing in #include paths is undefined behaviour in some C standards. It certainly doesn't work right on Linux. On most platforms, #include should work if the file exists.

If you were feeling really fussy about portability, you might also want to make it possible to get reasonable behaviour on systems which:

• Don't have sys/timeb.h

• Don't have access to wall-clock time

Convention violation: logging to stdout

Your 'default' log destination is stdout. For a well-behaved program, a more natural logging destination would be stderr.

Nonsense: checking assertion conditions after an assertion

The fragment:

assert(fp != NULL);
if (fp == NULL) {

is bizarre. While it's true that the assertion could be compiled out with NDEBUG (meaning the condition is reachable), one generally works quite hard to ensure that turning off assertions does not change the behaviour of the system. Either fall back to default, or raise an error; don't do different things depending on whether or not the program is being debugged.

Questionable: file extensions are ignored

AFAICT, the extension parameter is just dropped on the floor; the file extension is always .log.

Suggestion: use dynamic allocation

You have lots of ugly fixed-size buffers for path components etc. While these are mostly probably large enough to not run into issues, there are a lot of awkward edge cases if you get near the edges of those buffers, and awkward interactions of sizes. Unless you have some hard performance/memory requirement, it would be much easier just to malloc a chunk of memory the size you need (which you can work out easily from the various strings you're given).

Feature suggestion: accept a user-provided filehandle

A client of this library might want to take advantage of the logging functions, timestamping, and log level management, while managing log location themselves. It would be very straightforward to have the option of specifying a filehandle rather than a collection of path components.

Feature suggestion: consider wrapping your logging functions in macros to get location

For debugging purposes, I generally find that it's very helpful to log the filename and line number of the call to a logging function (depending a little on the context you're using the logger in). If you wrap loge and logd in macros, you can get these out automatically using __FILE__ and __LINE__.

For example, if you had loge as:

void loge(const char *line, int line, const char *format, ...);

then you could write a macro LOGE as:

#define LOGE(...) loge(__FILE__, __LINE__, __VA_ARGS__)

the macros __FILE__ and __LINE__ get expanded to the filename of the containing C file, and the line in that file at which the macro appears; then you can pass them through to the loge function, which could then log something like:

20:41:28.864 logtest.c:32: log session 3 finished

this could be useful in some situations, although you might want an option to turn it off, or to adjust the order (lots of tools can be configured to reference : pairs in various formats back to the source line, as a way of handling e.g. compiler messages).

Naming consideration: loge and logd are very similar.

The visual difference between loge and logd is tiny. Using more different names (e.g., log_error, log_debug), or passing the log_level as a parameter to a single log function:

```
void log(log_level level, const char *f

StackExchange Code Review Q#120715, answer score: 10