A simple logging library in C

Question

I've created a simple logging library in C which I can use in my other projects. I wanted to get started in software development using the C language, and this is my first attempt at writing something which comes close to an actual software. Looking for a code review/feedback for this library, especially about reusability, optimization, code flow, and testing. Here are the code files:

logger.h

#ifndef LOGGER_H
#define LOGGER_H

#include <stdlib.h>
#include <stdbool.h>
#define DEFAULT_LOG_PATH "/var/log/logger.log"
#define RESET 0

typedef enum {
    OFF,
    DEBUG,
    INFO,
    WARNING,
    ERROR
} LOGGING_LEVELS;

/**
 * @brief Function to initialize the logger.
 *
 * @params[in] level Log level.
 * @params[in] filename Path to the log file.
 * @params[in] to_file Boolean variable to allow writing to file or to stderr stream.
 *
 * @retval None.
 */
void init_logger(LOGGING_LEVELS, const char *, bool);

/**
 * @brief Function to write the message to the log file.
 *
 * @params[in] level Log level.
 * @params[in] frmt Format message.
 * @params[in] ... Variable arguments.
 *
 * @retval None.
 */
void log_func(LOGGING_LEVELS level, const char *filename, const size_t line, const char *frmt, ...);

#define log_debug(...) log_func(DEBUG, __FILE__, __LINE__, __VA_ARGS__)
#define log_info(...)  log_func(INFO, __FILE__, __LINE__, __VA_ARGS__)
#define log_warn(...)  log_func(WARNING, __FILE__, __LINE__, __VA_ARGS__)
#define log_error(...) log_func(ERROR, __FILE__, __LINE__, __VA_ARGS__)

#endif

logger.c

#include "logger.h"
#include <stdio.h>
#include <stdarg.h>
#include <string.h>
#include <time.h>

//Global variables to keep track of log files.
static int LOG_LEVEL = OFF;
static const char* log_filename = DEFAULT_LOG_PATH;
static bool write_to_file = false;

static const char* log_level_strings[] = {
    "OFF",
    "[DEBUG]",
    "[INFO]",
    "[WARNING]",
    "[ERROR]",
};

const char* colors[] = {
    "\x1B[0m",
    "\x1B[34m",
    "\x1B[32m",
    "\x1B[33m",
    "\x1B[31m",
};

/**
 * @brief Function to get the current date and time and write to log file.
 *
 * @params[in] fptr File pointer to the log file.
 *
 * @retval None.
 */
static void get_datetime(FILE *fptr) {
    time_t t = time(NULL);
    struct tm *tm  = localtime(&t);
    const char *d_time = asctime(tm);
    const char *datetime = strchr(d_time, '\n');
    int length = datetime - d_time;

    char *modified_date = malloc(length + 1);   
    strncpy(modified_date, d_time, length);
    modified_date[length] = '\0';
    fprintf(fptr, "%s ", modified_date); 

    free(modified_date);
}

/**
 * @brief Function to write the log level to the log file.
 *
 * @params[in] level Log level.
 * @params[in] fptr File pointer to the log file.
 *
 * @retval None
 */
static void log_msg(LOGGING_LEVELS level, FILE *fptr) {
    get_datetime(fptr);
    if(fptr == stderr) {
        fprintf(fptr, "%s%-10s%s", colors[level], log_level_strings[level], colors[RESET]);
    }
    else {
        fprintf(fptr, "%-10s", log_level_strings[level]);
    }
}

void log_func(LOGGING_LEVELS level, const char *file, const size_t line, const char *frmt, ...) {
    if(LOG_LEVEL > level) {
        return;
    }

    FILE *fptr = stderr;
    if(write_to_file) {
        fptr = fopen(log_filename, "a");
        if(NULL == fptr) {
            fprintf(stderr, "%sError:%s File not existing or Permission denied\nExiting the program with status 1.\n", colors[ERROR], colors[RESET]);
            exit(1);
        }
    }

    log_msg(level, fptr);
    fprintf(fptr, "%s: %lu: ", file, line);
    
    char *format = strdup(frmt);
    strcat(format, "\n");
    va_list argp;
    va_start(argp, frmt);
    vfprintf(fptr, format, argp);
    va_end(argp);

    if(fptr != stderr) {
        fclose(fptr);
    }
    free(format);
}

void init_logger(LOGGING_LEVELS level, const char* filename, bool to_file) {
    if(LOG_LEVEL > OFF) {
        fprintf(stderr, "%sError:%s Logger already initialised in the project\nExiting the program with status 1.\n", colors[ERROR], colors[RESET]);
        exit(1);
    }

    LOG_LEVEL = level;
    if(NULL != filename) {
        log_filename = filename;
    }
    write_to_file = to_file;
}

Test files:

test.c

/**
 * Test to check that init_logger and log_debug work properly.
 */

#include "../src/logger.h"
#include <stdio.h>

void test_init(void) {
    init_logger(INFO, "log.log", true); //Will write to log.log file
//  init_logger(DEBUG, NULL, true); //Will write to the default log path. Needs to be run with sudo.
//  init_logger(DEBUG, NULL, false); //Will output to stderr console.
}

