Follow up: A chat server using the select() API

Question

This is a follow up to my last question: Multiperson chat server using the select() API

Changes:

After much fine critique, I made the following changes to the code:

• Removed redundant casts and initializations.
• Moved the contents of selectserver.h to selectserver.c.
• Rewrote the read_line() function (Added checks for buffer overflow / DOS attack).
• Enabled line-buffering for the file stream instead of flushing logs each time.
• Replaced the FSYNC macro with a function, and moved it to log.c.
• Removed the exit_cleanup() function registered with atexit().
• Simplified main(). Moved all the socket opening and file opening logic in separate functions.
• Added error-handling for time.h functions.
• Added a check for a file descriptor > 1024, silently closing it to guard against undefined behaviour.

Review goals:

Some more questions:

1. Could any of these syscalls block?

2. Is using blocking sockets defeating (at least partially) the point of select()?

3. Is the overhead of continuously malloc()ing and free()ing comparable in contrast to the overhead of syscalls?

4. Do you see a potential buffer overflow / memory leak? (Other than the FILE * valgrind reports about, though that should be eliminated as well)

5. Does any part of my code exhibit undefined behaviour?

6. Is there a better strategy with handling file descriptors above 1024, other than gracefully exiting or silently closing any new connections? (Eliminating the possibility of using poll() and the like)

7. Does any part of my code require a comment?

8. How do I avoid mixed messages? For instance:

Jack (typing): hel...

John (typing): nig...

Jack's screen: hellnight

9. Would it suffice to call fsync() before closing the file descriptor once, instead of calling it after every write()?
10. How can I further improve my code?

Code:

log.h:

#ifndef LOG_H
#define LOG_H

#include <stdio.h>
#include <unistd.h>
#include <time.h>

#define LOG_TIME            0x01        /* 0b00000001 */
#define LOG_DATE            0x02        /* 0b00000010 */
#define LOG_USER            0x04        /* 0b00000100 */
#define LOG_COUNT           0x08        /* 0b00001000 */
#define LOG_ALL             0xFF        /* 0b11111111 */
#define LOG_FULLTIME        0x03        /* 0b00000011 */

void flush_cache (int fd);
void log_msg (FILE * fp, const char *msg, unsigned int options);

#endif

log.c:

#include "log.h"

#define TS_BUF_LENGTH 30

void flush_cache (int fd)
{
    if (fsync (fd) == -1) {
        perror ("fsync()");
    }
}

void log_msg (FILE * fp, const char *msg, unsigned int flags)
{
    static long long log_count = 0;
    char time_stamp[TS_BUF_LENGTH];
    char date_stamp[TS_BUF_LENGTH];
    time_t time_val = flags & LOG_FULLTIME ? time (0) : 0;
    struct tm *tm_info = flags & LOG_FULLTIME ? localtime (&time_val) : 0;

    if (flags & LOG_COUNT) {
        fprintf (fp, "%lld\n, ", ++log_count);
    }
    if (flags & LOG_DATE
        && strftime (date_stamp, TS_BUF_LENGTH, "%F (%a)", tm_info)) {
        fprintf (fp, "%s, ", date_stamp);
    }
    if (flags & LOG_TIME
        && strftime (time_stamp, TS_BUF_LENGTH, "%H:%M:%S", tm_info)) {
        fprintf (fp, "%s, ", time_stamp);
    }

    fprintf (fp, "%s\n", msg);
}

selectserver.c

/*
*  selectserver.c -- a multiperson chat server
*/

#define _POSIX_C_SOURCE 200809L

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
#include <errno.h>
#include <signal.h>

#include <unistd.h>
#include <sys/time.h>
#include <netdb.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <sys/ioctl.h>
#include <arpa/inet.h>

#include "log.h"

#define PROGRAM_NAME "selectserver"
#define PORT "9808"             /* Port we are listening on */
#define MAX_LOG_TEXT 2048       /* Max text lenght for logging */

#ifdef BUFSIZ                   /* Max text lenght */
#define BUFSIZE BUFSIZ
#else
#define BUFSIZE 4096
#endif

#define LOG_FILE "server.log"

