[ccan] Networking: Asynchronous Server+Client socket code

[Rusty Russell]

Ahmed Samy <f.fallen45 at gmail.com> writes:

> Hi,
>
> This is a lightweight networking code that I'd like to add in The ccan tree.
> Reading the code should be straight forward, it uses epoll etc, and has
> support for callbacks, it can be used a server or a client with 3 simple
> calls:
>
>     struct socket_t *sock = socket_create();
>     if (!socket_listen(sock, NULL, 1337))
>         fatal("failed to listen on port 1337.");
>     /*
>      * callbacks should be setup before polling on the socket
>      */
>     socket_poll(sock);
>
>
> For client side, replace socket_listen with socket_connect.
> The code can be viewed at: at:
> https://github.com/otfallen/csnippets/blob/master/src/socket.c (to view the
> header change '.c' to '.h')
>
> Please leave any kind of feedback.
>
> Thanks
>       asamy

Hi Asamy,

        Sorry I didn't get to this earlier...  Here's some detailed
feedback, I hope it helps.  You've chosen a difficult first module :)

> #define STREAM_SERVER 0x1
> 
> #define MAX_EVENTS 1024

It's unclear how these are used...

> #define sock_cast(self) (struct socket_t *)(self)

If you didn't use void *self, you'd be able to avoid this, and get
the compiler to typecheck.

> struct socket_t {

_t here is weird: it's usually for typedefs, and I believe also
reserved.  Just 'struct socket' perhaps?

>     int fd;
>     char ip[16];
>     time_t idle;
>     int type;

Perhaps an enum for type?

>     struct {
>         void (*on_read) (void *self, const char *read, int len);
>         void (*on_accept) (void *self, void *new_socket);
>         void (*on_write) (void *self, const char *written, int len);
>         void (*on_disconnect) (void *self);
>         void (*on_connect) (void *self);
>     };

Anonymous structs are not that portable, but that's OK.  I assume these
are the user-settable callbacks...

>     struct {
>         int (*write) (void *self, const char *data, int len);
>         int (*close) (void *self);
>         int (*shutdown) (void *self);
>     };

These are always set to the same thing, right?  Why are they pointers?

> 
>     struct list_node node;
>     struct list_head children;
> };

Ah, I see you create a tree off your listening socket.  Perhaps you want
to differentiate the two types more clearly, by having a struct
listening_socket and a struct connected_socket, both starting with a
common struct socket?

> extern struct socket_t *__attribute__((warn_unused_result))
> socket_create(void);

I dislike warn_unused_result; it's a mistake anyone will actually make.

> extern void socket_close(struct socket_t *socket);

This might be better called socket_free().

> extern bool socket_connect(struct socket_t *socket, const char *addr,
>         int32_t port);

You might want to used ccan/net here (you may need to adapt it).

> extern bool socket_listen(struct socket_t *socket, const char *address,
>         int32_t port);

Why not combine these functions with socket_create(), and have them make
and (start to?) connect the socket?

> extern void socket_poll(struct socket_t *socket);

This only works because you internally keep track of the children.  That
may be a bit limiting in future (you might want to listen on two sockets
at once).  But sufficient for simple programs.

> #include "socket.h"
> #include "platform.h"

I prefer to test for individual features, but we could fix that up later.

> #ifdef _WIN32
> #include <winsock2.h>
> #include <ws2tcpip.h>    /* socklen_t */
> #else

It would be nice for configurator to turn this into HAVE_WINDOWS_API,
which is the style used by most of config.h.

> #include <sys/socket.h>
> #include <arpa/inet.h>
> #include <unistd.h>
> #include <netdb.h>
> #include <errno.h>
> #endif
> #include <fcntl.h>
> #include <stdarg.h>
> #include <sys/time.h>
> #include <time.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
> 
> extern void __socket_set_init(const struct socket_t *socket) __attribute__((weak));

You don't want weak here; you're prefer it not to link if there's no
implementation, rather than crash.

> extern void __socket_set_deinit(void) __attribute__((weak));
> extern void __socket_set_add(const struct socket_t *socket) __attribute__((weak));
> extern void __socket_set_del(const struct socket_t *socket) __attribute__((weak));
> extern int  __socket_set_poll(const struct socket_t *socket) __attribute__((weak));
> extern int  __socket_set_get_active_fd(int i) __attribute__((weak));
> 
> #ifdef _WIN32
> static bool is_initialized = false;
> #endif

