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. - 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... - 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)? 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'm just trying to reduce boilerplate for setting up the hints argument all over the place if we don't really need to. - I think we also want hints.ai_flags to have the AI_ADDRCONFIG flag set. - 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 -- Jeffrey Stedfast Evolution Hacker - Novell, Inc. fejj ximian com - www.novell.com
Attachment:
smime.p7s
Description: S/MIME cryptographic signature