3
\$\begingroup\$

As someone who has programmed in C for a long time, I have just written my first C++ program and feel it would be very valuable to get a review of the code, to see what features of C++ I could make more use of.

The program provides a protocol for sending and receiving arrays of data over TCP. The idea is to

i) Send/receive the dimension of the array.
ii) Send/receive the shape of the array.
iii) Send/receive the data of the array.

To run the code you can do the following

g++ protocol.cpp -o protocol

Then in separate terminals, for example

./protocol server int32 float32

and

./protocol client int32 float32

The program itself is below

#include <stdlib.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <endian.h>
#include <unistd.h>
#include <iostream>
#include <cstring>

int is_system_le() {
    // Test if the system is little endian.
    short int word = 0x0001;
    char *b = (char *) &word;

    // In the little endian case, 1 will be read as 1 in a character.
    if (b[0]) {
        return 1;
    } else {
        return 0;
    }
}

template <class int_T, class data_T>
class Array {
 public:
    // Fields.
    int_T size;
    int_T dim;
    int_T *shape;
    data_T *data;

    // Methods.
    Array(int size, int dim);
    ~Array(void);

    int allocate(void);
};

template <class int_T, class data_T>
Array<int_T, data_T>::Array(int size, int dim) {
    // Set fields.
    this->size = size;
    this->dim = dim;
}

template <class int_T, class data_T>
int Array<int_T, data_T>::allocate() {
    // Create memory necessary for the array.
    this->shape = new int[dim];
    this->data = new data_T[size];

    return 0;
}

template <class int_T, class data_T>
Array<int_T, data_T>::~Array() {
    // Delete the created memory.
    delete[] shape;
    delete[] data;
}

class Protocol {
 public:
    // Class constants.
    static const int CHUNK_BYTES = 1024;

    // Fields.
    std::string socket_type;
    int verbose;

    // Methods.
    Protocol(std::string socket_type, int verbose);
    ~Protocol(void);

    int plisten(std::string host, int port);
    int pconnect(std::string host, int port);
    int pclose();

    template <class int_T, class data_T>
    int psend(
        Array<int_T, data_T> *arr,
        std::string int_format,
        std::string data_format
    );

    template <class int_T, class data_T>
    int preceive(
        Array<int_T, data_T> *arr,
        std::string int_format,
        std::string data_format
    );

 private:
    // Fields.
    int sock;
    int conn;

    // Methods.
    template <class int_T, class data_T>
    int psend_tcp(
        Array<int_T, data_T> *arr,
        std::string int_format,
        std::string data_format
    );

    template <class int_T, class data_T>
    int preceive_tcp(
        Array<int_T, data_T> *arr,
        std::string int_format,
        std::string data_format
    );
};

// Member functions.
Protocol::Protocol(std::string socket_type, int verbose) {
    if (verbose) {
        std::cout << "Protocol object created." << std::endl;
    }
    this->socket_type = socket_type;
    this->verbose = verbose;
}

Protocol::~Protocol(void) {
    if (verbose) {
        std::cout << "Protocol object deleted." << std::endl;
    }
}

int Protocol::plisten(std::string host, int port) {
    // Listen for a connection on the host/port.
    if (socket_type.compare("server") != 0) {
        throw "Error: Can only listen with server sockets.";
    }

    // Create the socket.
    sock = socket(AF_INET, SOCK_STREAM, 0);

    // Set up host address structure (don't understand this).
    struct sockaddr_in server_addr;
    server_addr.sin_family = AF_INET;
    server_addr.sin_port = htons(port);
    server_addr.sin_addr.s_addr = inet_addr(host.c_str());
    memset(server_addr.sin_zero, '\0', sizeof(server_addr.sin_zero));

    // Bind to host port.
    bind(sock, (struct sockaddr *) &server_addr, sizeof(server_addr));

    // Listen for connection.
    if (verbose) {
        std::cout << "Listening for connections." << std::endl;
    }
    listen(sock, 1);

    // Accept a connection.
    struct sockaddr_in client_addr;
    socklen_t client_addr_len = sizeof(client_addr);
    conn = accept(sock, (struct sockaddr *) &client_addr, &client_addr_len);
    if (verbose) {
        std::cout << "Accepted connection." << std::endl;
    }

    return 0;
}


