Lightweight logging library in C

Question

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 <string.h>
#include <stdio.h>
#include <stdarg.h> 
#include <sys/timeb.h> 
#include <time.h>
#include <assert.h>

#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 (len > 0 && logpath[len - 1] != SEPARATOR)
            logpath[len] = SEPARATOR;
    }
}

static const char* get_path() {
    if (!logpath) {
        sprintf(logpath, ".%c", SEPARATOR);
    }
    return logpath;
}

static char* get_append_name(char* buf) {
    time_t now;
    time(&now);
    strftime(buf, 20, "%y%m%d", localtime(&now));
    return buf;
}

static const char* get_log_filename_extension() {
    return file_extn ? file_extn : "";
}

static void set_log_filename_extension(const char* name) {
    if (name && *name != '\0')
        strncpy(file_extn, name, MAX_FILE_EXTN);
}

static char* construct_full_path(char* path) {
    char append[20] = { 0 };
    sprintf(path, "%s%s%s.%s", get_path(), get_log_filename(), get_append_name(append), get_log_filename_extension());
    return path;
}

void log_init(const char* path, const char* filename, const char* file_extension) {
    char fullpath[MAX_PATH_LENGTH];
    if (path && *path != '\0' && filename && *filename != '\0') {
        set_path(path);
        set_log_filename(filename);
        set_log_filename_extension(file_extension);
        fp = fopen(construct_full_path(fullpath), "a");
        assert(fp != NULL);
        /* just in case fopen fails, revert to stdout */
        if (fp == NULL) {
            fp = stdout;
            fprintf(fp, "Failed to change logging target\n");
        }
    }
    else {
        if (fp != NULL && fp != stdout)
            fclose(fp);

        fp = stdout;
    }
}

static char* get_timestamp(char* buf) {
    int bytes;
    struct timeb start;
    ftime(&start);
    bytes = strftime(buf, 20, "%H:%M:%S", localtime(&start.time));
    sprintf(&buf[bytes], ".%03u", start.millitm);
    return buf;
}

void set_log_level(log_level level) {
    loglevel = level;
}

log_level get_log_level() {
    return loglevel;
}

void logd(const char* format, ...) {
    char tmp[50] = { 0 };
    if(loglevel > 0) {
        va_list args;
        va_start (args, format);
        fprintf(fp, "%s ", get_timestamp(tmp));
        vfprintf (fp, format, args);
        va_end (args);
   }
}

void loge(const char* format, ...) {
    char tmp[50] = { 0 };
    va_list args;
    va_start (args, format);
    fprintf(fp, "%s ", get_timestamp(tmp));
    vfprintf (fp, format, args);
    va_end (args);
    fflush(fp);
}

void flush_log() {
    fflush(fp);
}

void close_log() {
    if(fp != stdout)
        fclose(fp);
    fp = NULL;
}

An exercising program:

#include "log.h"

int main() {
    set_log_level(LOG_EVERYTHING);

#ifdef __linux__ 
    log_init("/home/acomber/Documents/code/log_test", "myprogram", "log");
#else
    log_init("c:\\dell", "myprogram", "log");
#endif    

    logd("my number: %d\n", 1);
    logd("my string: %s\n", "string one");

    loge("log session 1 finished\n");

    flush_log();
    close_log();


    set_log_level(LOG_EVERYTHING);
    log_init(0, 0, 0);

    logd("my number: %d\n", 2);
    logd("my string: %s\n", "string two");

    loge("log session 2 finished\n");

    close_log();

    set_log_level(LOG_ERRORS);
    log_init(0, 0, 0);

    logd("my number: %d\n", 3);
    logd("my string: %s\n", "string three");

    loge("log session 3 finished\n");

    close_log();

}

Answer 1

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 <sys\timeb.h> 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 <sys/timeb.h> 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 <filename>:<line> 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 *format, ...)

would in my opinion be easier to read (you might choose to make the log level names a little shorter, e.g., LOG_ERR, LOG_ALL, in that case).

Naming consideration: prefer systematic naming

This is probably quite subjective, but: in the case of a module such as this one, my inclination would be to prefix all of the names with log_, like so:

log_init
log_set_level
log_get_level
log_flush
log_close

I find this makes it easier to cope with C's lack of 'real' namespacing.

Dependency-management suggestion: include your own header first

Opinions vary on this, but if you start your implementation file (e.g., log.c) by including your header file (i.e., line 1: #include <log.h>) then you can tell immediately if your header file has an accidental dependency on some type or macro introduced by another header file you're including. I prefer to make my header files self-sufficient; including them first anywhere guarantees this.

Subjective layout suggestion: implement top-level interface first

This is overwhelmingly subjective, but I would at least consider the overall layout of your implementation file. Personally, for a module like this one, I would prefer to forward-declare the static functions, then define them at the end. This way, the file reads top-down. In other contexts, that might be less clear, and it is more typing, so it's a judgement call.

Answer 2

Here is my suggestion to make it more object-oriented:

log_init() should return a handle, which is passed in subsequent calls to logd(), loge(), etc. In other words, there should be no global, static data in the .c file:

1. Bundle all types of static variables into a struct (i.e. "class" in OOP parlance)

2. log_init() dynamically creates and initializes a struct and returns a pointer to it (i.e. the handle mentioned above, "constructor" in OOP parlance)

3. All other logging functions must take a pointer to that struct type as first argument (i.e. "instance" or "object" in OOP parlance, e.g. "this"-pointer in C++)

This design decouples code and data and makes it possible to create multiple logging instances which are independent of each other.