C Socket Part-1

Question

In my ongoing attempts to become a better blog writer I have some written some more code that needs reviewing.

Full Source: https://github.com/Loki-Astari/Examples/tree/master/Version1
First Article: https://lokiastari.com/posts/SocketProgramminginC

This is a Simple Client Server implementation using RAW Sockets.

MakeFile

all:    client server
clean:
    rm -f *.o client server


CFLAGS      = -Wall -Wextra -pedantic -Werror

client: client.o
server: server.o

client.cpp

#include <arpa/inet.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#define CLIENT_BUFFER_SIZE     1024

int main(int argc, char* argv[])
{
    if (argc != 3)
    {
        fprintf(stderr, "Usage: client <host> <Message>\n");
        exit(1);
    }

    int socketId = socket(PF_INET, SOCK_STREAM, 0);

    struct sockaddr_in serverAddr;
    socklen_t addrSize = sizeof(serverAddr);
    bzero((char*)&serverAddr, sizeof(serverAddr));
    serverAddr.sin_family       = AF_INET;
    serverAddr.sin_port         = htons(8080);
    serverAddr.sin_addr.s_addr  = inet_addr(argv[1]);
    connect(socketId, (struct sockaddr*)&serverAddr, addrSize);

    write(socketId, argv[2], strlen(argv[2]));

    shutdown(socketId, SHUT_WR);

    char    buffer[CLIENT_BUFFER_SIZE];
    size_t  get = read(socketId, buffer, CLIENT_BUFFER_SIZE - 1);

    buffer[get] = '\0';
    fprintf(stdout, "%s %s\n", "Response from server", buffer);

    close(socketId);
}

server.cpp

#include <netinet/in.h>
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#define SERVER_BUFFER_SIZE      1024

int main()
{
    int socketId = socket(PF_INET, SOCK_STREAM, 0);

    struct sockaddr_in serverAddr;
    bzero((char*)&serverAddr, sizeof(serverAddr));
    serverAddr.sin_family       = AF_INET;
    serverAddr.sin_port         = htons(8080);
    serverAddr.sin_addr.s_addr  = INADDR_ANY;
    bind(socketId, (struct sockaddr *) &serverAddr, sizeof(serverAddr));

    listen(socketId, 5);

    int                         finished    = 0;
    while(!finished)
    {
        struct  sockaddr_storage    serverStorage;
        socklen_t                   addr_size   = sizeof serverStorage;
        int newSocket = accept(socketId, (struct sockaddr*)&serverStorage, &addr_size);

        char        buffer[SERVER_BUFFER_SIZE];
        int         get = read(newSocket, buffer, SERVER_BUFFER_SIZE - 1);

        buffer[get] = '\0';
        fprintf(stdout, "%s\n", buffer);

        write(newSocket, "OK", 2);

        fprintf(stdout, "Message Complete\n");

        close(newSocket);
    }
    close(socketId);
}

Answer 1

I can't resist saying this… the way you use a lot of whitespace to achieve your horizontal alignment, especially in server.cpp, is distracting and hinders readability.

I don't mind it so much when several lines have some relationship to each other. For example:

serverAddr.sin_family       = AF_INET;
serverAddr.sin_port         = htons(8080);
serverAddr.sin_addr.s_addr  = INADDR_ANY;

But for this? No, don't bother trying to horizontally align anything at all.

struct  sockaddr_storage    serverStorage;
socklen_t                   addr_size   = sizeof serverStorage;
int newSocket = accept(socketId, (struct sockaddr*)&serverStorage, &addr_size);

char        buffer[SERVER_BUFFER_SIZE];
int         get = read(newSocket, buffer, SERVER_BUFFER_SIZE - 1);

Answer 2

One of the issues with TCP sockets is that there's no guarantee that sending N bytes in one go will result in receiving exactly N bytes after one call to read(). I would thus emphasize this by using a separator in the messages (newline perhaps?) and only printing out the result of what I've read after the whole line has been received.

I'd also try to get rid of magic numbers. listen(socketId, 5); makes me look at the manual to figure out what 5 is supposed to mean. A carefully named constant would make reading the code easier.

Answer 3

Some issues that occur to me:

• The obvious problem is that there is no error checking.

• memset is often preferred to bzero, although I don't see why. Also the cast of serverAddr to char* in the bzero call is unnecessary.

• port number should be shared in a header file

• is inet_addr the correct way to get the address? Also note that it, too, can fail. And there is usually an memcpy involved in creating the address structure.

• does calling shutdown have any purpose?

• putting a 1k buffer on the stack would be a bad idea in some small systems

• the issue of read not returning all of what was written (or being interrupted) was already mentioned. Also read returns ssize_t not int or size_t.

• finished variable is redundant.

Trivia

• Usage message might use argv[0] instead of assumed executable name. Also the host must be a dotted address not a name.

• exit status might prefer EXIT_FAILURE

• I'd put "Response from server" in the format string.

Answer 4

There is a problem with the naming of your files. The .c extension is for the C language and the .cpp extension is for the C++ language. The tag on this question is c, so the expectation is the file name extension would be .c, not .cpp.

Most compilers that can handle both languages look at the file name extension to determine which language to compile.

The posted code needs to check the returned values from: socket(), bind(), read(), accept().

The variable finished is never updated, so it's not needed. Just use while (1).