int Protocol::pconnect(std::string host, int port) {
    // Connect to an actual socket.
    if (socket_type.compare("client") != 0) {
        throw "Error: Can only connect with client sockets.";
    }

    // Create the socket.
    sock = socket(AF_INET, SOCK_STREAM, 0);

    // Set up host address structure (don't understand this).
    struct sockaddr_in server_addr;
    server_addr.sin_family = AF_INET;
    server_addr.sin_port = htons(port);
    server_addr.sin_addr.s_addr = inet_addr(host.c_str());
    memset(server_addr.sin_zero, '\0', sizeof(server_addr.sin_zero));

    // Try to connect.
    if (verbose) {
        std::cout << "Trying to connect." << std::endl;
    }
    connect(
        sock,
        (struct sockaddr *) &server_addr,
        sizeof(server_addr)
    );
    if (verbose) {
        std::cout << "Accepted connection." << std::endl;
    }

    return 0;
}


int Protocol::pclose() {
    // Close a created server or client socket.
    if (socket_type.compare("server") == 0) {
        if (verbose) {
            std::cout << "Closing server socket." << std::endl;
        }

        close(sock);
    } else if (socket_type.compare("client") == 0) {
        if (verbose) {
            std::cout << "Closing client socket." << std::endl;
        }
        shutdown(sock, SHUT_WR);
        close(sock);
    }

    return 0;
}

template <class int_T, class data_T>
int Protocol::psend(Array<int_T, data_T> *arr, std::string int_format, std::string data_format) {
    // Send data in arr across the network.
    int ack;

    ack = psend_tcp(arr, int_format, data_format);

    if (ack != 0) {
        throw "Did not receive good acknowledgement after sending data.";
    }

    return 0;
}

