IPv6 Patch

[Tomash Brechko]

On Tue, Jan 29, 2008 at 14:23:10 -0800, Brian Aker 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.

That's exactly the point.  The user have the control with the -l
option.  And if I specify some name, I expect it to be resolved to all
addresses (for one address I would simply specify it as is).  I doubt
Drepper's paper is wrong with this respect.  Also, with IPv6 a single
interface may have _lots_ of addresses, and you can't predict how the
client would resolve the same host name (to which address).  Hence, if
you want inter-operation between server and client each specifying the
same server name, you should do as the Drepper's post describes.

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

Hmm, this may be true.  But from my experience with other projects
(MySQL I/O layer ;)) it seems there happens to be some oddities that
some say would improve portability, yet no one can name the platforms
this is intended for.  So, what platform do you imply, or rather, does
the rest of memcached is portable to it?  My point is that the excess
use of such "portability constructs" brings the confusion to the code
while not necessary making it more portable.  And to my believe addr
filling is there because at some point it was used for client address
reporting, not because of some portability issues.  I would expect the
appropriate comment otherwise.

OTOH, this is not really essential, we may leave it as is.


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

AI_ADDRCONFIG is required to skip the addresses when they are reported
by DNS but the host doesn't bind them to any interface.  As for the
rest, I didn't mean you should combine UPD and TCP.  What I mean is
that there's no point to disable IPv6 for UDP as you did, and no point
to explicitly specify IPPROTO_* along with SOCK_*.


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

"SUCCESS" can't be ever reported, the code above runs perror() only
when getaddrinfo() returns EAI_SYSTEM.  gai_strerror(EAI_SYSTEM) will
return "System error", while perror() will be more specific.  Here's
what man page says:

       EAI_SYSTEM
              Other system error, check errno for details.

And the code above "checks" errno with perror().


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

Exactly because of this.  On other exit points the code is careful to
close the socket, so you should do your cleanup as well.


-- 
   Tomash Brechko