Low level tcp socket

Question

Since I am a Unix newbie, I wanted to know if the code reflects the "unix" style and if there is something to add for it to be robust.

P. S. I hope I am not overusing assert calls.

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

#include <string.h>
#include <errno.h>
#include <assert.h>

#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>

#define whaterror strerror(errno)
#define PORT 0xbb8
#define BACKLOG 0xf // can't be > 128. 
#define BUFFSIZE 0x400

void init_server(struct sockaddr_in* server)
{
    assert(server != NULL && "Server pointer can't be null");

    server -> sin_family = AF_INET; // use IP.
    server -> sin_addr.s_addr = INADDR_ANY; // listen on any address (0.0.0.0).
    server -> sin_port = htons(PORT); // listen on PORT.
}

int main()
{
    struct sockaddr_in sock_in; // an inbound socket
    const int sockfd = socket(PF_INET, SOCK_STREAM, 0); // try to open the socket.
    assert(sockfd >= 0 && "Error in opening socket."); // check if socket opened.
    memset((char*)&sock_in, 0, sizeof(sock_in)); // reset sock_in to 0

    init_server(&sock_in); // initialize server struct
    assert(
        bind(sockfd, (const struct sockaddr*)&sock_in, sizeof(sock_in)) >= 0 &&
        "Port binding failed."
    );
    assert(
        listen(sockfd, BACKLOG) == 0 &&
        "Can't listen to the socket specified."
    );

    int bytes = 0;
    char* chunk = memset(malloc(BUFFSIZE), 0, BUFFSIZE);
    const int asocfd = accept(sockfd, NULL, 0);
    assert(asocfd >= 0 && "Error in accepting socket."); // check if socket opened.

    do {
        bytes = recv(asocfd, chunk, BUFFSIZE - 1, 0);
        memset(bytes == -1 ? chunk : chunk + bytes, 0, BUFFSIZE);
        !errno ?
            bytes && printf("%d\n%s\n", bytes, chunk)
            : fprintf(stderr, "ERRNO: %d [%s]\n", errno, whaterror);
    } while (errno == EAGAIN | bytes > 0);

    return 0;
}

Answer 1

1. You are definitely overusing, and misusing assert(). The problem is that it is something that should become a no-op when your code is compiled for release. As such, it should have NO side effects (like calling bind() or listen(), and should not be used for testing environmental errors. assert() should only be used for testing for programming errors. This means the use of assert() in init_server() is valid, but the rest aren't.

2. Your read loop is a bit twisted. I might write it like:

   for (;;) {
       bytes = recv(asocfd, chunk, BUFFSIZE, 0);
       if (bytes < 0) {
           fprintf(stderr, "ERRNO: %d [%s]\n", errno, whaterror);
           if (errno != EAGAIN) break;
       } else if (bytes == 0) {
           break;
       } else {
           printf("%d\n%.*s\n", bytes, bytes, chunk)
       }
   }
   

   Among other issues there:

   • Your memset() call overran your buffer, quite possibly by the entire length of the buffer.
   • Your memset() was unneeded if there was an error.
   • memset() is excessive just to add a single trailing NUL. It could have been written chunk[bytes] = 0; This is actually why you included the - 1 in the recv() call. (I removed the - 1 as telling printf() not to go past the length is equally effective.
   • Don't avoid if statements.

Answer 2

I hope I am not overusing assert calls

You do. assert is a debugging instrument. If NDEBUG macro is defined, the calls to assert are removed at the compile time. This is a usual practice for the production code.

• #define whaterror strerror(errno)

  I advise against it. The macro adds no value, and only obfuscates the code.

• memset(malloc

  Keep in mind that malloc may fail.

• memset(bytes == -1 ? chunk : chunk + bytes, 0, BUFFSIZE);

  Setting BUFFSIZE bytes to 0 writes beyond the allocated space. You need BUFFSIZE - bytes.

  On the other hand, clearing the entire tail of the chunk is not necessary. chunk[bytes] = 0; is enough.

• !errno ?, besides being another obfuscation, is quite wrong. errno is a way to inspect errors, not to detect them. In fact, quoting man errno,

  Successful calls never set errno; once set, it remains until another error occurs.

  recv returning -1 is one and only indication of error.

• Since the socket is not marked nonblocking, it may never return EAGAIN. On the other hand, some errors are transient, and should not break the loop.