HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

C Version of a Client/Server application

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
versionclientapplicationserver

Problem

Starting to write an article about Socket Programing.

So I need a simple C version of a client/server app.

So here it is for review (Also on github)

A linked question is the C++ version

MakeFile

all:    client server

.PHONY:         all
.INTERMEDIATE:  %.o

CFLAGS  += -Wall -Wextra -Werror -pedantic 

client: client.o common.o
server: server.o common.o

server.o:   server.c common.h
client.o:   client.c common.h
common.o:   common.c common.h


Common.h

#ifndef COMMON_H
#define COMMON_H

#include 

#define QUOTE_(X)       #X
#define QUOTE(X)        QUOTE_(X)

extern char const* socketError;

int serverSocket();
int clientSocket(char const* host);
int sendMessage(int socket, char const* buffer, size_t size);
int getMessage(int socket, char* buffer, size_t size);

#endif


common.cpp

```
#include "common.h"

#include
#include
#include
#include

char const* failedSocket = "Failed: socket()";
char const* failedBind = "Failed: bind()";
char const* failedListen = "Failed: listen()";
char const* failedConnect= "Failed: connect()";
char const* socketError = "None";

int serverSocket()
{
int socketId = socket(PF_INET, SOCK_STREAM, 0);
if (socketId == -1)
{
socketError = failedSocket;
return -1;
}

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;
if (bind(socketId, (struct sockaddr *) &serverAddr, sizeof(serverAddr)) != 0)
{
socketError = failedBind;
close(socketId);
return -1;
}

if(listen(socketId, 5) != 0)
{
socketError = failedListen;
close(socketId);
return -1;
}
return socketId;
}

int clientSocket(char const* host)
{
int socketId = socket(PF_INET, SOCK_STREAM, 0);
if (socketId == -1)
{
socketError = failedSocket;
return -1;
}

str

Solution

General
Nice clean presentation.

-
A potential infinite loop. Consider some limitation on repetitive EINTR conditions. I would expect a errno==0 before the write().

while(sentSize != size) {
  errno = 0;  // add
  size_t sent = write(socket, buffer + sentSize, size - sentSize);
  if (sent == -1u && errno == EINTR) {
    continue;
  }


-
Name space: The names of functions, variables and macros in common.h share no cohesive naming scheme. This makes it unclear of their source when used. The use of QUOTE() and common.h is something that easily could collide with other libraries. Suggest something like CVersion.h with CVersion_serverSocket(),CVersion_QUOTE(), etc.

-
Rather than extern char const socketError;, make it a function: char const socketError(); to prevent user code from changing it.

-
bzero() is not standard C, yet implementations I find use void bzero(void *s, size_t n);, thus the cast is not needed. Also parens not needed with sizeof variable

// bzero((char*)&serverAddr, sizeof(serverAddr));
bzero(&serverAddr, sizeof serverAddr );


-
Alternate initialization

//struct sockaddr_in serverAddr;
//bzero((char*)&serverAddr, sizeof(serverAddr));
struct sockaddr_in serverAddr = { 0} ;


-
Avoid naked magic numbers. Consider self-documenting code

// htons(8080)
#define PORT_DUJOUR 8080
htons(PORT_DUJOUR);

// listen(socketId, 5)
#define LISTIN_BACKLOG 5
listen(socketId, LISTIN_BACKLOG)


-
I have a suspicion that usage of format = get != SERVER_BUFFER_SIZE ? endBufferFormat : fullBufferFormat; may result in the final getMessage() not printing a ’\n’.

Minor

-
Recommend using bool for Boolean like variables.

// int finished    = 0;
//while(!finished)
#include 
bool finished  = false;
while(!finished)


-
Style: Prefer the explicit function signature int serverSocket(void) vs int serverSocket().

-
Style: The spacing used in the below adds little clarity, but adds typing. Suggest simplification.

// int         get;
// char        buffer[SERVER_BUFFER_SIZE];
int get;
char buffer[SERVER_BUFFER_SIZE];

Code Snippets

while(sentSize != size) {
  errno = 0;  // add
  size_t sent = write(socket, buffer + sentSize, size - sentSize);
  if (sent == -1u && errno == EINTR) {
    continue;
  }
// bzero((char*)&serverAddr, sizeof(serverAddr));
bzero(&serverAddr, sizeof serverAddr );
//struct sockaddr_in serverAddr;
//bzero((char*)&serverAddr, sizeof(serverAddr));
struct sockaddr_in serverAddr = { 0} ;
// htons(8080)
#define PORT_DUJOUR 8080
htons(PORT_DUJOUR);

// listen(socketId, 5)
#define LISTIN_BACKLOG 5
listen(socketId, LISTIN_BACKLOG)
// int finished    = 0;
//while(!finished)
#include <stdbool.h>
bool finished  = false;
while(!finished)

Context

StackExchange Code Review Q#125272, answer score: 3

Revisions (0)

No revisions yet.