IPv6 Patch

[Brian Aker]

Hi!

Thanks for look at the code, comments inline:

On Jan 28, 2008, at 11:54 PM, Tomash Brechko wrote:

> On Mon, Jan 28, 2008 at 16:46:03 -0800, Brian Aker wrote:
>> 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

Right, which is a bad idea most of the time since user's don't expect  
this behavior. Better to error and let them pick the proper interface.  
Binding to multiple interfaces will allow a sloppy DNS entry to expose  
a server to the outside of a firewall.


>> +            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

It is not portable to pass NULL to all OS'es accept(). You are right  
though that the structure is unused, but there is no portable way  
around it (the previous author of the loop thought so as well).

> 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.

This would work if the memcached's stack understood UDP and TCP at the  
same time (instead of the current case). While someone was at it they  
could refactor the server to handle all three internal protocols at  
the same time. I am interested in looking at that in the future.

>> +    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;
>       }

All errors returned by getaddrinfo() are handled by gai_strerror().  
Running perror in the way you suggest above will get you a haphazard  
message of "SUCCESS" from perror(). Just let the error fall to  
gai_strerror().

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


That is how the program is structured. Look at how the other exit  
points are done. I agree that the error logic could be cleaned up in  
general, but right now memcached just bails a lot.

Cheers,
	-Brian

--
_______________________________________________________
Brian "Krow" Aker, brian at tangent.org
Seattle, Washington
http://krow.net/                     <-- Me
http://tangent.org/                <-- Software
http://exploitseattle.com/    <-- Fun
_______________________________________________________
You can't grep a dead tree.