Re: [evolution-patches] ipv6 patch

[Jeffrey Stedfast <fejj ximian com>]

fair enough.

Jeff

On Tue, 2004-09-14 at 21:18, Not Zed wrote:
> On Tue, 2004-09-14 at 16:31 -0400, Jeffrey Stedfast wrote: 
> > a few things I've noticed:
> > 
> > - hints.ai_family is not always set. I realise that PF_UNSPEC is
> > probably 0 on linux and so this just works, but I think we should
> > explicitly set this.
> I'm pretty sure setting 0 for everything 'unset' is sufficient.  Thats
> why pf_unspec is zero.  But it could always be added. 
> > - is there ever a time when hints.ai_socktype won't be SOCK_STREAM in
> > camel? if not, what if we set this in camel_getaddrinfo() to save typing
> > in all the providers (and everywhere else this gets used)?
> > heck, same for PF_UNSPEC even...
> I'd rather keep the interface as close to the real thing as possible. 
> Just because we only use streams now doesn't mean we should break the
> interface. 
> > - while we don't always need AI_CANONNAME in hints.ai_flags, maybe we
> > could still just always set this in camel_getaddrinfo() just simply so
> > we can reduce the number of arguments needed for it? (and save typing in
> > the providers, etc)?
> As above.  When AI_CANONNAME is set it seems to me its going to do
> more work than if not, enough to want to not do if if the value isn't
> going to be used. 
> > the only reason I can think of against this is overhead of requesting
> > that bit of data...? but if that's the case, perhaps we can just use a
> > bool or something as to whether or not we care about it?
> I think the existing interface is good enough, why bastardise it with
> some busted camel-knockoff-looks-the-same-but-isn't thing?  I don't
> think having potentially multiple bool arguments and a changing
> interface if requirements change, is a better replacement.
> 
> I don't know what your concern is, the replacement code is maybe 2
> lines more than the old stuff, and it does more in the end. 
> > I'm just trying to reduce boilerplate for setting up the hints argument
> > all over the place if we don't really need to.
> But they're not all the same, so ...
> > - I think we also want hints.ai_flags to have the AI_ADDRCONFIG flag
> > set.
> I'm not sure that is necessary since we are using multiple addresses
> now.  It seems to me that it could lead to the same problems this
> whole patch is trying to fix in the first place? 
> > - I agree with your sentiment about simply dropping non-getaddrinfo()
> > implementation stuff. afaik, all the platforms we care about have it
> > (linux, solaris, etc).
> > 
> > other than above comments, patch looks good (I'd even agree to let patch
> > get committed as-is)
> > 
> > Jeff
> > 
> > On Mon, 2004-09-13 at 18:11 +0800, Not Zed wrote:
> > > 
> > > This rather big patch addresses a major issue with network connections
> > > from camel, that is, only 1 type of address is supported (ipv6 or
> > > ipv4) for a given host at a time, and only one physical address for
> > > that host will ever be tried, even if multiple addresses of the same
> > > format exist.  The only other way to address this issue would be to
> > > have all of the service connect calls perform two lookups using an
> > > extended gethostbyname2_r interface, which is only available on GNU
> > > anyway, where we already have getaddrinfo.  It would be a messier and
> > > even less portable solution.  What we have now though is 'really
> > > wrong'.
> > > 
> > > Most of the patch is just changing the api's to suit the changes and
> > > to clean up some of the related apis too.  And to remove a lot of ipv6
> > > vs ipv4 specific code from most of the places where it doesn't need to
> > > know about the difference (like, just about everywhere, even the stuff
> > > in stream-ssl could probably just use memcpy).
> > > 
> > > The main change is the new hostname resolution interfaces, which are
> > > async/cancellable like the old ones, and use a getaddrinfo/getnameinfo
> > > interface rather than a gethostby* interface.  So it emulates
> > > getaddrinfo when it isn't available, and forces ipv4 in that case.
> > > Sure it could also handle some ipv6 stuff, but i just don't care.
> > > 
> > > I'm inclined to simply drop support for all systems which don't
> > > support getaddrinfo/getnameinfo anyway, because its too much pain to
> > > deal with different address formats using the old-style interfaces.
> > > But for now the patch has a very limited emulation which mostly seems
> > > to work.
> > > 
> > > Some configure changes mean that ipv6 is simply enabled by default if
> > > your system has the necessary interfaces, and it always uses those
> > > interfaces if available even if you force it to use ipv4 (not that i
> > > can see there's actually a reason to want to do this).
> > > 
> > > I would bet my house on there being lots of portability issues with it
> > > all though.  e.g. redefining struct addrinfo on half/broken systems.
> > > 
> > > It hasn't been tested a lot, so there are probably other bugs too.
> > > 
> > > -- 
> > > 
> > > Michael Zucchi <notzed ximian com>
> > > "born to die, live to work, it's
> > > all downhill from here"
> > > Novell's Evolution and Free
> > > Software Developer
> -- 
> 
> Michael Zucchi <notzed ximian com>
> "born to die, live to work, it's
> all downhill from here"
> Novell's Evolution and Free
> Software Developer