# C++ Poll Socket Wrapper Linux

So I created a C++ socket wrapper class using poll and events. The creation of the listening socket and binding is already handled in the initialize() method. I'd mainly like to get feedback on the socketLoop() method, as it is the most complex one. If there are any bad habits, unhandled exceptions, bugs or anything else I can improve, please let me know. I appreciate strong criticism, feel free to be picky!

bool NetworkSocket::socketLoop(){

// Start listening on specified port
if (listen(this->listeningSocket, this->backlog) < 0) {
this->raiseSocketExceptionEvent("Failed to start listening.");
return false;
}

std::cout << "Listening for connections on port " << this->listening_port << std::endl;

// Setup pollfd set and add listening socket to the set
this->pfd.fd = this->listeningSocket; //
this->pfd.events = POLLIN;
this->pfd.revents = 0;

while(this->run){

char buf[256];

// Poll on all file descriptors
this->raiseSocketExceptionEvent("poll() failed");
return false;
}

// Loop through all descriptors and check which ones are readable
for (int j = 0; j <  this->readfds.size(); j++) {

// If its the listening socket continue
continue;

// There is data to read?

// If its on the listening socket its an incomin connection
if (sd == this->listeningSocket) {

// Accept new connection
if (new_socket < 0) {
this->raiseSocketExceptionEvent("Failed to accept incoming connection.");
return false;
}

// Information about the new connection
this->raiseClientConnectedEvent(new_socket);

// Add new connection to fdset
this->pfd.fd = new_socket;
this->pfd.events = POLLIN | POLLRDHUP;
this->pfd.revents = 0;

// Add new connection to vectors
this->clients.push_back(new_socket);

} else {

// Recieve data on this connection until it fails
ssize_t rc = recv(sd, buf, sizeof(buf), 0);
if (rc > 0) {

std::vector<char> data(static_cast<unsigned long>(rc));
for (int i = 0; i < rc; i++) {
data[i] = buf[i];
}

this->raiseDataReceivedEvent(data); // ToDo replace with Packet structure later
}
// Connection was closed by the client
else if (rc == 0) {
}
else {
}
}
}

// If revents is not POLLIN its an unexpected result, exit
if (sd == this->listeningSocket) {
std::cout << "Unknown exception..?" << std::endl;
} else {
} else {
this->raiseClientDisconnectedEvent(sd);
}
close(sd);
this->clients.erase(std::find(this->clients.begin(),this->clients.end(), sd));
continue;
}
}
}
}
return true;
}


Alright as requested, here is the code of the class header so you get a better understanding of the class structure:

#ifndef NETWORKSOCKET_H
#define NETWORKSOCKET_H

#include <sys/ioctl.h>
#include <iostream>
#include <sys/socket.h>
#include <netinet/in.h>
#include <vector>
#include <cstring>
#include <arpa/inet.h>
#include <unistd.h>
#include <cerrno>
#include "NetworkEventListener.h"
#include <algorithm>
#include <poll.h>

class NetworkSocket
{
public:
NetworkSocket(uint16_t port);
~NetworkSocket();

bool initialize();
void start();
bool socketLoop();
void stop();

void removeEventListener(NetworkEventListener *listener);
bool run = true; // testing only will be replaced with getter/setter
private:

void raiseDataSendEvent(const std::vector<char> &data) const;
void raiseClientConnectedEvent(int socket) const;
void raiseClientDisconnectedEvent(int socket) const;
void raiseSocketExceptionEvent(std::string message) const;

std::vector<NetworkEventListener *> listeners;

uint16_t listening_port = 0;
int listeningSocket;
int socketopts = 1;
int backlog;

fd_set master_fd_set;
std::vector<int> clients;
pollfd pfd;

};

void socketLoopAuxiliary(NetworkSocket * socket);

#endif // NETWORKSOCKET_H

• Welcome to Code Review! It might help you get better reviews if you provide the code for the header file. – Null Mar 28 '18 at 3:30
• @Null Thanks for the welcome. As requested I added the header file into my post and edited my description. – Kyu96 Mar 28 '18 at 3:55
• @Null After 3 days of waiting and no replies, I wanted to ask if there is a way to get more intention to my post? – Kyu96 Mar 30 '18 at 15:23
• You could post a bounty if you had more rep. You still might not get an answer, though. I have unanswered questions, too. – Null Mar 30 '18 at 17:15
• This is not SO. Answers here usually take more time, codereviews should not be rushed. Above all there is no guarantee someone will review your code at all. – yuri Mar 31 '18 at 9:10

I'd lose the this-> as a rule. At least in my opinion, it adds visual noise without contributing anything positive to understanding the code.

    // If its the listening socket continue
continue;


This comment doesn't look to me like it matches what the code is doing. Rather than checking whether this is the listening socket, it's checking whether this socket actually has an event to process (and if not, basically ignore it).

### Use constructors where appropriate

For example, in this code:

                // Add new connection to fdset
this->pfd.fd = new_socket;
this->pfd.events = POLLIN | POLLRDHUP;
this->pfd.revents = 0;


...The NetworkSocket knows all about the internals of the pfd. As a rule, I'd prefer code that encapsulated the details of a pfd into the pfd itself, so this ended up something like:

readfds.push_back(new pfd(new_socket, POLLIN | POLLRDHUP));
clients.push_back(new_socket);


### Variable Locality

At least from the looks of things, NetworkSocket::pfd is really only needed as a kind of temporary item while accepting a new connection. If that's correct, then it should probably be local to the code that accepts a new connection, rather than being part of the whole object.

### Code Structure

My immediate reaction is that socketLoop is really too large, with too many pieces that are only somewhat related all packed together into one place.

I'd rather some of the pieces of functionality inside of socketLoop were broken out into functions of their own, and it mostly called them to do the dirty work, so to speak. That would reduce the code for socketLoop to something like this:

while(run){
char buf[256];

// Poll on all file descriptors
raiseSocketExceptionEvent("poll() failed");
return false;
}

for (int j = 0; j <  readfds.size(); j++) {

continue;

if (sd == listeningSocket) {
accept_connection(sd);
}
else {
}
}
else {
handle_error(sd);
}
}
}


Maybe I'm not not terribly bright, but I find something like this quite a bit easier to follow.

### Use range-based for loop where applicable

A loop like this:

for (int j = 0; j <  readfds.size(); j++)


...can usually be changed to a range-based for loop like this:

for (auto &fd : readfds)


...then inside the loop, fd will be equivalent to readfds[j] in your loop, so you'd use (for example):

for (auto &fd : readfds)

if (fd.revents == 0)
continue;


Among other things, this tends to be less prone to errors (such as using the size of one collection, but indexing into a different collection).

### Prefer Data to Code

                else if (rc == 0) {

int values = { POLLERR, POLLHUP};

• Thanks a lot for your answer! I am currently applying some of the mentioned changeds, however there are also some things which I don't understand. You suggest readfds.push_back(new pfd(new_socket, POLLIN | POLLRDHUP)); which is not possible because pollfd is a struct, therefore it will prompt an exception expected type-specifier before 'pfd'. – Kyu96 Mar 31 '18 at 8:04