template <class int_T, class data_T>
int Protocol::psend_tcp(Array<int_T, data_T> *arr, std::string int_format, std::string data_format) {
    // Send data in arr across the network using TCP.

    // This can certainly be cleaned up by using templates and factoring out
    // the function for sending chunked data over the network.

    // Note we always use little endian.
    uint8_t buffer[1024];
    size_t buffer_byte_size;

    // Server sends over conn, client over sock.
    int send_sock;
    if (socket_type.compare("server") == 0) {
        send_sock = conn;
    } else if (socket_type.compare("client") == 0) {
        send_sock = sock;
    }

    // i) Send the integer dimensions.
    if (int_format.compare("int32") == 0) {
        int32_t little_endian_int = (int32_t) htole32((uint32_t) arr->dim);
        memcpy(
            &buffer[0],
            &little_endian_int,
            sizeof(little_endian_int)
        );
        buffer_byte_size = sizeof(int32_t);
    } else if (int_format.compare("int64") == 0) {
        int64_t little_endian_int = (int64_t) htole64((uint64_t) arr->dim);
        memcpy(
            &buffer[0],
            &little_endian_int,
            sizeof(little_endian_int)
        );
        buffer_byte_size = sizeof(int64_t);
    }

    if (verbose) {
        std::cout << "Sending dimensions: " << int_format << std::endl;
    }
    send(send_sock, buffer, buffer_byte_size, 0);

    // ii) Send the integer shape. May need to be done in multiple chunks.
    if (int_format.compare("int32") == 0) {
        buffer_byte_size = sizeof(int32_t);
    } else if (int_format.compare("int64") == 0) {
        buffer_byte_size = sizeof(int64_t);
    }

    // Make sure our arr->shape is stored in int_format. In future can maybe
    // auto-fix by converting it.
    if (sizeof(int_T) != buffer_byte_size) {
        throw "Error: Array integers not in int_format so cannot send.";
    }

    // Make sure our host system is little endian as we don't bother converting
    // here. We could theoretically write to a buffer at convert.
    if (!is_system_le()) {
        throw "Currently rely on the system being little endian.";
    }
    int reps = arr->dim * buffer_byte_size/CHUNK_BYTES;
    int leftover = arr->dim * buffer_byte_size % CHUNK_BYTES;
    if (verbose) {
        std::cout << "Sending shape: " << int_format << std::endl;
    }
    for (int i=0; i<reps; i++) {
        memcpy(
            &buffer[0],
            &arr->shape[i*CHUNK_BYTES/buffer_byte_size],
            CHUNK_BYTES
        );
        send(send_sock, buffer, CHUNK_BYTES, 0);
    }

    // Send leftover bytes.
    memcpy(
        &buffer[0],
        &arr->shape[reps*CHUNK_BYTES/buffer_byte_size],
        leftover
    );
    send(send_sock, buffer, leftover, 0);

    // iii) Send the array data (I'm not sure if we can be sure of float and
    //      double size). Requires converting to data_format.
    if (data_format.compare("int32") == 0) {
        buffer_byte_size = sizeof(int32_t);
    } else if (data_format.compare("int64") == 0) {
        buffer_byte_size = sizeof(int64_t);
    } else if (data_format.compare("float32") == 0) {
        buffer_byte_size = sizeof(float);
    } else if (data_format.compare("float64") == 0) {
        buffer_byte_size = sizeof(double);
    }

    if (sizeof(data_T) != buffer_byte_size) {
        throw "Error: Array data not in data_format so cannot send.";
    }

    reps = arr->size * buffer_byte_size/CHUNK_BYTES;
    leftover = arr->size * buffer_byte_size % CHUNK_BYTES;
    if (verbose) {
        std::cout << "Sending data: " << data_format << std::endl;
    }
    for (int i=0; i<reps; i++) {
        memcpy(
            &buffer[0],
            &arr->data[i*CHUNK_BYTES/buffer_byte_size],
            CHUNK_BYTES
        );
        send(send_sock, buffer, CHUNK_BYTES, 0);
    }

    // Send lefotver bytes.
    memcpy(
        &buffer[0],
        &arr->data[reps*CHUNK_BYTES/buffer_byte_size],
        leftover
    );
    send(send_sock, buffer, leftover, 0);

    // Receive acknowledgement.
    recv(send_sock, buffer, 1, 0);

    return buffer[0];
}

template <class int_T, class data_T>
int Protocol::preceive(Array<int_T, data_T> *arr, std::string int_format, std::string data_format) {
    // Put the received value into arr.
    preceive_tcp(arr, int_format, data_format);

    return 0;
}

