## `log.h` I don't think that calling code has any need for `TS_BUF_LENGHT`, so this could be moved to the implementation file. Consider spelling `LENGTH` the usual way, too. `FILE` and `uint8_t` are undefined. You need to include `<stdio.h>` and `<stdint.h>, as it's not possible to portably forward-declare these typedefs. Note that `uint8_t` is provided only when the underlying hardware has support for 8-bit values without padding. There's no good reason to restrict the number of flags so tightly this early in development - I recommend using plain `unsigned int` there. ## `log.c` We're missing declarations from `<time.h>` and `<stdio.h>`. Don't ignore the return value: if we're running in a locale where the output exceeds the provided maximum length, then > `strftime()` returns 0, and the contents of the array are **undefined**. (my emphasis). So check the result before using the buffer. We can reduce the work in this function by calling the time-related functions only when we're actually going to use the output: ``` void log_msg (FILE * fp, const char *msg, unsigned options) { time_t time_val = options & LOG_FULLTIME ? time (0) : 0; static long long log_count = 0; char time_stamp[TS_BUF_LENGTH]; char date_stamp[TS_BUF_LENGTH]; struct tm *tm_info = options & LOG_FULLTIME ? localtime (&time_val) : NULL; if (options & LOG_COUNT) { fprintf (fp, "%lld\n, ", ++log_count); } if (options & LOG_DATE && strftime (date_stamp, TS_BUF_LENGTH, "%F (%a)", tm_info)) { fprintf (fp, "%s, ", date_stamp); } if (options & LOG_TIME && strftime (time_stamp, TS_BUF_LENGTH, "%H:%M:%S", tm_info)) { fprintf (fp, "%s, ", time_stamp); } fprintf (fp, "%s\n", msg); } ``` If this program is to run as a daemon, it's probably better to allow use of `syslog()` - that takes care of re-opening the file during log rotation, for example. ## `selectserver.h` That's a lot of includes for a header file! None of them seem to be necessary (though it's polite to include `<stdio.h>` so the macros can be expanded). However, I don't think we need this header at all - the implementation is where `main()` is, so these definitions can simply all go into `selectserver.c`. `POSIX_C_SOURCE` should expand to a `long` constant, so it's best to define it as `200809L`. `ARRAY_CARDINALITY()` improperly protects its argument from precedence errors - we need `(x)[0]` rather than `(x[0])`: ``` #define ARRAY_CARDINALITY(x) (sizeof (x) / sizeof (x)[0]) ``` The `FFLUSH()` and `FSYNC()` macros are dangerous - use the `do`/`while(0)` idiom to protect against matching the wrong `else`: ``` #define FFLUSH(fp) do { if (fflush (fp) == EOF) perror ("fflush()"); } while (0) #define FSYNC(fd) do { if (fsync (fd) == -1) perror ("fsync()"); } while (0) ``` ## `selectserver.c` There's a lot of unnecessary resetting of `errno = 0`: in most cases, we test function return values and only access `errno` if the function has indicated that it has written to it. We can trust that these functions behave as advertised, and eliminate those effectively-dead writes. The signal handlers appear to be overkill, since we flush logs each time we write them anyway (and I think that responsibility ought to move to `log.c`, rather than mixed into the application code). In `init_addr()` we only ever assign to `status` and never use it. So that variable can be eliminated. In `init_sock()` we should close `master_fd` before we continue when `setsockopt()` fails. Actually, we shouldn't `continue` to the next iteration - just log the failure and attempt to bind regardless. Similarly, when we fail to `listen()`, we also leak the file descriptor. `accept_new_connection()` also leaks fds - whenever we return something other than `slave_fd`, we need to ensure that it is closed first. Again, consider proceeding despite failure to set options. In `read_line`, we have `if (!buf) { … free(buf); }`. That's not wrong, but is redundant. And the subsequent `status = 0` is pointless, as that's immediately followed by `return` (or perhaps `*status = 0` was intended - it's not clear what the status values mean). We could eliminate the duplication of the `realloc()` calls an consequent tests by starting with `buf = NULL` and moving the allocation to the beginning of the loop. I think this is wrong: > char *new = realloc (buf, page_size + page_size); Perhaps `total + page_size`? The redefinition of `ARRAY_CARDINALITY()` suffers the same problem as the first one.