#define ARRAY_CARDINALITY(x) (sizeof(x) / sizeof (x)[0])
#define NI_MAXHOST 1024
#define NI_MAXSERV 35

FILE *log_fp = 0;
int log_fd = 0;

static void sigint_handler (int sig)
{
    close (log_fd);
    signal (sig, SIG_DFL);
    raise (sig);
}

static int init_addr (struct addrinfo **servinfo)
{
    struct addrinfo hints;

    memset (&hints, 0x00, sizeof hints);
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_flags = AI_PASSIVE;

    if (getaddrinfo (0, PORT, &hints, servinfo) != 0) {
        perror ("getaddrinfo()");
        return -1;
    }
    return 0;
}

static void configure_tcp (int master_fd)
{
    int yes = 1;
    socklen_t size_yes = sizeof yes;

    if (setsockopt
        (master_fd, SOL_SOCKET, SO_REUSEADDR, (void *) &yes,
         size_yes) == -1) {
        perror ("setsockopt()");
    }
    if (setsockopt
        (master_fd, SOL_SOCKET, SO_KEEPALIVE, (void *) &yes,
         size_yes) == -1) {
        perror ("setsockopt()");
    }
    int keep_cnt = 9;
    socklen_t size_cnt = sizeof keep_cnt;

    if (setsockopt
        (master_fd, IPPROTO_TCP, TCP_KEEPCNT, (void *) &keep_cnt,
         size_cnt) == -1) {
        perror ("setsockopt()");
    }
    int keep_idle = 25;
    socklen_t size_idle = sizeof keep_idle;

    if (setsockopt
        (master_fd, IPPROTO_TCP, TCP_KEEPIDLE, (void *) &keep_idle,
         size_idle) == -1) {
        perror ("setsockopt()");
    }
    int keep_intvl = 25;
    socklen_t size_intvl = sizeof keep_intvl;

    if (setsockopt
        (master_fd, IPPROTO_TCP, TCP_KEEPINTVL, (void *) &keep_intvl,
         size_intvl) == -1) {
        perror ("setsockopt()");
    }
}

/* Returns: -1 on failure, a listening socket descriptor otherwise.
*/
static int open_tcp_socket (struct addrinfo **servinfo)
{
    int master_fd = 0;
    struct addrinfo *p = 0;

    for (p = *servinfo; p; p = p->ai_next) {
        master_fd = socket (p->ai_family, p->ai_socktype, p->ai_protocol);
        if (master_fd == -1) {
            perror ("socket()");
            continue;
        }
        configure_tcp (master_fd);

        if (bind (master_fd, p->ai_addr, p->ai_addrlen) == -1) {
            close (master_fd);
            perror ("bind()");
            continue;
        }
        break;
    }

    if (!p) {
        fprintf (stderr, "%s: Failed to setup a socket.\n", PROGRAM_NAME);
        return -1;
    }

    if (listen (master_fd, SOMAXCONN) == -1) {
        perror ("listen()");
        close (master_fd);
        return -1;
    }
    return master_fd;
}

static void get_slave_addr (int slave_fd, char *ip_str, int *port)
{
    struct sockaddr_storage addr;
    socklen_t addr_len = sizeof addr;

    if ((getpeername (slave_fd, (struct sockaddr *) &addr, &addr_len)) ==
        -1) {
        perror ("getpeername");
    }
    /* Deal with both IPv4 and IPv6 */
    if (addr.ss_family == AF_INET) {
        struct sockaddr_in *s = (struct sockaddr_in *) &addr;

        *port = ntohs (s->sin_port);
        if (!inet_ntop (AF_INET, &s->sin_addr, ip_str, INET_ADDRSTRLEN)) {
            perror ("inet_ntop()");
        }
    } else {
        struct sockaddr_in6 *s = (struct sockaddr_in6 *) &addr;

        *port = ntohs (s->sin6_port);
        if (!inet_ntop (AF_INET6, &s->sin6_addr, ip_str, INET6_ADDRSTRLEN)) {
            perror ("inet_ntop()");
        }
    }
}

