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);
}

• slebetman: 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.
• Loki Astari: @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).

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);

• Loki Astari: I agree it does not work well there. Removing it.
• chux: @MartinYork Personalized spacing is a distraction. Rather than comment on it much. I tend to copy the source code, press about 5 keys to auto-format it using a very standard style and then review. If the reformatted code looks OK, all it well. It not, the code is not well adapted to auto-formatting. Save time. Do not manually format code. Auto-format it.

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.

• Loki Astari: Yes I know about the read. That is a deliberate mistake so I can explicitly talk about it in my blog and show how to do it correctly. lokiastari.com/blog/2016/04/09/socket-read
• chux: @MartinYork Good review etiquette: "deliberate mistake", for whatever reason, should be pointed out in the question or code.
• Loki Astari: Above Page movedd: localhost:3000/posts/SocketRead_Write

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.

• 200_success: The client calling shutdown() should tell the server that there is nothing more to read.
• Loki Astari: 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.
• glampert: @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).
• William Morris: Yes, I agree about EXIT_FAILURE, just being pedantic. Also I forgot to note that the finished variable is redundant.

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