template <class int_T, class data_T>
int Protocol::preceive_tcp(Array <int_T, data_T> *arr, std::string int_format, std::string data_format) {
    // Receive network data over TCP.

    // Note we always use little endian.
    uint8_t buffer[1024];
    size_t buffer_byte_size;

    // Server receives over conn, client over sock.
    int send_sock;
    if (socket_type.compare("server") == 0) {
        send_sock = conn;
    } else if (socket_type.compare("client") == 0) {
        send_sock = sock;
    }

    // i) Receive integer dimension.
    if (int_format.compare("int32") == 0) {
        buffer_byte_size = sizeof(int32_t);
    } else {
        buffer_byte_size = sizeof(int64_t);
    }

    if (buffer_byte_size != sizeof(int_T)) {
        throw "Tried to receive data into an array with wrong integer format";
    }
    recv(send_sock, buffer, buffer_byte_size, 0);

    if (int_format.compare("int32") == 0) {
        int32_t little_endian_int = (int32_t) le32toh(((uint32_t *) buffer)[0]);
        arr->dim = (int) little_endian_int;
    } else if (int_format.compare("int64") == 0) {
        int64_t little_endian_int = (int64_t) le64toh(((uint64_t *) buffer)[0]);
        arr->dim = (int) little_endian_int;
    }

    if (verbose) {
        std::cout << "Received dimensions: " << arr->dim << std::endl;
    }

    // ii) Receive integer shape (may need to be done in chunks).
    arr->shape = new int[arr->dim];
    //arr->shape = (int_T *) malloc(arr->dim*sizeof(int_T));

    int reps = arr->dim * buffer_byte_size/CHUNK_BYTES;
    int leftover = arr->dim * buffer_byte_size % CHUNK_BYTES;
    for (int i=0; i<reps; i++) {
        recv(send_sock, &arr->shape[i*CHUNK_BYTES/buffer_byte_size], CHUNK_BYTES, 0);
    }

    // Recieve lefotver bytes.
    recv(send_sock, &arr->shape[reps*CHUNK_BYTES/buffer_byte_size], leftover, 0);

    if (verbose) {
        std::cout << "Received shape:" << std::endl;
        std::cout << " [";
        for (int i=0; i<arr->dim; i++) {
            std::cout << arr->shape[i];
            if (i != arr->dim - 1) {
                std::cout << ",";
            }
        }
        std::cout << "]" << std::endl;
    }

    // iii) Recieve data.
    if (data_format.compare("int32") == 0) {
        buffer_byte_size = sizeof(int32_t);
    } else if (data_format.compare("int64") == 0) {
        buffer_byte_size = sizeof(int64_t);
    } else if (data_format.compare("float32") == 0) {
        buffer_byte_size = sizeof(float);
    } else {
        buffer_byte_size = sizeof(double);
    }

    if (sizeof(data_T) != buffer_byte_size) {
        // Should check on full type not just bytes. Unfortunately don't know
        // how to do this in C++.
        throw "Error: Array data not in data_format so cannot receive.";
    }

    // Determine the size from the shape.
    arr->size = 1;
    for (int i=0; i<arr->dim; i++) {
        arr->size *= arr->shape[i];
    }

    arr->data = new data_T[arr->size];
    //arr->data = (data_T *) malloc(arr->size * sizeof(data_T));
    reps = arr->size * buffer_byte_size/CHUNK_BYTES;
    leftover = arr->size * buffer_byte_size % CHUNK_BYTES;
    for (int i=0; i<reps; i++) {
        recv(
            send_sock,
            &arr->data[i*CHUNK_BYTES/buffer_byte_size],
            CHUNK_BYTES,
            0
        );
    }

    // Receive leftover bytes.
    recv(
        send_sock,
        &arr->data[reps*CHUNK_BYTES/buffer_byte_size],
        leftover,
        0
    );

    if (verbose) {
        std::cout << "Received data:" << std::endl;
        if ((arr->size < 50) & (arr->dim == 2)) {
            for (int i=0; i<arr->shape[0]; i++) {
                for (int j=0; j<arr->shape[1]; j++) {
                    std::cout << arr->data[i*arr->shape[1] + j] << ", ";
                }
                std::cout << std::endl;
            }
        }
    }

    // No acknowledgement necessary for TCP however send anyway.
    buffer[0] = 0;
    send(send_sock, buffer, 1, 0);

    return 0;
}

int main(int argc, char *argv[]) {
    // Declare the protocol.
    std::string socket_type;
    if (argc == 1) {
        std::cout << "No socket type provided so using server." << std::endl;
        socket_type = "server";
    } else {
        std::cout << "Using protocol type " << argv[1] << "." << std::endl;
        socket_type = argv[1];
    }
    Protocol cp(socket_type, 1);

    std::cout << "Total chunk bytes: " << cp.CHUNK_BYTES << std::endl;
    std::cout << "Socket Type: " << cp.socket_type << std::endl;
    std::cout << "Verbose: " << cp.verbose << std::endl;

    if (cp.socket_type.compare("server") == 0) {
        try {
            cp.plisten("127.0.0.1", 65432);
        } catch (const char *msg) {
            std::cerr << msg << std::endl;
        }

        // Now send some random data (5 x 2 array).
        Array<int, float> arr(10, 2);
        arr.allocate();

        arr.shape[0] = 5;
        arr.shape[1] = 2;

        for (int i=0; i<10; i++) {
            if (!strcmp(argv[3], "float32") || !strcmp(argv[3], "float64")) {
                arr.data[i] = i + i*0.1;
            } else {
                arr.data[i] = i;
            }
        }
        cp.psend(&arr, argv[2], argv[3]);
    } else {
        try {
            cp.pconnect("127.0.0.1", 65432);
        } catch (const char *msg) {
            std::cerr << msg << std::endl;
        }

        // Now receive some random data.
        Array<int, float> arr(-1, -1);
        cp.preceive(&arr, argv[2], argv[3]);
    }

    cp.pclose();

    return 0;
}