static void write_slave_info (int slave_fd)
{
    struct sockaddr_in slave_addr;
    socklen_t addr_len = sizeof slave_addr;
    char slave_ip[INET6_ADDRSTRLEN] = { 0 };
    char local_ip[INET6_ADDRSTRLEN] = { 0 };
    int port = 0;

    get_slave_addr (slave_fd, slave_ip, &port);

    if (getsockname (slave_fd, (struct sockaddr *) &slave_addr, &addr_len)
        == -1) {
        perror ("getsockname()");
    }

    if (!inet_ntop
        (AF_INET, (const void *) &slave_addr, local_ip, sizeof local_ip)) {
        perror ("inet_ntop()");
    }

    char host[NI_MAXHOST] = { 0 };
    char service[NI_MAXSERV] = { 0 };
    int ret_val = 0;

    if ((ret_val =
         getnameinfo ((struct sockaddr *) &slave_addr, addr_len, host,
                      sizeof host, service, sizeof service, 0)) != 0) {
        fprintf (stderr, "%s: getnameinfo: %s\n", PROGRAM_NAME,
                 gai_strerror (ret_val));
    }
    char log_txt[MAX_LOG_TEXT] = { 0 };
    snprintf (log_txt, sizeof log_txt,
              "INFO: New connection from FOREIGN IP ADDRESS:%s, HOST:%s, SERVICE:%s,\n\t\t\t\t  LOCAL IP ADDRESS:%s, PORT:%d on socket %d.",
              slave_ip, host, service, local_ip, port, slave_fd);
    log_msg (stderr, log_txt, LOG_FULLTIME);
    log_msg (log_fp, log_txt, LOG_FULLTIME);

    flush_cache (log_fd);
}

/*  Returns:  The slave file descriptor on success, -1 otherwise.
*/
static int accept_new_connection (int master_fd)
{
    int slave_fd = 0;
    struct sockaddr slave_addr = { 0 };
    socklen_t addr_len = sizeof slave_addr;

    do {
        slave_fd = accept (master_fd, &slave_addr, &addr_len);
    } while ((slave_fd == -1) && (errno == EINTR));

    if (slave_fd == -1) {
        perror ("accept()");
        return -1;
    }
    write_slave_info (slave_fd);
    return slave_fd;
}

static void write_farewell (int slave_fd)
{
    char log_txt[MAX_LOG_TEXT] = { 0 };
    snprintf (log_txt, sizeof log_txt, "INFO: Socket %d hung up.\n",
              slave_fd);
    log_msg (stderr, log_txt, LOG_FULLTIME);
    log_msg (log_fp, log_txt, LOG_FULLTIME);
    flush_cache (log_fd);
}

/* Synopsis: Get the number of bytes that are immediately available for
*                        reading.
*  Returns:  -1 on failure, number of bytes available elsewise.
*/
static int get_bytes (int slave_fd)
{
    int flag = 0;

    if (ioctl (slave_fd, FIONREAD, &flag) == -1) {
        perror ("ioctl()");
        return -1;
    }
    return flag;
}

/* Synopsis: Calls recv() in a loop to read as much as available.
*
*  Returns:  0 on allocation failure, storing 0 in status, a pointer
                         to the line elsewise.
*                        Stores -1 in status in case of a closed connection
*                        or a recv() error.
*
*  Remarks:  The caller is responsible for freeing the returned
*                        memory (in case of success), else we risk exhaustion.
*/
static char *get_response (size_t *nbytes, int slave_fd, int *status)
{
    static const size_t page_size = BUFSIZE;
    char *buf = 0;
    int flag = 0;
    ssize_t ret_val = 0;
    size_t total = 0;

    do {
        if (total > (BUFSIZE * 10)) {   /* Likely a DOS attack */
            free (buf);
            *status = -1;
            return 0;
        }
        char *new = realloc (buf, total + page_size);

        if (!new) {
            perror ("realloc()");
            status = 0;
            return 0;
        }
        buf = new;
        new = 0;

        ret_val = recv (slave_fd, buf + total, page_size - 1, 0);
        if (ret_val > 0) {
            total += (size_t) ret_val;
            buf[ret_val] = '\0';

            flag = get_bytes (slave_fd);
            if (flag == -1) {
                free (buf);
                *status = -1;
                return 0;
            }
        } else {
            flag = 0;
        }
    } while (flag > 0);

    if (ret_val <= 0) {
        if (ret_val == 0) {
            write_farewell (slave_fd);
        }
        free (buf);
        *status = -1;
        return 0;
    }
    *nbytes = total;
    return buf;
}

