Little log engine in C

I programmed a little log engine in C I plan to use in my project and maybe some others in future. I am very novice C programmer and would like to have feedback of some experienced ones on this. It's spawned over few files but I've joined them so it's easier to compile. Thank you.

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

#include "config.h"

/ this is log.h /
#define TPL_IDENT PACKAGE_NAME

enum tp_log_level {
TPL_DEBUG,
TPL_INFO,
TPL_ERR,
TPL_EMERG,
};

enum tp_log_mode {
TPLM_SYSLOG,
TPLM_FILE,
};

void tp_log_init(int mode, int level, int fd);
void tp_log_write(int level, const char *fmt, ...);
void tp_log_close(void);
/ end of log.h /

/ function from pio.c I use /
ssize_t tp_write(int fd, const void *buf, size_t len)
{
ssize_t ret, wlen = 0;
const char *ptr;

ptr = buf;
while (wlen != len) {
ret = write(fd, ptr, len-wlen);

if (ret == -1)
if (errno == EINTR)
continue;
else
return -1;

wlen += ret;
ptr += ret;

}
return wlen;
}

/ start of log.c /

static int log_fd = -1;
static int log_level;
static int log_mode;

static const char *level_txt[] = {
"DEBUG", "INFO", "ERROR", "EMERG"
};

static const int level_syslog[] = {
LOG_DEBUG, LOG_INFO, LOG_ERR, LOG_EMERG
};

void tp_log_init(int mode, int level, int fd)
{
assert(mode == TPLM_SYSLOG || mode == TPLM_FILE);
assert(mode == TPLM_FILE ? fd >= 0 : 1);
assert(level >= TPL_DEBUG && level = TPL_DEBUG && level = MAX_MSG_LEN)
len += MAX_MSG_LEN-1;
else
len += ret;

snprintf(msg+len, MAX_POSTFIX_LEN, "\n");
tp_write(log_fd, msg, len+1);
break;

case TPLM_SYSLOG:
#ifdef HAVE_VSYSLOG
vsyslog(level_syslog[level], fmt, alist);
#else
vsnprintf(msg, MAX_MSG_LEN, fmt, vl);
syslog(level_syslog[level], "%s

As promised, here are some comments on the actual code you posted.

General:

-
don't define multiple variables on the same line.

-
is there a good reason to redefine log levels in tp_log_level rather than
use the levels Syslog defines?

-
you have two arrays, level_txt and level_syslog that are dependent upon
the values of tp_log_level but nothing ties these together. In such a
small program, this is unlikely to be a problem, but this sort of loose
dependency tends to break with time - someone appends (or even inserts!) a
value to one part without adjusting the other. Do you really need to log
the level text, "DEBUG" etc? Syslog doesn't. Why complicate the job? If
you just use syslog constants, both these arrays could be discarded. If you
really must use them, make a structure holding a level and a text and make
an array of these; then at least you only have one dangling dependency...

-
I personally don't have any dogmatic objection to multiple returns. Some
coding standards object to them on the grounds that they make functions
unclear. I would argue the contrary: they can, if used well, make functions
clearer.

In tp_write

-
why using raw write (as opposed to stdio)? The fact that you test for
interrupted writes makes me think that you intend to log to a socket or a
serial port etc... Is that so?

-
you should get compiler warnings about comparisons and conversions between
size_t and ssize_t. Since the function only has two return values, -1 and
len, and the caller knows the value of len anyway, it seems unnecessary
to return anything but an int: 0 for success, and -1 for failure. That way
you can avoid using a ssize_t and the warnings it generates.

-
you need braces in the if/else statements: the compiler has to assume
that the else belongs to the inner if. Using braces even when not
strictly necessary is generally considered good practice. But they can be
ugly...

-
might be better coded as:

ssize_t tp_write(int fd, const void *buf, size_t len)
{
    const char *p = buf;
    const char *end = p + len;

    while (p  0) {
            p += ret;
        }
        else if (errno != EINTR) {
            return -1;
        }
    }
    return 0;
}

In tp_log_init

-
mode would be more logically of type tp_log_mode

-
I don't use asserts much. You are relying on the asserts to find problems
that the subsequent code does not handle (eg. the switch has no
default).
But another approach is to allow the code to fail if the parameters are
wrong by falling back on syslog. This works even when NDEBUG is defined
(asserts disabled):

void tp_log_init(enum tp_log_mode mode, enum tp_log_level level, int fd)
{
    log_level = level;
    if (mode == TPLM_FILE) {
        if (fd >= 0) {
            log_fd = fd;
            log_mode = TPLM_FILE;
            return;
        }
        /* print an error or abort() if you like */
    }
    openlog(TPL_IDENT, LOG_PID|LOG_CONS, LOG_DAEMON);
    log_mode = TPLM_SYSLOG;
}

In tp_vlog_write

-
MAX_TIME_LEN and MAX_MSG_LEN seem generous for a log message. Remember that
msg is on the stack and if you are in a restricted environment, 1500 bytes
might be too much. ctime uses 26 bytes for the time/date, so I doubt your time
format needs 512.

-
level seems like it should be a enum tp_log_level

-
the switch could be done with if/else

-
I would extract the creation of a time string into a separate function.
Also, why abort if localtime fails? This makes your code more fragile
than it needs to be - just don't call strftime() if tm == NULl.

-
your assert(log_fd) fails if log_fd == 0. This should apply to the
TPLM_FILE mode only.

-
as I said before, printing the level text seems unnecessary. If you need
it, use strcpy instead of snprintf.

-
printing "\n" could be done with strcpy(msg+len, "\n")

-
Personally, I have no objection to the use of varargs in this code. @Lundin
is right that it is not type-safe, but sometimes it has its uses. This
seems a legitimate use to me.

In tp_log_close

• your assert fails if log_fd == 0.

• if/else would be clearer.

StackExchange Code Review Q#23110, answer score: 2