[systemd-devel] Early review request: socket activation bridge

[Lennart Poettering]

On Thu, 10.10.13 21:54, David Strauss (david at davidstrauss.net) wrote:

> #define __STDC_FORMAT_MACROS

This is an unnecessary C++ism, isn't it?

> #include <errno.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <netdb.h>
> #include <sys/fcntl.h>
> #include <sys/socket.h>
> #include <sys/un.h>
> #include <unistd.h>
> 
> #include "log.h"
> #include "sd-daemon.h"
> #include "sd-event.h"
> 
> #define BUFFER_SIZE 1024
> 
> unsigned int total_clients = 0;
> 
> struct proxy {
>     int listen_fd;
>     bool remote_is_inet;
>     const char *remote_host;
>     const char *remote_service;
> };

Indenting to 8 spaces please please.

> 
> struct connection {
>     int origin_fd;
>     int destination_fd;
>     sd_event_source *w_destination;
>     struct connection *c_destination;
> };

I figure you need to keep a per-connection buffer to copy things over.

> 
> static int transfer_data_cb(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
>     struct connection *connection = (struct connection *) userdata;
> 
>     char *buffer = malloc(BUFFER_SIZE);

OOM check, please!

Also, we try to avoid doing function invocations in the variable
declaration part of functions. Initialisation to fixed values is fine,
but please don't mix complex code with variable declartions.

Also, I think we shouldn't do malloc() on every single data block we
get. It sounds like a better approach to keep the buffer in the
connection struct, simply as a uint8_t array in there...

>     ssize_t buffer_len;
> 
>     assert(revents & EPOLLIN);
>     assert(fd == connection->origin_fd);
> 
>     log_info("About to transfer up to %u bytes from %d to %d.", BUFFER_SIZE, connection->origin_fd, connection->destination_fd);
> 
>     buffer_len = recv(connection->origin_fd, buffer, BUFFER_SIZE, 0);
>
>     if (buffer_len == 0) {
>         log_info("Clean disconnection.");
>         sd_event_source_unref(connection->w_destination);
>         sd_event_source_unref(s);
>         close(connection->origin_fd);
>         close(connection->destination_fd);
>         free(connection->c_destination);
>         free(connection);

This looks as something to move to a function of its own.

>         goto finish;
>     }
>     else if (buffer_len == -1) {
>         log_error("Error %d in recv from fd=%d: %s", errno, connection->origin_fd, strerror(errno));
>         exit(EXIT_FAILURE);

If an error happens on a connection, you shouldn't just exit. Just log
something with log_warning and treat it otherwise like EOF: destroy the
connection struct, and that's it.

>     }
> 
>     if (send(connection->destination_fd, buffer, buffer_len, 0) < 0) {
>         log_error("Error %d in send to fd=%d: %s", errno, connection->destination_fd, strerror(errno));
>         exit(EXIT_FAILURE);
>     }

This will block IO until this is written. This is not ideal. Instead,
you should put the destination fd into non-blocking mode, try to write
as much as you can, and if there's something you cannot write, then you
should listen for EPOLLOUT and write it then. For that you need to store
the buffer in the connection struct.

> finish:
>     free(buffer);

_cleanup_free_ is your friend.

>     return 0;
> }
> 
> static int connected_to_server_cb(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
>     struct connection *c_server_to_client = (struct connection *) userdata;
>     struct sd_event *e = sd_event_get(s);
> 
>     log_info("Connected to server. Initializing watchers for sending data.");
> 
>     // Start listening for data sent by the client.
>     sd_event_add_io(e, c_server_to_client->destination_fd, EPOLLIN,
>     transfer_data_cb, c_server_to_client->c_destination,
>     &c_server_to_client->w_destination);

No C++ // comments, please. We use /* C comments */.

Also, please check return values.

> 
>     // Cancel the write watcher for the server.
>     sd_event_source_unref(s);
> 
>     // Start listening for data sent by the server.
>     sd_event_add_io(e, c_server_to_client->origin_fd, EPOLLIN, transfer_data_cb, c_server_to_client, &c_server_to_client->c_destination->w_destination);
> 
>     return 0;
> }
> 
> 
> static int set_nonblock(int fd) {
>     int flags;
>     flags = fcntl(fd, F_GETFL);
>     flags |= O_NONBLOCK;
>     return fcntl(fd, F_SETFL, flags);
> }

We have a function for this in util.c called fd_nonblock(). Also
SOCK_NONBLOCK is your friend.

> 
> static int get_server_connection_fd(const struct proxy *proxy) {
>     int server_fd;
>     int len;
> 
>     if (proxy->remote_is_inet) {
>         struct addrinfo hints;
>         struct addrinfo *result;
>         int s;
> 
>         memset(&hints, 0, sizeof(struct addrinfo));

Please initialize with = {} in the variable declaration instead. And
zero() is kinda cool too.

>         hints.ai_family = AF_UNSPEC; /* IPv4 or IPv6 */
>         hints.ai_socktype = SOCK_STREAM;  /* TCP */
>         hints.ai_flags = AI_PASSIVE; /* Any IP address */
> 
>         //log_error("Looking up address info for %s:%s", proxy->remote_host, proxy->remote_service);
>         s = getaddrinfo(proxy->remote_host, proxy->remote_service, &hints, &result);
>         if (s != 0) {
>             log_error("getaddrinfo error (%d): %s", s, gai_strerror(s));
>             exit(EXIT_FAILURE);
>         }

getaddrinfo is blocking and might involve network, unfortunately, so i
am not sure this is the right thing to invoke here. I wrote "libasyncns"
to make NSS calls asynchronous, which might be an option
here. "libasyncns" is also what glib uses internally.

> 
>         if (result == NULL) {
>             log_error("getaddrinfo: no result");
>             exit(EXIT_FAILURE);
>         }
> 
>         // @TODO: Try connecting to all results instead of just the first.
>         server_fd = socket(result->ai_family, result->ai_socktype, result->ai_protocol);
> 
>         if (-1 == set_nonblock(server_fd)) {
>             log_error("Error setting socket to non-blocking.");
>             exit(EXIT_FAILURE);
>         }
> 
>         if (!connect(server_fd, result->ai_addr, result->ai_addrlen))
>         {

should be an < error check.

>             log_error("Could not connect to socket: %s:%s", proxy->remote_host, proxy->remote_service);
>             freeaddrinfo(result);
>             exit(EXIT_FAILURE);

Hmm, instead of exiting from arbitrary places I'd really prefer
returning an error upwards. Note that sd_event will return negative
errno-style errors that the handlers return back all the way up.

>     if (argc != 3) {
>         fprintf(stderr, "usage: %s hostname service-or-port\n", argv[0]);
>         exit(1);
>     }

"return EXIT_FAILURE" might be nicer here...

We need to put limits on the number of connections, i.e. you need to
count how many you have open, and disconnect incoming connections
immediately if we hit some limit.

Lennart

-- 
Lennart Poettering - Red Hat, Inc.