/* Synopsis: Calls send() in a loop to ensure that all data is sent.
*                        Stores the number of bytes sent in len.
*  Returns:  0 on success, -1 otherwise.
*/
static int send_all (int slave_fd, const char *line, size_t *len)
{
    size_t total = 0;
    size_t bytes_left = *len;
    ssize_t status = 0;

    while (total < *len) {
        status = send (slave_fd, line + total, bytes_left, MSG_NOSIGNAL);
        if ((status == -1) && (errno == EINTR)) {       /* A send() error */
            continue;
        }
        if (status == -1) {
            break;
        }
        total += (size_t) status;
        bytes_left -= (size_t) status;
    }
    *len = total;
    return status == -1 ? -1 : 0;
}

static void send_msg (size_t nbytes, const char *line, int sender_fd,
                      int master_fd, fd_set master, int fd_max)
{
    for (int i = 0; i <= fd_max; i++) {
        /* Send it to everyone */
        if (FD_ISSET (i, &master)) {
            /* Excluding the master and sender */
            if (i != master_fd && i != sender_fd) {
                size_t len = nbytes;
                int status = send_all (i, line, &len);

                if (status == -1) {
                    perror ("send()");
                } else if (len != nbytes) {
                    fprintf (stderr,
                             "%s: We only sent %ld bytes because of a send() error.\n",
                             PROGRAM_NAME, len);
                }
            }
        }
    }
}

/* Synopsis: Calls select and handles new connections.
*  Returns:  -1 on failure to init select or ENOMEM, 0 otherwise.
*/
static int handle_connections (int master_fd)
{
    fd_set master;              /* Master file descriptor list */
    fd_set read_fds;            /* Temp file descriptor list for select() */
    int fd_max = 0;             /* Max descriptor seen so far */

    FD_ZERO (&master);
    FD_ZERO (&read_fds);

    FD_SET (master_fd, &master);

    fd_max = master_fd;

    for (;;) {
        /* Because select is destructive */
        read_fds = master;
        if (select (fd_max + 1, &read_fds, 0, 0, 0) == -1) {
            if (errno == EINTR) {
                continue;
            }
            perror ("select()");
            return -1;
        }

        /* Iterate through the existing connections looking for data to read */
        for (int i = 0; i <= fd_max; i++) {
            /* We have a connection */
            if (FD_ISSET (i, &read_fds)) {
                /* It's the master */
                if (i == master_fd) {
                    int slave_fd = 0;

                    if ((slave_fd =
                         accept_new_connection (master_fd)) != -1) {
                        if (slave_fd < 1023) {
                            FD_SET (slave_fd, &master);
                        } else {
                            close (slave_fd);
                            continue;
                        }
                    }
                    if (slave_fd > fd_max) {
                        fd_max = slave_fd;
                    }
                }               /* We have data to read */
                else {
                    size_t nbytes = 0;
                    int status = 0;
                    char *line = get_response (&nbytes, i, &status);

                    /* A read error, or the slave closed connection */
                    if (!line && !status) {
                        return -1;      /* ENOMEM */
                    } else if (status == -1) {
                        FD_CLR (i, &master);
                        close (i);
                    } else {
                        send_msg (nbytes, line, i, master_fd, master,
                                  fd_max);
                        free (line);
                    }
                }
            }
        }
    }
    return 0;
}

/* Returns: the master_fd on success, -1 otherwise.
*/
static int setup_server (void)
{
    struct addrinfo *servinfo;
    int master_fd = 0;

    if (init_addr (&servinfo) == -1) {
        return -1;
    }
    if ((master_fd = open_tcp_socket (&servinfo)) == -1) {
        freeaddrinfo (servinfo);
        return -1;
    }
    freeaddrinfo (servinfo);
    return master_fd;
}