> static int __write(void *self, const char *data, int len)
> {
>     int32_t ret;
>     struct socket_t *socket = sock_cast(self);
>     if (!socket)
>         return -1;

Seems to me that you should just crash if they hand you NULL.

>     socket->write = __write;
>     socket->close = __close;
>     socket->idle  = time(NULL);

So, this is really "last_active" not idle?

> bool socket_connect(struct socket_t *socket, const char *addr, int32_t port)
> {
>     time_t start;
>     struct hostent *hp;
>     bool connected = false;
>     struct sockaddr_in srv;
> 
>     if (socket->fd < -1)
>         return false;
>     if (!(hp = gethostbyname(addr)))
>         return false;
> #ifdef WIN32
>     memcpy((char*)&srv.sin_addr, (char*)hp->h_addr, hp->h_length);
> #else
>     bcopy((char*)hp->h_addr, (char*)&srv.sin_addr, hp->h_length);
> #endif
> 
>     srv.sin_family = AF_INET;
>     srv.sin_port = htons(port);

IPv6 will hate you.  Recommend ccan/net/.

>     start = time(NULL);
>     while (!connected && time(NULL) - start < 10) {

10 seconds?  You're nonblocking, so why spin at all?  You'll only be
spinning around for 10 seconds if you're failing to connect the whole
time.

> bool socket_listen(struct socket_t *socket, const char *address, int32_t port)
> {
>     struct sockaddr_in srv;
>     int reuse_addr = 1;
>     if (!socket)
>         return false;
>     if (socket->fd < 0)
>         return false;
> 
>     srv.sin_family = AF_INET;
>     srv.sin_addr.s_addr = !address ? INADDR_ANY : inet_addr(address);
>     srv.sin_port = htons(port);
> 
>     setsockopt(socket->fd, SOL_SOCKET, SO_REUSEADDR, &reuse_addr, sizeof(reuse_addr));
>     if (bind(socket->fd, (struct sockaddr *)&srv, sizeof(srv)) == -1)
>         return false;
>     if (listen(socket->fd, MAX_EVENTS) == -1)
>         return false; 
>     socket->type = STREAM_SERVER;
>     return true;
> }

We should probably add a net_server_lookup() for this case.

> void socket_poll(struct socket_t *socket)
> {
>     int n_fds, i;
>     __socket_set_init(socket);
>     __socket_set_add(socket);
>     for (;;) {
>         n_fds = __socket_set_poll(socket);
>         for (i = 0; i < n_fds; i++) {
>             struct socket_t *sock = NULL;
>             int active_fd = __socket_set_get_active_fd(i);
>             if (active_fd < 0)
>                 continue;

OK, so this idea is that this is the main loop of your program, as it
never returns?

It doesn't handle failure (eg. failure to create epoll fd) gracefully.

>             if (socket->fd == active_fd) {
>                 if (socket->type == STREAM_SERVER) {
>                     struct socket_t *new_socket;
>                     struct sockaddr_in their_addr;
>                     socklen_t len = sizeof(their_addr);
>                     int their_fd;
> 
>                     their_fd = accept(socket->fd, (struct sockaddr *)&their_addr, &len);
>                     if (their_fd == -1) {
>                         if (ERRNO != E_AGAIN && ERRNO != E_BLOCK)
>                             perror("accept()");
>                         break;
>                     }
> 
>                     new_socket = malloc(sizeof(*new_socket));
>                     if (!new_socket)
>                         break;

Leaks their_fd; if you really want to handle allocation failures, your
life will be more difficult.

>                     setup_socket(new_socket);
>                     setup_async(new_socket);
> 
>                     new_socket->fd = their_fd;
>                     strncpy(new_socket->ip, inet_ntoa(their_addr.sin_addr), 16);
> 
>                     if (socket->on_accept) {
>                         socket->on_accept(socket, new_socket);
>                         if (new_socket->on_connect)
>                             new_socket->on_connect(new_socket);
>                     }
>                     list_add(&socket->children, &new_socket->node);
>                     __socket_set_add(new_socket);
>                     continue;
>                 }
>                 sock=socket;

Why do you set sock here?

>             if (socket->type == STREAM_SERVER && !list_empty(&socket->children)) {
>                 list_for_each(&socket->children, sock, node) {
>                     if (sock->fd == active_fd)
>                         break;
>                 }
>             }

Why the if()?  Surely list_for_each() will do nothing of there are no
children.

>             bool done = false;
>             for (;;) {
>                 ssize_t count;
>                 char buffer[4096];
> 
>                 count = read(active_fd, buffer, sizeof buffer);
>                 if (count == -1) {
>                     if (ERRNO != E_AGAIN && errno != E_BLOCK)
>                         done = true;
>                     break;
>                 } else if (count == 0) { /* EOF */
>                     done = true;
>                     break;
>                 }
> 
>                 if (sock && sock->on_read)
>                     sock->on_read(sock, buffer, count);
>             }
>             if (done && sock) {
>                 list_del(&sock->node);
>                 sock->close(sock);
>             }
>         }
>     }
> 
>     __socket_set_deinit();
>     socket->close(socket);

AFAICT these lines can never be executed.

> }
> 
> void socket_close(struct socket_t *socket)
> {
>     if (!socket)
>        return;
> 
>     socket->close(socket);
>     free(socket);
> }

You should probably use this internally yourself, rather than calling
socket->close() & free manually.

Hope that helps,
Rusty.
PS.  And of course, documentation and tests would be nice :)