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.