/* Returns: -1 on failure, 0 elsewise.
*/
static int open_logfile (void)
{
    log_fp = fopen (LOG_FILE, "a");
    if (!log_fp) {
        perror ("fopen()");
        return -1;
    }
    /* Be less efficient here, to solve a problem more important
     *  than mere efficiency
     */
    if (setvbuf (log_fp, 0, _IOLBF, 0)) {
        perror ("setvbuf()");
        fclose (log_fp);
        return -1;
    }
    log_fd = fileno (log_fp);
    if (log_fd == -1) {
        perror ("fileno()");
        fclose (log_fp);
        return -1;
    }
    return 0;
}

int main (void)
{
    static sigset_t caught_signals;

    sigemptyset (&caught_signals);

    static int const sig[] = {
        SIGALRM, SIGHUP, SIGINT,
        SIGPIPE, SIGQUIT, SIGTERM,
    };

    const size_t nsigs = ARRAY_CARDINALITY (sig);

    struct sigaction act;

    for (size_t i = 0; i < nsigs; i++) {
        if (sigaction (sig[i], 0, &act) == -1) {
            perror ("sigaction()");
            return EXIT_FAILURE;
        }

        if (act.sa_handler != SIG_IGN) {
            sigaddset (&caught_signals, sig[i]);
        }
    }
    act.sa_handler = sigint_handler;
    act.sa_mask = caught_signals;
    act.sa_flags = 0;

    for (size_t i = 0; i < nsigs; i++) {
        if (sigismember (&caught_signals, sig[i])) {
            if (sigaction (sig[i], &act, 0) == -1) {
                perror ("sigaction()");
                return EXIT_FAILURE;
            }
        }
    }
    if (open_logfile () == -1) {
        return EXIT_FAILURE;
    }
    int master_fd = 0;

    if ((master_fd = setup_server ()) == -1) {
        return EXIT_FAILURE;
    }
    /* Wait for and eventually handle a new connection */
    printf ("\nListening for connections on port %s.\n", PORT);

    if (handle_connections (master_fd) == -1) {
        close (master_fd);
        return EXIT_FAILURE;
    }
    close (master_fd);
    return EXIT_SUCCESS;
}

Dynamic analysis:

==387== Memcheck, a memory error detector
==387== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==387== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==387== Command: ./selectserver
==387== Parent PID: 325
==387==
==387== error calling PR_SET_PTRACER, vgdb might block
==387==
==387== Process terminating with default action of signal 2 (SIGINT)
==387==    at 0x497FF7A: select (select.c:41)
==387==    by 0x10A4FE: handle_connections (selectserver.c:372)
==387==    by 0x10AB96: main (selectserver.c:504)
==387==
==387== HEAP SUMMARY:
==387==     in use at exit: 472 bytes in 1 blocks
==387==   total heap usage: 111 allocs, 110 frees, 56,793 bytes allocated
==387==
==387== 472 bytes in 1 blocks are still reachable in loss record 1 of 1
==387==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==387==    by 0x48ED92D: __fopen_internal (iofopen.c:65)
==387==    by 0x48ED92D: fopen@@GLIBC_2.2.5 (iofopen.c:86)
==387==    by 0x10A884: open_logfile (selectserver.c:444)
==387==    by 0x10AB40: main (selectserver.c:494)
==387==
==387== LEAK SUMMARY:
==387==    definitely lost: 0 bytes in 0 blocks
==387==    indirectly lost: 0 bytes in 0 blocks
==387==      possibly lost: 0 bytes in 0 blocks
==387==    still reachable: 472 bytes in 1 blocks
==387==         suppressed: 0 bytes in 0 blocks
==387==
==387== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Edit: fixed typo.

Answer 1

Answers to the remaining 8 questions

1. Could any of these syscalls block?

That's an excellent question to ask for a program like this. There are lots of system calls in your code that can block, apart from select() of course which is intended to block. To name a few:

