2
\$\begingroup\$

This is a small socket library I made to avoid complicated C boilerplate code and several #includes for connecting sockets, bind, listening etc.

Please note that socket_create() does not support bind to specific IP addresses and that the IP parameter is ignored.

The only C++11 feature I am using is constexpr. Even though it is C++11, I decided not to wrap everything in a class, but to work with standard int.

In the future, I might add Unix domain socket in a way similar to socket_create() and / or SSL support, but at the moment I do not need those.

sockets.h

#ifndef NET_SOCKETS_H_
#define NET_SOCKETS_H_

#include <cstdint>

namespace net{

constexpr int SOCKET_NONBLOCK   = 0x01;
constexpr int SOCKET_TCPNODELAY = 0x02;

// ===========================

constexpr int SOCKET_DEFAULTOPT = SOCKET_NONBLOCK & SOCKET_TCPNODELAY;

// ===========================

int socket_create(const char *ip, uint16_t port, uint16_t backlog = 0, int options = SOCKET_DEFAULTOPT) noexcept;

bool socket_makeNonBlocking(int fd) noexcept;
bool socket_makeTCPNoDelay(int fd) noexcept;

int socket_accept(int fd) noexcept;

void socket_close(int fd) noexcept;

bool socket_check_eagain() noexcept;

}

#endif

sockets.cc

#include "sockets.h"

#include <sys/socket.h>     // EAGAIN
#include <arpa/inet.h>      // AF_INET
#include <netinet/tcp.h>    // TCP_NODELAY
#include <fcntl.h>      // fcntl

#include <unistd.h>     // close

#include <errno.h>      // errno

namespace net{



bool socket_check_eagain() noexcept{
    return errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK;
}

bool socket_makeNonBlocking(int const fd) noexcept{
    if (fd >= 0)
        return fcntl(fd, F_SETFL, O_NONBLOCK) >= 0;

    return false;
}

bool socket_makeTCPNoDelay(int const fd) noexcept{
    int const opt = 1;
    return (setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, & opt, sizeof opt) >= 0);
}

void socket_close(int const fd) noexcept{
    if (fd >= 0)
        ::close(fd);
}

int socket_accept(int const fd) noexcept{
    if (fd >= 0)
        return ::accept(fd, NULL, NULL);

    return -1;
}

int socket_create(const char *ip, uint16_t const port, uint16_t const backlog, int const options) noexcept{
    int fd = socket(AF_INET , SOCK_STREAM , 0);

    if(fd < 0)
        return -1;

    int const opt = 1;
    if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, & opt, sizeof opt) < 0){
        ::close(fd);
        return -2;
    }

    if (options & SOCKET_NONBLOCK)
    if (! socket_makeNonBlocking(fd) ){
        ::close(fd);
        return -3;
    }

    if (options & SOCKET_TCPNODELAY)
    if (! socket_makeTCPNoDelay(fd) ){
        ::close(fd);
        return -4;
    }

    struct sockaddr_in address;

    address.sin_family = AF_INET;
    address.sin_addr.s_addr = INADDR_ANY;
    address.sin_port = htons(port);

    if (bind(fd, (struct sockaddr *) & address, sizeof address) < 0){
        ::close(fd);
        return -5;
    }

    if (listen(fd, backlog ? backlog : SOMAXCONN) < 0){
        ::close(fd);
        return -6;
    }

    return fd;
}



} // namespace
\$\endgroup\$
6
\$\begingroup\$

NO. NO. NO. NO.

This does not do you any good none at all. In fact I think this is worse than doing nothing. All you have done is created a new layer that provides no advantages over the old and exposed a new set of error codes (that are not standard).

This is definitely NOT C++ code. You have just written another C layer that has all the same disadvantages of C and absolutely none of the benefits provided by C++.

The main thing to learn about C++ (in fact the overriding absolutely essential thing) is RAII. Resource acquisition is initialization. When you create an object it gets the resource, if it fails to get the resource during initialization you throw and the object does not exist and thus can not be used. Because the object only exists if it was correctly constructed most of the runtime checking to make sure the object is in the correct state disappears (because we know it is in the correct state because it exists).

I would expect the interface to a socket class to look like this:

Socket   webserver(8080);

while(webserver.accept(connection))
{
    InBoundConnection connection(webserver.buildConnection());

    connection.send(HttpObject(404, "<html><head>Miss</head><body>404 not found</body></html>"));
}
\$\endgroup\$
  • \$\begingroup\$ @JerryCoffin: Removed. I added that as an after though I was trying to emphasis there was a different style in writing C++ than there is for writing C. \$\endgroup\$ – Martin York Apr 8 '16 at 16:34
  • \$\begingroup\$ the purpose of the library is to pack the boilerplate code. I have single include now. Notice that functions are not even inlined. The code is packed into the .o file. Sure is not C++, just C++ syntax. Do you think code will benefit if I made it in pure C? I added link to some classes to show you where I am using the code. \$\endgroup\$ – Nick Apr 8 '16 at 16:40
  • \$\begingroup\$ @Nick: As it stands I don't think it has any benefits. You heave packed this boilerplate code. But the user of your code still has to write the same type of boiler plate code every time they use your code. Thus you have not provided any real benefit as the user just needs to learn a different abstraction layer. \$\endgroup\$ – Martin York Apr 8 '16 at 16:44
  • \$\begingroup\$ sure, but now you do just create_socket(), then do accep() or close() also check_eagain(). instead of copy paste all boring code everytime. this is similar to anet.h from redis. \$\endgroup\$ – Nick Apr 8 '16 at 16:47
  • 1
    \$\begingroup\$ @Nick: That is exactly the problem. You can JUST do that. The problem is when you do that its worse than the original code. You still need to check for the error codes and taking actions based on those. You are still leaving the code open to abuse because people don't check the error codes. So to use your code they still have to write the same boilerplate checks for all the possible errors. Your code is no different than the original it just adds a non standard layer. \$\endgroup\$ – Martin York Apr 8 '16 at 16:51
0
\$\begingroup\$

I think you should expose the values of your errors as either constants or some sort of enum.

You should keep the "negative is error, positive is socket descriptor" return value of socket_create, but it'd be better for users of your code to be able to check why socket creation failed. And that's better done by checking for SOCKET_MAKE_TCP_NO_DELAY_FAILED of some sort than -4.

\$\endgroup\$
  • \$\begingroup\$ You are correct. I did it with different values, but I actually never check what the error is. Will fix this because is good style. \$\endgroup\$ – Nick Apr 8 '16 at 9:00
  • \$\begingroup\$ check this pls: stackoverflow.com/questions/36496144/… \$\endgroup\$ – Nick Apr 8 '16 at 9:30
  • \$\begingroup\$ Error codes are fine internally. But error codes should NEVER be leaked from an interface boundary throw an exception if there is an error that leaks back into your users code. If you leak error codes you are allowing your code to be brittle as the user of the code must explicitly check for errors and if they don't then they still use your code in its bad state. By throwing an exception they can't use your code in a bad state unless they explicitly do something to compensate for the exception. \$\endgroup\$ – Martin York Apr 8 '16 at 15:22
  • \$\begingroup\$ Your advice would be perfectly correct for "C" but for C++ this is a terrible idea. Though the error code should be enum just not returned as the result (bit part of the exception that was thrown). \$\endgroup\$ – Martin York Apr 8 '16 at 15:37

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.