Re: [evolution-patches] ipv6 patch

[Not Zed <notzed ximian com>]

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