• printf(), fprintf() and perror(): even output to stdio or stderr can block. You might consider ignoring that though, by the time this blocks you might have bigger problems.
• fsync(): this actually does more than just flushing the stream buffer, it also waits until the data is guaranteed to be on the disk. This might take a significant amount of time, especially on a mechanical hard drive.
• getnameinfo(): this will do a DNS lookup which can take an arbitrary amount of time. Consider whether you need to have a symbolic hostname at all, or whether just the address in numeric form is good enough. You can get the latter by passing AI_NUMERICHOST | AI_NUMERICSERV for the last parameter.
• send(): this can easily block, consider for example that one of the peers might not be responding (maybe their network is down, someone paused the chat program, or any other number of reasons). Do you want to wait for that one peer and hold up passing the message to the remaining peers? To make this non-blocking, either set O_NONBLOCK on the socket, or pass MSG_DONTWAIT as a flag to send().
• recv(): obviously this blocks by default. But you might think you are safe, because you are using select(). However, if you read the manpage for select(), you'll notice it mentions that it can spuriously report that a filedescriptor is ready for reading, when in fact it isn't. It is safer to set the O_NONBLOCK flag on the socket, or pass MSG_DONTWAIT to recv().

2. Is using blocking sockets defeating (at least partially) the point of select()?

There might be reasons why you want the sockets to be blocking. For example, in a congested network, you might just want to let things wait and slow down until messages can be sent, instead of returning an error early.

3. Is the overhead of continuously malloc()ing and free()ing comparable in contrast to the overhead of syscalls?

malloc() and free() can cause system calls to happen as well. However, I would not worry about it too much: even if it does, you have at least as many calls to recv() as you have to realloc(), so you were paying for system calls anyway. If you believe it is a problem, consider using a profiling tool to find out where your program spends most of its CPU cycles. If it is in realloc(), you can consider finding ways to avoid allocating memory all the time, for example by keeping a pool of allocated memory around, and returning buf to that pool when it's no longer being used.

4. Do you see a potential buffer overflow / memory leak? (Other than the FILE * valgrind reports about, though that should be eliminated as well)

Yes. If realloc() fails after the first iteration of the do…while loop, you forget to free(buf). In handle_connections(), I also see two potential paths where line isn't freed: if !line but status != 0, and if status == -1.

As you can see, Valgrind doesn't catch those errors if those error paths are never exercised when running the program. Consider using static analysis tools like cppcheck, Clang's static analyzer and/or GCC's static analyzer. These can find bugs by analyzing all the possible paths through a program without having to actually run it. Note that even those cannot guarantee they find every possible bug.

6. Is there a better strategy with handling file descriptors above 1024, other than gracefully exiting or silently closing any new connections? (Eliminating the possibility of using poll() and the like)

Closing new connections that have a filedescriptor above FD_SETSIZE seems like the best option. You might consider printing a warning when that happens though, or even sending a message over that filedescriptor, in a non-blocking way, right before closing it, to inform the peer that tried to join that this server cannot handle any more connections at this moment.

7. Does any part of my code require a comment?

Ideally, your code is structured in a logical way, and has clear variable and function names such that the meaning of the code is obvious to a reader. In those cases where clear code is not enough to tell a reader what is going on, you should add comments.

I see you added some comments above functions, describing what they do and what the return values mean. That's great, but consider doing this in the Doxygen format. This allows the Doxygen tools to generate a reference manual for your code, and if you enable warnings those tools can even check that you didn't forget to document all functions, parameters and return values.

8. How do I avoid mixed messages? For instance:

Jack (typing): hel...
John (typing): nig...
Jack's screen: hellnight

By keeping a per-socket buffer, and only forwarding a message when it has been received completely. This will increase the complexity of your program of course, but there is no way around that.

9. Would it suffice to call fsync() before closing the file descriptor once, instead of calling it after every write()?

It depends on what you want to happen if the program is terminated abnormally. Do you need the log to be as complete as possible at all times, or is it OK if you miss part of it in the rare case that your program crashes or your computer loses power? You are already handling errors very gracefully now, and not calling exit() in the middle of the program. That means that if there are no bugs in your program that would cause it to crash, it will always exit from main() in an orderly fashion, in which case any open file descriptors will be flushed and closed automatically. So I would consider removing the calls to fsync() entirely.

Answer 2

Nice effort.

This review only addresses 2 of the 10 questions. I hope OP gets a review of the other 8 questions too.

