Multi-Interface Patch

Tomash Brechko tomash.brechko at gmail.com
Sun Feb 3 14:01:09 UTC 2008


On Sat, Feb 02, 2008 at 12:59:00 -0800, Brian Aker wrote:
> This is a multi-interface patch. It allows memcached to bind to  
> multiple interfaces (and per Tomash wish'es you can do this via DNS  
> entries... buyer beware).

Nothing to beware of.  I'm glad you are implementing this.


> @@ -64,7 +64,7 @@ std *
>   */
>  static void drive_machine(conn *c);
>  static int new_socket(struct addrinfo *ai);
> -static int server_socket(const int port, const bool is_udp);
> +static int *server_socket(const int port, const bool is_udp);

The original interface was designed to return one fd.  While you may
extend it to return an array of those, the code might become cleaner
if you redesign the whole thing instead.  It seems that the array of
fds isn't really needed: fds are created, then passed to libevent.
The only place where you use the array is mysterious pre_gdb().  There
they are closed, in a lame way, and then pre_gdb() does kill(getpid(),
SIGABRT).  Perhaps this function may be removed, because

  - it is not used now (see memcached.h).

  - it was added to allow "hot" debugging there unfreezing clients is
    important.  However for unfreezing it's possible to close the
    sockets from inside GDB with some script (I guess libevent has the
    list of them anyway).

In other words, it could be a single function that iterates over all
addresses, creates sockets, passes them to libevent, and forgets about
them.  Unless you would decide to do the complete cleanup, as
described later.

OTOH, as the person doing the whole work, you are free to choose any
way ;).


> @@ -2377,39 +2386,50 @@ static int server_socket(const int port,
> +      return NULL;
> +    }
> +
> +    for (count= 1, next= ai; next->ai_next; next= next->ai_next, count++);
> +
> +    sfd_list= (int *)malloc(sizeof(int) * MIN_NUMBER_OF_SOCKETS * count);
> +    memset(sfd_list, 0, sizeof(int) * MIN_NUMBER_OF_SOCKETS * count);

These two lines are calloc(), but you either don't have to initialize
the array at all, or set it to -1, as zero is a valid file descriptor.
Zero is a valid file descriptor.  In a daemon mode memcached should
detach from the console, thus zero may be reused for the very first
server socket.

Unfortunately the rest is also incorrect.  Why do you multiply by
MIN_NUMBER_OF_SOCKETS?  What's the meaning of this constant (== 2)?
You have count addresses, you'll get count fds.  Remember that
server_socket() is called for TCP and UDP separately.

If you would completely get rid of server_socket() function, but
decide to keep fds array, you will be able to have a single array for
all server sockets.  After all, in a general case there's a number of
TCP sockets, a possibly _different_ number of UDP sockets, and a
single Unix socket (no point to have several of them).  you could
store them all in a single array.  Or get rid of the fds array at all.


> +    for (sfd_ptr= sfd_list, next= ai; next->ai_next; next= next->ai_next, sfd_ptr++) {
> +        if ((sfd = new_socket(next)) == -1) {
> +            free(sfd_list);
> +            freeaddrinfo(ai);
> +            return NULL;
> +        }

It was me who told you to do the cleanup once it was already there,
and that was the point: it was there already.  But for multiple
addresses you also have to close all previously created sockets.  This
is where you would use the single socket array.  However, wouldn't it
be simpler not to do any cleanup?  memcached would exit soon anyway.


> +        if (bind(sfd, next->ai_addr, next->ai_addrlen) == -1) {
> +            perror("bind()");
> +            close(sfd);
> +            freeaddrinfo(ai);
> +            free(sfd_list);
> +            return NULL;
> +        }

Drepper explains that at least EADDRINUSE should be ignored.  But I
think you may ignore _any_ bind error, just skip over such sockets.
If even with AI_ADDRCONFIG the system returns the address that you
can't bind to, there may be some reasons for that (or it may be a
system bug).  Continuing would also solve that issue on OS X 10.5, as
eventually you'd come to IPv4 address down the list for UDP.


> +        *sfd_ptr= sfd;
>      }

When you would be skipping some sockets don't forget to increment
fds_ptr only after something has been assigned, not on every loop
iteration.


> +    sfd_list= (int *)malloc(sizeof(int) * 2);
> +    memset(sfd_list, 0, sizeof(int) * 2);

Same mystery as above.  Why *2?


>  /* invoke right before gdb is called, on assert */
>  void pre_gdb(void) {

This function may be removed.


> +    int *l_socket_ptr;
> +    for (l_socket_ptr= l_socket; *l_socket_ptr; l_socket_ptr++) {

Zero is a valid file descriptor in *l_socket_ptr test.


> -    if (u_socket > -1) {
> +    if (u_socket) {
>          for (c = 0; c < settings.num_threads; c++) {
>              /* this is guaranteed to hit all threads because we round-robin */
> -            dispatch_conn_new(u_socket, conn_read, EV_READ | EV_PERSIST,
> +            dispatch_conn_new(*u_socket, conn_read, EV_READ | EV_PERSIST,
>                                UDP_READ_BUFFER_SIZE, 1);

u_socket hols the array returned by server_socket().  Thus
semantically it's better to write not *u_socket, but u_socket[0].
Because this will quickly bring us to the question: why only the first?


> +/* Minimum number of sockets to allocate for listeners */
> +#define MIN_NUMBER_OF_SOCKETS 2

The comment is wrong.  Instead, it's some kind of coefficient you
multiply the number of sockets by.  I can't figure the purpose of it.


Regards,

-- 
   Tomash Brechko


More information about the memcached mailing list