6
\$\begingroup\$

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 <stddef.h>

#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 <arpa/inet.h>
#include <errno.h>
#include <string.h>
#include <unistd.h>

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

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

/*
 * Returns:
 *      0:      OK.     Message was sent with no errors.
 *      -1:     Error.  Something went wrong. See errno
 */
int sendMessage(int socket, char const* buffer, size_t size)
{
    size_t sentSize = 0;
    while(sentSize != size)
    {
        size_t sent = write(socket, buffer + sentSize, size - sentSize);
        if (sent == -1u && errno == EINTR)
        {
            continue;
        }
        if (sent == -1u)
        {
            return -1;
        }
        if (sent == 0)
        {
            return -1;
        }
        sentSize += sent;
    }
    return 0;
}

/*
 * Returns:
 *      0       OK.     End of Message. No data in buffer.
 *      >0      MORE.   Data in buffer. Value is number of bytes read.
 *      -1:     Error.  Something went wrong. See errno
 */
int getMessage(int socket, char* buffer, size_t size)
{
    size_t getSize = 0;
    do
    {
        size_t get = read(socket, buffer + getSize, size - getSize);
        if (get == -1u && errno == EINTR)
        {
            continue;
        }
        if (get == -1u)
        {
            return -1;
        }
        if (get == 0)
        {
            return getSize;
        }
        getSize += get;
    }
    while(getSize != size);
    return getSize;
}

server.c

#include "common.h"

#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 = serverSocket();
    if (socketId == -1)
    {
        fprintf(stderr, "%s\n%s\n", socketError, strerror(errno));
        exit(1);
    }
    struct  sockaddr_storage    serverStorage;
    socklen_t                   addr_size   = sizeof serverStorage;
    int                         finished    = 0;
    while(!finished)
    {
        int newSocket = accept(socketId, (struct sockaddr*)&serverStorage, &addr_size);
        if (newSocket == -1)
        {
            fprintf(stderr, "Failed: accept()\n%s\n", strerror(errno));
            close(socketId);
            exit(1);
        }

        static char const*  fullBufferFormat = "%." QUOTE(SERVER_BUFFER_SIZE) "s";
        static char const*  endBufferFormat  = "%s\n";

        int         get;
        char        buffer[SERVER_BUFFER_SIZE];
        while((get = getMessage(newSocket, buffer, SERVER_BUFFER_SIZE)) > 0)
        {
            char const* format = get != SERVER_BUFFER_SIZE ? endBufferFormat : fullBufferFormat;
            fprintf(stdout, format, buffer);
        }

        char const* msg     = get == 0 ? "OK" : "ERROR";
        if (sendMessage(newSocket, msg, strlen(msg) + 1) != 0)
        {
            fprintf(stderr, "Failed: sendMessage\n%s\n", strerror(errno));
            close(newSocket);
            close(socketId);
            exit(1);
        }
        fprintf(stdout, "Message Complete\n");

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

client.c

#include "common.h"

#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 = clientSocket(argv[1]);
    if (socketId == -1)
    {
        fprintf(stderr, "%s\n%s\n", socketError, strerror(errno));
        exit(1);
    }

    if (sendMessage(socketId, argv[2], strlen(argv[2]) + 1) != 0)
    {
        fprintf(stderr, "Failed: sendMessage()\n%s\n", strerror(errno));
        close(socketId);
        exit(1);
    }
    if (shutdown(socketId, SHUT_WR) != 0)
    {
        fprintf(stderr, "Failed: shutdown()\n%s\n", strerror(errno));
        close(socketId);
        exit(1);
    }

    static char const*  fullBufferFormat = "%s %." QUOTE(SERVER_BUFFER_SIZE) "s";
    static char const*  endBufferFormat  = "%s %s\n";

    int     get;
    char    buffer[CLIENT_BUFFER_SIZE];
    while((get = getMessage(socketId, buffer, CLIENT_BUFFER_SIZE)) > 0)
    {
        char const* format = get != CLIENT_BUFFER_SIZE ? endBufferFormat : fullBufferFormat;
        fprintf(stdout, format, "Response from server", buffer);
    }

    if (get == -1)
    {
        fprintf(stderr, "Failed: getMessage()\n%s\n", strerror(errno));
    }

    close(socketId);
}

CentOS 7:

From @pacmaninbw in the comments (so it is readable)

cc -Wall -Wextra -Werror -pedantic -c -o client.o client.c client.c
In function ‘main’:
client.c:19:5: error: ISO C90 forbids mixed declarations and code [-Werror=pedantic] int socketId = clientSocket(argv[1]); 
client.c:39:5: error: ISO C90 forbids mixed declarations and code [-Werror=pedantic] static char const* fullBufferFormat = "%s %." QUOTE(SERVER_BUFFER_SIZE) "s";
client.c:56:1: error: control reaches end of non-void function [-Werror=return-type] }
\$\endgroup\$
  • \$\begingroup\$ CentOS 7: cc -Wall -Wextra -Werror -pedantic -c -o client.o client.c client.c: In function ‘main’: client.c:19:5: error: ISO C90 forbids mixed declarations and code [-Werror=pedantic] int socketId = clientSocket(argv[1]); ^ client.c:39:5: error: ISO C90 forbids mixed declarations and code [-Werror=pedantic] static char const* fullBufferFormat = "%s %." QUOTE(SERVER_BUFFER_SIZE) "s"; ^ client.c:56:1: error: control reaches end of non-void function [-Werror=return-type] } ^ from the zip file on github \$\endgroup\$ – pacmaninbw Apr 10 '16 at 17:31
3
\$\begingroup\$

General Nice clean presentation.

  1. 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;
      }
    
  2. 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.

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

  4. 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 );
    
  5. Alternate initialization

    //struct sockaddr_in serverAddr;
    //bzero((char*)&serverAddr, sizeof(serverAddr));
    struct sockaddr_in serverAddr = { 0} ;
    
  6. 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)
    
  7. 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

  1. Recommend using bool for Boolean like variables.

    // int finished    = 0;
    //while(!finished)
    #include <stdbool.h>
    bool finished  = false;
    while(!finished)
    
  2. Style: Prefer the explicit function signature int serverSocket(void) vs int serverSocket().

  3. 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];
    
\$\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.