---

5. Does any part of my code exhibit undefined behaviour?

Calls to time() and localtime() deserve to have their return values checked.

time_t time_val = flags & LOG_FULLTIME ? time (0) : 0;
if (time_val == -1) TBD_Code();
struct tm *tm_info = flags & LOG_FULLTIME ? localtime (&time_val) : 0;
if (tm_info == NULL && flags & LOG_FULLTIME) TBD_Code();

Use matching specifiers

size_t len = nbytes;
...
// fprintf (stderr, "... %ld bytes ...\n", ..., len);
fprintf (stderr, "... %zu bytes ...\n", ..., len);

---

10 How can I further improve my code?

Large buffer initialization

Initializing a large buffer and then calling snprintf() simply soaks up a bunch of CPU.

Instead consider:

// char log_txt[MAX_LOG_TEXT] = { 0 };
char log_txt[MAX_LOG_TEXT];
snprintf(log_txt, sizeof log_txt, ...

Logging arbitrary messages

With general purpose logging like fprintf (fp, "%s\n", msg);, consider sentinel characters as in fprintf (fp, "\"%s\"\n", msg); to clearly demarcate the message. Messages have a way of annoyingly having leading, trailing spaces and/or embedded line-feeds.

Spelling

lenght --> length.

Surprising declaration

void flush_cache (int fd); in log.h might be difficult to track its origin to "log.h". Maybe void log_flush_cache (int fd);?

Simplified initialization

//struct addrinfo hints;
//memset (&hints, 0x00, sizeof hints);
struct addrinfo hints = { 0 };

Unneeded casts

At least 5x.

// setsockopt(master_fd, SOL_SOCKET, SO_REUSEADDR, (void *) &yes, size_yes)
setsockopt(master_fd, SOL_SOCKET, SO_REUSEADDR, &yes, size_yes)

Leaked memory?

Shouldn't there exist a free (line); here?

} else if (status == -1) {
     FD_CLR (i, &master);
     close (i);

log_count?

Hmmm, if there are mixed calls to log_msg(), some with, some without LOG_COUNT, might it make sense to always ++log_count even when flags & LOG_COUNT == 0? Just a thought.

static?

Why static here? I'd reccomend:

// static const size_t page_size = BUFSIZE;
const size_t page_size = BUFSIZE;

Error checking

log_msg(FILE * fp, const char *msg, unsigned int flags) as a public function deserve some argument checking.

One test might be for msg == NULL. Or extend functionality and allow msg == NULL with:

 if (msg) fprintf (fp, "%s", msg);
 fprintf (fp, "\n");

Yet that might up prior fprintf() with their , " separators.

Why '\n' in fprintf (fp, "%lld\n, ", ++log_count);

Seems inconsistent to sometimes have 1 line of logging and other times 2.

log_msg() return value

All standard print functions return a value. I'd rework log_msg() to perform 1 fprintf() call and return its value.

Answer 3

Undefined Behavior:

if (flags & LOG_DATE
        && strftime (date_stamp, TS_BUF_LENGTH, "%F (%a)", tm_info)) {
        fprintf (fp, "%s, ", date_stamp);
}

If the call to localtime() failed, we would be passing a NULL pointer to strftime(), which would invoke undefined behavior.

Add a check for it:

if (flags & LOG_DATE && tm_info 
    && strftime (date_stamp, TS_BUF_LENGTH, "%F (%a)", tm_info)) {
    fprintf (fp, "%s, ", date_stamp);
}

Code duplicity:

In configure_tcp(), we have four identical, and hideous, calls to setsockopt(), albeit with different options. It breaks the programming principle DRY, which stands for Don't repeat yourself.

The DRY (don't repeat yourself) principle is a best practice in software development that recommends software engineers to do something once, and only once.

We could instead make a struct that would hold all the values and call setsockopt() in a loop.

static void configure_tcp (int slave_fd)
{
    struct option {
        int level;
        int opt_name;
        const void *opt_val;
    } const options[] = {
        { SOL_SOCKET, SO_KEEPALIVE, (int[]) { 1 } },
        { IPPROTO_TCP, TCP_KEEPCNT, (int[]) { 9 } },
        { IPPROTO_TCP, TCP_KEEPIDLE, (int[]) { 25 } },
        { IPPROTO_TCP, TCP_KEEPINTVL, (int[]) { 25 } },
    };

    for (size_t i = 0; i < ARRAY_CARDINALITY (options); i++) {
        if (setsockopt
            (slave_fd, options[i].level, options[i].opt_name,
             options[i].opt_val, sizeof (int)) == -1) {
            perror ("setsockopt()");
        }
    }
}

Similarly, there are two identical calls to fcntl() in set_socket_nonblock(). These too can be eliminated with a loop:

static int enable_nonblocking (int fd)
{
    int cmd = F_GETFL;
    int flags;

    for (flags = 0; flags = fcntl (fd, cmd, flags); flags |= O_NONBLOCK) {
         cmd = F_SETFL;
    }
    return flags;
}

The code is cluttered with various log/error messages. These can be collected in an array and indexed in the calls to fprintf() / err_ret():

/*
*       Different types of logs.
*/
static const char *const logs[] = {
    "%s: We only sent %zu bytes because of a send() error.\n",
#define SS_SEND_ERROR           0
    "%s: INFO: Socket %d hung up.",
#define SS_CLOSED_CONN          1
    "%s: This server can not handle any more connections at this moment.\n",
#define SS_CONN_SURPLUS         2
    "%s: Couldn't send the peer the full message.\n",
#define SS_FAILED_EXCUSE        3
    "%s: INFO: New connection from HOST:%s, SERVICE:%s,\n\t\t\t\t\t   LOCAL IP ADDRESS:%s, on socket %d.",
#define SS_NEW_CONN             4
    "%s: Server overloaded. Caution advised.\n",
#define SS_OVERLOAD             5
    "%s: Failed to setup a socket.\n",
#define SS_SOCKET_ERROR         6
    "%s: fclose() failed. Logs might have been lost.\n",
#define SS_FCLOSE_ERROR         7
    "\n%s:Listening for connections on port %s.\n",
#define SS_INITIATE             8
};

This eases maintenance. A sample call would now look like:

fprintf (log_fp, logs[SS_SEND_ERROR],
                         PROGRAM_NAME, len);

There are multiple calls to snprintf() and then log_msg() when a log is to be printed. We can encapsulate it in a variadic function and call it at will:

static void err_ret (FILE * stream, unsigned level, const char *fmt, ...)
{
    char buf[BUFSIZE];
    va_list argp;

    va_start (argp, fmt);
    vsnprintf (buf, sizeof buf, fmt, argp);
    LOG_MSG (stream, buf, level);

    va_end (argp);
}

Now, instead of writing this everytime:

char buffer[MAX_LOG_TXT];
snprintf (buffer, sizeof buffer, /* format string */, /* args */);
log_msg (log_fp, buffer, LOG_FULLTIME);
log_msg (stderr, log_txt, LOG_FULLTIME);

We have:

err_ret (log_fp, LOG_FULLTIME, logs[SS_SEND_ERROR],
         PROGRAM_NAME, len);

And the duplicated calls to log_msg() are eliminated with a macro having an identical name which expands to:

#define LOG_MSG(stream, msg, flags) log_msg (stream, msg, flags); \                                                                                  
                                    log_msg (0, msg, flags)

Simplify initialization:

struct addrinfo hints;

memset (&hints, 0x00, sizeof hints);
hints.ai_family = AF_UNSPEC;
hints.ai_socktype = SOCK_STREAM;
hints.ai_flags = AI_PASSIVE;

can be shortened to just:

const struct addrinfo hints = { .ai_family = AF_UNSPEC, .ai_socktype = SOCK_STREAM, 
                          .ai_flags = AI_PASSIVE };

Combine multiple conditions:

//for (int i = 0; i <= fd_max; i++) {
//        /* Send it to everyone */
//        if (FD_ISSET (i, &master))

for (int i = 0; i <= fd_max && FD_ISSET (i, &master); i++) {
     /* Send it to everyone. */
}

Vulnerable to DOS attack:

Currently, there's no facility to restrict one IP to just one connection at a time.