int main() {
    test_init();
    log_debug("Print: %d", DEBUG); //Will not print anything.
    log_info("Print: %d", INFO);
    log_warn("Print: %d", WARNING);
    log_error("Print: %d", ERROR);
    return 0;
}

Answer 1

I'll go step by step:

---

[EDIT: removed part about global vars as they are only in the C implementation file].

---

"\x1B[0m",

Now which color is this? I would presume black or white (it's not, it is RESET), but you're leaving the next developer to guess: use a #define TERM_RESET "\x1B[0m". Also for the others of course.

I did look for an easy to include header for these escape codes, but I could not find it directly.

---

static void get_datetime(FILE *fptr) {

This is not a good name for a function if it writes to the file. Careless inspection would suggest that you will get the datetime from the file. As such, it fails the principle of least surprise. I would suggest something like write_current_localtime(FILE *fptr).

I'd also remove log from and write to log file. in the comment. At that point in time, it is unclear what kind of file we're talking about. It is always best to make functions as generic as possible.

---

int length = datetime - d_time;

char *modified_date = malloc(length + 1);   
strncpy(modified_date, d_time, length);
modified_date[length] = '\0';

This is all unnecessary. You already know that the string ends with \n. Furthermore, you don't need a second string at all. Strings in C are null-terminated so you just have to insert the \0 once in the datetime string:

datetime[strlen(datetime) - 1] = `\0`;

Of course, you'd probably want to put that in a function called remove_last_character() and then, when calling, indicate that you are just removing the newline using a comment.

---

static void log_msg(LOGGING_LEVELS level, FILE *fptr) {

This name is even worse; it would be expected to log a message - which it doesn't. I'd use something like static void write_loglevel(FILE *fptr, LOGGING_LEVELS level).

Personally I order the parameters from least variable to more variable. As you can see, in most file handling functions the file pointer is always the first one, so I'd keep to that if just for symmetry.

---

/**
 * @brief Function to write the message to the log file.
 *
 * @params[in] level Log level.
 * @params[in] frmt Format message.
 * @params[in] ... Variable arguments.
 *
 * @retval None.
 */
 void log_func(LOGGING_LEVELS level, const char *filename, const size_t line, const char *frmt, ...);

You should make very clear in the documentation that *filename is just a string that is logged, rather than a log file (possible confusion again). Note that you use just *file in the implementation of the function. I strongly suspect that the file or filename is a later addition as you forgot to document the parameter.

I'd also make sure to indicate that the end of line character is supplied by the function.

---

if(LOG_LEVEL > level) {

I'm not that much of a C programmer anymore, but I'd change the arguments around: if(level < LOG_LEVEL) {. If the argument is nullable I can understand turning it around, and maybe this is now best practice, but generally I use the variable value on the left, and the constant value on the right.

---

fprintf(fptr, "%s: %lu: ", file, line);

It might be that the user forgets to indicate the file and uses null. In that case fprintf is not defined, and in C you may want to avoid that kind of undefined behavior.

---

/**
 * Test to check that init_logger and log_debug work properly.
 */

This header comes a bit out of the blue for me, and you'd want to test all levels I suppose. I'm also wondering why it starts with /**; generally file headers are not part of the official documentation - but that doesn't mean that it is necessarily wrong - I was just surprised by it.

---

Generally the code looks nice and is well formatted, no issues there. That's an important start, it means you're methodical and the code shows you understand how to group things correctly.

Answer 2

This isn't so much a critique of the code, as an outline of how I tend to approach creating something intended as a reusable API.

I've not included all of the OP code functionality, because this more about encapsulation and conventions rather than specific functionality. I'm also writing this after the first answer so I'm trying to avoid duplication.

Consider this (explanation below)...

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

typedef enum {
    OFF,
    DEBUG,
    INFO,
    WARNING,
    ERROR
} logger_level_t;

typedef struct {
    logger_level_t level;
    FILE *fp;
} logger_t;

int logger_init(logger_t * const logger, logger_level_t const level, char const * const filename, bool const write_to_file) {

    logger->level = level;

    if (write_to_file) {
        logger->fp = fopen(filename, "a");
        if (logger->fp == NULL) {
            return -1;
        }
    } else {
        logger->fp = stderr;
    }
    return 0;
}

void logger_log(logger_t const * const logger, logger_level_t const level, char const * const file, size_t const line, char const * const fmt, ...) {
    if (level < logger->level) {
        return;
    }
    
    fprintf(logger->fp, "%s: %lu: ", file, line);
    va_list argp;
    va_start(argp, fmt);
    vfprintf(logger->fp, fmt, argp);
    va_end(argp);    
    fprintf(logger->fp, "\n");
}

int main() {
    logger_t logger;
    logger_init(&logger, DEBUG, NULL, false);
    logger_log(&logger, DEBUG, __FILE__, __LINE__, "Hello %s", "World");
    return 0;
}

Prefix term for the package

First up, choose a prefix for symbols and stick to it. Here I've chosen the work 'logger' and then applied appropriate capitalisation for types and variables.

Naming conventions

Note that in C, the convention is that types are lowercase, snake case, and suffixed with _t, variable names are lower snake case, and constants and macros are all uppercase. Convention varies of course, your target audience may have its own style conventions.

When typedef'ing an enum, the type should be named in the singular, or its use as a parameter can be misleading (suggesting it might be an array of values).

Globals

Where at all possible, avoid globals. Using globals makes assumptions about the environment and memory allocation of the code that is consuming your API, so its good to delegate that to the consumer.

As you can see in the example, the logger structure can be allocated on the stack, as a global, or dynamically allocated, as determined by the consumer. It can be encapsulated in another structure, array or any number of conventions that are useful to the consumer.

Parameter constness

I have a habit of writing const as appropriate for all parameters. It makes it explicit what the expectations are externally, and also prevents misuse internally. I also avoid the easily learnt use of const before the type, which I find creates ambiguity once we have one or more indirections. e.g. It's much easier if you read the type 'backwards', so

char const * const

is read as 'a const pointer to a const char'.

Prerequistes

Leaving the open/close of the file to any eventual logging event leaves the possibility that the specfied file is not writable, meaning that you lose the log information. By opening the file in the 'init' function we ensure that the file is accessible. This doesn't consider the possibility of later 'write' failures such as lack of storage.

By opening the file in the init, we raise the problem of cleaning up (we should consider that application exit might not be the actual final action, the consumer might want to init the logger multiple times, and this is in fact an inevitability when writing tests. As a result, a logger_deinit function should probably be provided.

We also no longer need to maintain access to the log filename or the 'write to file' variable.

Dynamic memory allocation

Try very hard to avoid dynamic memory allocation. It makes assumptions about the environment the API will be used in. Even when dynamic is necessary, it can be useful to pass function pointers to the 'init' method that mimics the malloc/free API, so that the consumer can determine how this should be done. You can always support NULL as a function pointer to indicate to use malloc/free so as not to overly complicate the API.

In OP's code, the dynamic allocation was completely redundant anyway.

This actually raises a broader issue about assumptions when it comes to the standard C library. All of the '*printf' API's have some issues and implications so you are potentially tying yourself to those, but I'm not suggesting that you SHOULD allow delegation of those, merely that it demonstates that assumptions have baggage.

Testing

Merely exercising the API doesn't validate that it works. Generally we shouldn't be testing standard library functions (fopen/close,*printf) because we can 'trust' that those work, but we don't know 'our' code uses them in the right way. This suggests that we should provide 'seams' that allow our tests to provide mock/dummy/spy implementations of the API's it uses, so that we can verify the calling parameters, rather than the outcome. This again raises the notion that we should provide a way to inject the function pointers for API's we intend to consume. This may not be what's needed for a relatively simple API like this one, but as complexity increases, it can become attractive or even mandatory (imagine these are API's that rely on external resources like network requests or database tables with expected data).

Using the style outlined above, its possible to write API's that always allow for injection. The logger_t structure can contain function pointers to all of the expected API's, and the default logger_init can initialise those with the standard API's we 'assume' people will use. But it allows us to then patch that for testing purposes.

Personally I find the C-Green unit testing library to be very useful, and allows you to create and validate mocks injected using this sort of approach.

Here is the same code again, but with the standard file routines isolated and passed into the logger API:

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

typedef struct {
    FILE * (*fopen)(char const * const filename, char const * const mode);
    int (*fprintf)(FILE *fp, char const * const fmt, ...);
    int (*vfprintf)(FILE *fp, char const * const fmt, va_list argp);
} std_file_t;

void std_file_init(std_file_t * const std_file) {
    std_file->fopen = fopen;
    std_file->fprintf = fprintf;
    std_file->vfprintf = vfprintf;
}

typedef enum {
    OFF,
    DEBUG,
    INFO,
    WARNING,
    ERROR
} logger_level_t;

typedef struct {
    logger_level_t level;
    FILE *fp;
    std_file_t *std_file;
} logger_t;

int logger_init(
    logger_t * const logger, 
    logger_level_t const level, 
    char const * const filename, 
    bool const write_to_file,
    std_file_t *std_file
) {

    logger->level = level;

    if (write_to_file) {
        logger->fp = logger->std_file->fopen(filename, "a");
        if (logger->fp == NULL) {
            return -1;
        }
    } else {
        logger->fp = stderr;
    }
    
    logger->std_file = std_file;
    
    return 0;
}

void logger_log(logger_t const * const logger, logger_level_t const level, char const * const file, size_t const line, char const * const fmt, ...) {
    if (level < logger->level) {
        return;
    }
    
    logger->std_file->fprintf(logger->fp, "%s: %lu: ", file, line);
    va_list argp;
    va_start(argp, fmt);
    logger->std_file->vfprintf(logger->fp, fmt, argp);
    va_end(argp);    
    logger->std_file->fprintf(logger->fp, "\n");
}

int main() {
    std_file_t std_file;
    std_file_init(&std_file);
    logger_t logger;
    logger_init(&logger, DEBUG, NULL, false, &std_file);
    logger_log(&logger, DEBUG, __FILE__, __LINE__, "Hello %s", "World");
    return 0;
}