Notes

1) One of the issues I had was that I felt it would be nicer for the Protocol::preceive method to return an Array instance but I didn't know how to pass the template to the method when none of the arguments used it. Instead I went for passing a pointer to an instance.

2) This had another issue that I wanted the Array constructor to allocate the memory necessary for holding the shape and data information. However the size of this memory is not known before the method is run. Hence I ended up not including this in my constructor. I would be interested in hearing if there is a way around this.

3) Note that a quirk of the program is that I have Protocol::psend and Protocol::psend_tcp methods (similarly for receiving). This is for a fairly good reason that I copied the logic of this program from a Python program I wrote that also allowed sending and receiving over UDP. I wanted to leave them like this in case I wanted to add this feature in future.

4) I thought it may be a good idea to wrap some of the C libraries (to do with sockets) in a namespace as I got quite a few name clashes (hence why I have the prefix p in all my methods). When I tried to do this however I ended up getting a compiler error so gave up and moved on.

\$\endgroup\$
2
\$\begingroup\$

Alternate Header Files
In C++ if you want to include stdlib.h or string.h the proper include statements are

#include <cstdlib>
#include <cstring>

C++ Container Classes
In C++ there is std::array that provides a built in array with iterators that might prove helpful. std::vector might be even more helpful since it is basically a variable sized array of any type.

Iterators have taken the place of pointers when using container classes. Iterators make for loops over containers safer because the container classes have 2 defined iterators as member, begin and end.

Old Style C Casts Versus new C++ Casts
C++ has 2 new forms of casting variables, static_cast and dynamic_cast. These casts perform error checking on the cast and make casting type safe. The cast in int is_system_le() is not type safe if it is changed to a static_cast. See the second answer of this stack overflow question for a workaround to the cast.

The third answer to the stack overflow question suggests using htonl() as a library function that performs the same test and is portable.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Reading your code is difficult because you have put all the features into a single class, the Protocol. It's better to split this class into several, each with its own responsibility.

Programming sockets using the C/POSIX API is something really ugly. You should hide away all these details in a class called Socket (and maybe a second class called ServerSocket). Then you can write:

Socket conn(host, port);
conn.psend("hello", 5);
if (!conn)
    std::cerr << "sending failed: " << conn.last_error() << "\n";

This is much nicer than dealing with raw sockets and struct inaddr.

On top of this Socket class you should build the LowLevelProtocol that knows how to encode an int32_t and in which order to send it over the wire:

LowLevelProtocol llproto(conn);
llproto.write_int32(12345678);
llproto.write_uint32(12345678);

Using this low-level protocol, you can finally build your encoding of arrays. At this point, you don't have to think about big endian or little endian anymore, you just tell the low-level protocol to send this, send that.

The benefit of this separation is that at each of these abstraction levels, there are only a few topics of interest. It easy to test these levels individually by connecting them to mocked-away objects that you only create during the test. For example, the LowLevelProtocol can not only write to a socket, it can also write to a std::ostringstream, and this one can be tested very easily.

Another benefit is that the high-level protocol now contains the rules of how an array is encoded, and nothing else. This makes it easy to understand this small part of the program.

| improve this answer | |
\$\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.