IPv6 Patch

[Tomash Brechko]

On Mon, Jan 28, 2008 at 16:46:03 -0800, Brian Aker wrote:
> The attached patch does the following:
> 1) Adds support for binding with host names (not just ip addresses)
> 2) Makes memcached support IPv6 for TCP connections.

The patch is not 100% complete, getaddrinfo() returns the list of
addresses, and you have to bind to all of them on the server side (and
try to connect to all of them in turn on the client side).  This page
explains how:

  http://people.redhat.com/drepper/userapi-ipv6.html

and this gives some clues why:

  http://people.redhat.com/drepper/linux-rfc3484.html


Here are some more minor issues:


> @@ -2046,7 +2047,7 @@
>      bool stop = false;
>      int sfd, flags = 1;
>      socklen_t addrlen;
> -    struct sockaddr addr;
> +    struct sockaddr_storage addr;
>      int res;
>  
>      assert(c != NULL);
> @@ -2056,7 +2057,7 @@
>          switch(c->state) {
>          case conn_listening:
>              addrlen = sizeof(addr);
> -            if ((sfd = accept(c->sfd, &addr, &addrlen)) == -1) {
> +            if ((sfd = accept(c->sfd, (struct sockaddr *)&addr, &addrlen)) == -1) {

The code fills addr and addrlen, but never uses them if I'm correct,
so accept(c->sfd, NULL, NULL) would do better.


> +    /*
> +     * the memset call clears nonstandard fields in some impementations
> +     * that otherwise mess things up.
> +     */
> +    memset(&hints, 0, sizeof (hints));
> +    hints.ai_flags = AI_PASSIVE;
> +    if (is_udp)
> +    {
> +      hints.ai_protocol = IPPROTO_UDP;
> +      hints.ai_socktype = SOCK_DGRAM;
> +      hints.ai_family = AF_INET;
> +    }
> +    else
> +    {
> +      hints.ai_family = AF_UNSPEC;
> +      hints.ai_protocol = IPPROTO_TCP;
> +      hints.ai_socktype = SOCK_STREAM;
> +    }

It is recommended to set AI_ADDRCONFIG flag, and you shouldn't set
ai_family and ai_protocol, the whole getaddrinfo() mechanism is
positioned to be protocol independent.


> +    error= getaddrinfo(settings.inter, port_buf, &hints, &ai);
> +    if (error)
> +    {
> +      fprintf(stderr, "getaddrinfo(): %s\n", gai_strerror(error));
> +      return -1;
> +    }

It should be

       if (error != 0) {
           if (error != EAI_SYSTEM)
               fprintf(stderr, "getaddrinfo(): %s\n", gai_strerror(error));
           else
               perror("getaddrinfo()");
     
           return -1;
       }


> +    if (bind(sfd, ai->ai_addr, ai->ai_addrlen) == -1) {
>          perror("bind()");
>          close(sfd);
>          return -1;

When binding to several addresses, you will have to to handle
EADDRINUSE differently (even though SO_REUSEADDR is set, pointers
above explain why).


> @@ -2379,6 +2402,8 @@
>          close(sfd);
>          return -1;
>      }
> +    freeaddrinfo(ai);
> +
>      return sfd;
>  }

You have lots of premature returns where you better to do
freeaddrinfo() too, just for the case ;).



-- 
   Tomash Brechko