4
\$\begingroup\$

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: http://lokiastari.com/blog/2016/04/08/socket-programming-in-c-version-1/

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);
}
\$\endgroup\$
  • \$\begingroup\$ This server isn't capable of handling multiple connections. Are you aware of that? If that is one of the issues you'll be talking about I'd appreciate it if you show the non-blocking/event-oriented code using select (or some event library like libevent) along with the more common threaded approach. It's how fast servers are written these days - from node.js to lighthttpd to HAProxy. I generally consider the naive thread approach a mistake. You may want to talk about thread pools around event loops but that's a slightly more advanced topic. \$\endgroup\$ – slebetman Jun 6 '16 at 3:13
  • \$\begingroup\$ @slebetman: Yes. I was going to mention select (and it being a bit old with some issues) but prefer to use libevent. But before I get there I want to implement a threaded version (then show the event driven version to show that it is better). \$\endgroup\$ – Martin York Jun 6 '16 at 18:53
15
\$\begingroup\$

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);
\$\endgroup\$
  • \$\begingroup\$ I agree it does not work well there. Removing it. \$\endgroup\$ – Martin York Jun 5 '16 at 22:19
11
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ The client calling shutdown() should tell the server that there is nothing more to read. \$\endgroup\$ – 200_success Jun 6 '16 at 18:45
  • \$\begingroup\$ The EXIT_FAILURE macro is so ugly. But yes. @glampert also mentioned the bzero() issue in a review of the C++ version. Apparently its archaic and non-standard codereview.stackexchange.com/a/131172/507 Apart from shutdown I will address all these issues. Thanks. \$\endgroup\$ – Martin York Jun 6 '16 at 18:46
  • \$\begingroup\$ @LokiAstari, those are my words, hehe. But here is a more in-depth discussion: stackoverflow.com/questions/17096990/why-use-bzero-over-memset (the archaic part is my own opinion, but the function is not part of the C or C++ standard). \$\endgroup\$ – glampert Jun 6 '16 at 19:21
  • \$\begingroup\$ Yes, I agree about EXIT_FAILURE, just being pedantic. Also I forgot to note that the finished variable is redundant. \$\endgroup\$ – William Morris Jun 6 '16 at 19:44
3
\$\begingroup\$

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 , 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).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.