I am relatively new to socket programming, so I would like to ask if there is anything wrong with my code :) For the server socket I included the great new C++ lambdas, so you can start an initialized server using:server.run([&](serverSock * serv) {...}Moreover most things run in a new thread to be able to handle multiple clients if necessary. The readAll function is just for my project, because I tell the receiver that the message ended with the simple string END.Is there any bad practice, any possible bug in my code? I'm thankful for every tip! :)clientSock.h#ifndef CLIENTSOCK_H#define CLIENTSOCK_H#include <iostream>#include <future>#include <functional>#include <errno.h>#include <string.h>#include <strings.h>#include <sys/socket.h>#include <netinet/in.h>#include <netinet/tcp.h>#include <arpa/inet.h>#include <sys/types.h>#include <sys/stat.h>#include <netdb.h>#include <unistd.h>#include <sys/signal.h>using namespace std;class clientSock {  public:    clientSock(string host, unsigned int port);    clientSock();    clientSock(int sock);    ~clientSock();    bool hasError();    int connect(string host, unsigned int port);    int disconnect();    int write(string mesg);    string read();    string readAll();    string host;    unsigned int port;    bool connected;  protected:  private:    int enable_keepalive(int sock);    static const unsigned int buffSize = 1000;    int sockfd;//establish connection to ID distribution server    struct sockaddr_in servaddr;    char recv[buffSize];    struct hostent* server;};#endif // CLIENTSOCK_HclientSock.cpp#include clientSock.hclientSock::clientSock(string host, unsigned int port) {    connect(host, port);}clientSock::clientSock() {    connected = false;}clientSock::clientSock(int sock) {    sockfd = sock;    connected = true;}clientSock::~clientSock() {    //disconnect();}int clientSock::connect(string host, unsigned int port) {    clientSock::host = host;    clientSock::port = port;    bzero(&servaddr, sizeof(servaddr));    servaddr.sin_family = AF_INET;    server = gethostbyname(host.data());    bcopy((char*) server->h_addr, (char*) &servaddr.sin_addr.s_addr, server->h_length);    servaddr.sin_port = htons(port);    if(connected)        disconnect();    sockfd = socket(AF_INET, SOCK_STREAM, 0);    /*struct timeval tv;    tv.tv_sec = 5;//5 Secs Timeout    tv.tv_usec = 0;//Not init'ing this can cause strange errors    setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, (char*)&tv, sizeof(struct timeval));*/    enable_keepalive(sockfd);    for(size_t i = 0; i < 3; i++) { //try to connect 3 times        if(::connect(sockfd, (struct sockaddr*) &servaddr, sizeof(servaddr)) < 0)            cerr << Error on connecting:  << errno <<    << strerror(errno) << endl;        else {            connected = true;            return 0;        }    }    connected = false;    return 1;}bool clientSock::hasError() {    if(sockfd == -1)        return true;    int error = 0;    socklen_t len = sizeof(error);    int retval = getsockopt(sockfd, SOL_SOCKET, SO_ERROR, &error, &len);    if(retval != 0 || error != 0)        return true;    else        return false;}int clientSock::enable_keepalive(int sock) {    int yes = 1;    if(setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, &yes, sizeof(int)) == -1)        return -1;    int idle = 1;    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &idle, sizeof(int)) == -1)        return -1;    int interval = 1;    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPINTVL, &interval, sizeof(int)) == -1)        return -1;    int maxpkt = 10;    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT, &maxpkt, sizeof(int)) == -1)        return -1;    return 0;}int clientSock::disconnect() {    if(!connected)        return -1;    close(sockfd);    connected = false;    return 0;}int clientSock::write(string mesg) {    if(!connected)        return 1;    struct timeval tv;    tv.tv_sec = 10;    tv.tv_usec = 0;    fd_set writefds;    FD_ZERO(&writefds);    FD_SET(sockfd, &writefds);    //cout << w:  << mesg << endl;    int sentBytes = 0;    for(size_t i = 0; i < mesg.length(); i += sentBytes) {        FD_ZERO(&writefds);        FD_SET(sockfd, &writefds);        int rv = select(sockfd + 1, NULL, &writefds, NULL, &tv);        if(rv == -1)            cerr << errno <<    << strerror(errno) << endl;        else if(rv == 0)            sentBytes = 0;        else if(rv > 0 && FD_ISSET(sockfd, &writefds)) {            sentBytes = ::write(sockfd, mesg.substr(i, mesg.length() - i).c_str(), mesg.length() - i);            if(sentBytes == -1) {                cerr << Error sending IDs:  << errno <<    << strerror(errno) << endl;                return 1;            }        }    }    return 0;}string clientSock::read() {    if(!connected)        return ;    struct timeval tv;    tv.tv_sec = 10;    tv.tv_usec = 0;    fd_set readfds;    FD_ZERO(&readfds);    FD_SET(sockfd, &readfds);    string resp = ;    unsigned int n = 0;    do {        FD_ZERO(&readfds);        FD_SET(sockfd, &readfds);        int rv = select(sockfd + 1, &readfds, NULL, NULL, &tv);        if(rv <= -1)            cerr << errno <<    << strerror(errno) << endl;        else if(rv == 0)            break;        else if(rv > 0 && FD_ISSET(sockfd, &readfds)) {            int tn = ::read(sockfd, recv, buffSize - 1);//avoid signcompare warning            if(tn > 0) {                n = tn;                recv[n] = '\0';                string tResp(recv, n);                resp += tResp;            }            else if(tn == -1) {                if(errno == 11) { //get the good part of the received stuff also if the connection closed during receive -> happens sometimes with short messages                    string tResp(recv);                    if(tResp.find_first_not_of(abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890) == std::string::npos) //but only allow valid chars                        resp += tResp;                }                else                    cerr << errno <<    << strerror(errno) << endl;                break;            }            else                break;        }        else            cerr << ERROR: rv:  << rv << endl;    }    while(n >= buffSize - 1);    //if(resp != )    //cout << r:  << resp << endl;    return resp;}string clientSock::readAll() {    string full = read();    while(full.find(END) == string::npos)        full += read();    full = full.substr(0, full.find(END));    return full;}serverSock.h#ifndef SERVERSOCK_H#define SERVERSOCK_H#include <iostream>#include <future>#include <functional>#include <clientSock.h>#include <errno.h>#include <string.h>#include <strings.h>#include <sys/socket.h>#include <netinet/in.h>#include <arpa/inet.h>#include <sys/types.h>#include <sys/stat.h>#include <netdb.h>#include <unistd.h>using namespace std;class serverSock {  public:    serverSock(unsigned int port);    serverSock();    ~serverSock();    clientSock accept();    bool inited;    int init(unsigned int port);//!<Necessary only if not already done by using constructor with port    int start();    int run(std::function<void(serverSock* sock)> fn);    int runBlocking(std::function<void(serverSock* sock)> fn);    int stop();  protected:    future<void> thr;  private:    int enable_keepalive(int sock);    int sockfd;//start server to listen for clients to send them ids    socklen_t clilen;    struct sockaddr_in servaddr, cliaddr;    static const unsigned int buffSize = 1000;    char mesg[buffSize];    unsigned int listeningPort;    bool started;    bool stopSock;};#endif // SERVERSOCK_HserverSock.cpp#include serverSock.hserverSock::serverSock(unsigned int port) : stopSock(false) {    inited = false;    init(port);    inited = true;}serverSock::serverSock() {    listeningPort = 0;}serverSock::~serverSock() {    stop();}int serverSock::init(unsigned int port) {    listeningPort = port;    sockfd = socket(AF_INET, SOCK_STREAM, 0);    int yes = 1;    if(setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &yes, sizeof(yes)) == -1)        cerr << errno <<    << strerror(errno) << endl;    enable_keepalive(sockfd);    bzero(&servaddr, sizeof(servaddr));    servaddr.sin_family = AF_INET;    servaddr.sin_addr.s_addr = htonl(INADDR_ANY);    servaddr.sin_port = htons(port);    started = false;    return 0;}int serverSock::enable_keepalive(int sock) {    int yes = 1;    if(setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, &yes, sizeof(int)) == -1) {        cerr << errno <<    << strerror(errno) << endl;        return -1;    }    int idle = 1;    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPIDLE, &idle, sizeof(int)) == -1) {        cerr << errno <<    << strerror(errno) << endl;        return -1;    }    int interval = 1;    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPINTVL, &interval, sizeof(int)) == -1) {        cerr << errno <<    << strerror(errno) << endl;        return -1;    }    int maxpkt = 10;    if(setsockopt(sock, IPPROTO_TCP, TCP_KEEPCNT, &maxpkt, sizeof(int)) == -1) {        cerr << errno <<    << strerror(errno) << endl;        return -1;    }    return 0;}clientSock serverSock::accept() {    if(started) {        int newsockfd = ::accept(sockfd, (struct sockaddr*) &cliaddr, &clilen);        if(newsockfd == -1)            cerr << errno <<    << strerror(errno) << endl;        return clientSock(newsockfd);    }    else return clientSock();}int serverSock::start() {    if(listeningPort == 0) {        cerr << ERROR No port defined to listen to << endl;        return 1;    }    if(bind(sockfd, (struct sockaddr*) &servaddr, sizeof(servaddr)) < 0) {        cerr << errno <<    << strerror(errno) << endl;        return 1;    }    listen(sockfd, 5);    clilen = sizeof(cliaddr);    started = true;    stopSock = false;    return 0;}int serverSock::run(std::function<void(serverSock* sock)> fn) {    thr = std::async([this, fn]() {        while(!stopSock)            fn(this);    });    return 0;}int serverSock::runBlocking(std::function<void(serverSock* sock)> fn) {    while(!stopSock)        fn(this);    return 0;}int serverSock::stop() {    stopSock = true;    int res = close(sockfd);    if(res < 0)        cerr << errno <<    << strerror(errno) << endl;    return 0;}
C++ Linux TCP client/server socket wrapper
c++;c++11;linux;socket;tcp
DesignThis is a very thin C like wrapper around sockets. In my opinion there are a lot of style changes that are need to make this good C++ or usable by modern C++ library.Main issues:Two phase constructionError Codes escape public interface (use exception)You member variables have state not related to the object after construction.Your use of select() is incorrect for this context.I did not review the serverSock but most comments that apply to the clientSock also apply to the server socket.Self Plug / Further readingI started writing a series of blogs about Socket programming; Still not complete. But as part of that series you will find a simple socket wrapper class.Socket Programming in CSocket Read/WriteC++ Wrapper for SocketSocket ProtocolsThe code is:reviewed here - C++ Socket Part-2 source hereCode ReviewDon't use using namespaceusing namespace std;This is particularly bad here because it is in a header file. You should not use it in a source file either (bad habit).Doing so makes your code much more susceptible to bugs, read: Why is using namespace std considered bad practice?Putting it in a header file is worse because it can pollute other people's code (any code that includes your header file). If I include your header file in my code it could potentially break my code in subtle ways. Things like that get you band from projects.Its a bad habit and you should break it as soon as possible.The reason the Standard namespace is called std:: is so that it is short and therefore not much of a burden to use as a prefix in-front of Types/Objects.Public Member Variables are always a bad idea:Anybody can come along and change the state of your object without you being able to tell.class clientSock {  public:    .....    string host;    unsigned int port;    bool connected;Member objects should always be private with access controlled via public/protected member methods.Never Use Two Phase ConstructionAn object should be created correctly or not created at all. Never use the constructor to try and initialize an object that fails; then force the user to check to the state with an additional call. This will result in your users code having bugs as users of your code will forget to check the state.This is your second phase check.    bool hasError();If the constructor fails then throw an exception.Error CodesError codes are great inside your object to indicate error states internally. But error codes should never escape your public interface. Prefer to throw an exception rather than force the user of your object to check an error code to verify correctness.These return error codes.    int connect(string host, unsigned int port);    int disconnect();A user of your code will eventually forget to check an error code thus resulting in bug.If something goes wrong that you can't fix locally then throw an exception. If the user of the code does not catch the exception then it will correctly terminate the application (any other action is a bug without knowing the context). You can then find this in testing and add the appropriate code to fix the problem (or ignore it as appropriate).Semantics and Rule of ThreeThe compiler generates the copy constructor and copy assignment operator for you by default. So the following are not your only constructors.class clientSock {  public:    clientSock(string host, unsigned int port);    clientSock();    clientSock(int sock);    ~clientSock();Compiler generated:    clientSock(clientSock const&);    clientSock& operator=(clientSock const&);This allows:{    clientSock  sock1(google.com, 80);    clientSock  sock2(sock1);}// Destructor calls close on sock2// Destructor calls close on sock1 (same as sock1)// Probably not devastating but definitely untidy.Personally I would disable the copy semantics of the object.Conversely I would enable the move semantics of a socket object. So that it can be passed into functions etc.    clientSock(clientSock&&);             // Notice the double &&    clientSock& operator=(clientSock&&);Interface design    string readAll();Read everything? You have not defined a protocol at this level. So how do you know what everything is?Member Vs Local variableThese two are not really part of the state of the object.    struct sockaddr_in servaddr;    struct hostent* server;They are used to set up the connection. But are not used after that point. So not really much point in keeping them as members.Initializer ListclientSock::clientSock(string host, unsigned int port) {    connect(host, port);}OK I am not going to say its totally wrong. But I think you should set up member variables in the constructor before calling member functions. Currently the state of the object is random (for POD values) and member functions assume (especially public facing ones) assume that the state of the object is well defined.Now using member function to set the state is OK (if you have a couple of constructors and some large piece of common code). But I don't think this falls into the same category. So I would initialize the members then call connect().clientSock::clientSock(string host, unsigned int port)    : host()    , port(-1)    , connected(false)   // this is the important one as you check it in connect.    , sock(-1){    connect(host, port);}Good practice to initialize all members so they have defined values. In the next one port will have an indeterminate value.clientSock::clientSock() {    connected = false;}This one shows that host and port are not really part of the state of the object. They should be removed from the object. As soon as you have a socket them they are no longer relevant.clientSock::clientSock(int sock) {    sockfd = sock;    connected = true;}Why is the disconnect commented out?clientSock::~clientSock() {    //disconnect();}Maybe you need to check if it is connected before disconnecting.Member accesses    clientSock::host = host;    clientSock::port = port;This is a funny way of accessing member variables. Normally I would use this-> to specify the exact member (rather than className:: as that implies static members).But better yet is not never to shadow member variables. Shadowed variables will eventually cause a problem as you will forget to disambiguify them from the shadow and your compile will not warn you when you go wrong.A nice trick is to turn on your compiler warnings to tell you about shadowed variables and to treat them as errors.CommentsI hate bad comments. But this would be a nice place for a comment.    setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, (char*)&tv, sizeof(struct timeval));*/    enable_keepalive(sockfd);Magic NumbersPut magic numbers (like 3) into constants.    for(size_t i = 0; i < 3; i++) { //try to connect 3 times        if(::connect(sockfd, (struct sockaddr*) &servaddr, sizeof(servaddr)) < 0)            cerr << Error on connecting:  << errno <<    << strerror(errno) << endl;        else {            connected = true;            return 0;        }    }But why try and connect three times? Is there some reason you think things will change? If they do change what does that mean about the error that happened the first two times? This code is too low level to make decisions like this. If there is an error here you should be getting a bit of code that has much more context to make the decision to try and reconnect again.Boolean results with if    if(retval != 0 || error != 0)        return true;    else        return false;This can be much more logically represented as:    return (retval != 0 || error != 0);Is it really an error to try and disconnect when not connected?int clientSock::disconnect() {    if(!connected)        return -1;        // This is an error state?    close(sockfd);    connected = false;    return 0;}Pass parameters by const reference (when they are non-mutable objects).int clientSock::write(string mesg) {You just made a copy of mesg. If this is a large string then that is a waste of time since you will just be reading it.int clientSock::write(string const& mesg) {SelectYou are using select() incorrectly. It's not going to break anything as it is used here. But this is not what it is designed for.        FD_ZERO(&writefds);        FD_SET(sockfd, &writefds);        int rv = select(sockfd + 1, NULL, &writefds, NULL, &tv);The select() blocks waiting for one of the file descriptors to become available so that a using it will not block when it is used.But you are blocking on the select() so you are not gaining any advantage to using select here. The point of select() is when you have lots of sockets with lots of data being read/written. Then select() allows you to continuously read from sockets that are ready and not block waiting on sockets that have nothing yet.Remove select() from this context.Printing error messagesThis code is way too low level to be printing error messages.        if(rv == -1)            cerr << errno <<    << strerror(errno) << endl;Error messages should be sent to the user by a piece of code that understands the context under which the code is being used. If this code is being used as a server nobody will ever see this message. If this code is being used as a windows application nobody will ever see this message.Throw an exception if this is unrecoverable (with the message). Some higher level piece of code will get the message and put it in the appropriate place (log file std::cerr etc).Not all write errors are unrecoverable            if(sentBytes == -1) {                cerr << Error sending IDs:  << errno <<    << strerror(errno) << endl;                return 1;            }There are a couple of error messages from ::write that are not unrecoverable.https://linux.die.net/man/2/writeEAGAIN or EWOULDBLOCK or EINTR (EINTR being relatively common).A whole bunch of those error codes should never happen (assuming your socket code works). There are error messages that should never happen here (caused by bugs in the code (bad file descriptor)) and there are error messages that are transient (network cable was unplugged). You should distinguish between the two types in your errors.Success failure as 0/1?C++ has a bool type. If you want to pass success/failure state use a bool rather than a number (unless that number has other meaning). And don't let an error code escape a public interface.Read AllThis implies a protocol so you know where the end is.string clientSock::readAll() {    string full = read();    while(full.find(END) == string::npos)        full += read();    full = full.substr(0, full.find(END));    return full;}Which this code seems to do. But this is supposed to be low level socket code. It should not be handling protocols; this code should be in a derived class for the specific protocol that it understands.