Created attachment 52774 [details] [review]
Use getaddrinfo() also when SUPPORT_IP6 is *NOT* defined

I dislike the fact that the patch changes the working code on the other
OSes. At this point I would rather get a patch that is limited to 
that version of windows. I don't know how to check for just that platform
though.
  getaddrinfo seems to be POSIX  1003.1-2003, i.e. recent
while gethostbyname is BSD 4.3 and has been around forever. I don't want
to raise portability issue just because one version of Windows 98 has a
bug in its implementation of gethostbyname().

Daniel 

Hi Daniel,

thanks for your fast reply. About the portability issues:

I've tested getaddrinfo() on all Windows platforms starting from Windows 95 (95,
98FE, 98SE, ME, 2000, XP), and I can run specific tests on those platforms if
you want me to. Additionaly, Mircrosoft urges developers to use getaddrinfo()
instead of gethostbyname(), as stated here:

http://msdn.microsoft.com/library/en-us/winsock/winsock/gethostbyname_2.asp

From my experience with such recomondations I would say, on Windows platforms
you will be much better off switching to the new function, because Microsoft
doesn't care anymore about problems with the old function.

What about using getaddrinfo() only on Windows platforms (or when SUPPORT_IP6 is
defined, like before)?

> What about using getaddrinfo() only on Windows platforms (or when SUPPORT_IP6 is
> defined, like before)?

  Sounds fine, I think the __WIN__ (or __WIN32__) macro can be used as tests
then for the patch like in other parts of the libxml2 code.

Daniel

Created attachment 52849 [details] [review]
Use only getaddrinfo() when HAVE_GETADDRINFO and _WIN32 are defined

Hi Daniel,

here comes a new patch. Non-Windows-platforms still use exactly (*) the same
logic to decide whether to use gethostbyname() or getaddrinfo(), but if _WIN32
*and* HAVE_GETADDRINFO are defined, only getaddrinfo() will be used. Sadly, this
has required more #ifdef's than my first patch.

I've kept the dependency on HAVE_GETADDRINFO to make reverting to the old state
on Windows easy by removing the define from wsockcompat.h, for example if this
should be made a configure option.

(*) One thing my patch changes for Non-Windows-platforms:

If SUPPORT_IP6 is defined, but HAVE_GETADDRINFO is *not*, the following
(simplified) code was produced:

if (have_ip6 ()) {
  h = gethostbyname (...);
  for (... all entries in h ...) {
    if (h->h_addrtype == AF_INET)
      // try to connect
    else if(have_ip6 () && (h->h_addrtype == AF_INET6))
      // try to connect
  }
}

I think the outer if() clause is useless, because availability of AF_INET6
sockets is tested again at the point where the address family is checked. The
outer if() effectively breaks the entire function if libxml2 is compiled whith
SUPPORT_IP6 but is executed on a system without support for AF_INET6 sockets.

So I've assumed this was not intended and removed the outer if() in this special
case.

This isnt going to work unless building with VC7 or above as getaddrinfo is not
defined in ws2tcpip.h for VC6 (which also no longer supported is what many of us
out there still use - and new Platform SDKs no longer will work with it). Looks
like some other projects out there have custom addrinfo.h headers for this, but
unless this is something that's going to be applied for other OSes, I dont think
it's worth it.

Oh, I didn't knew that VC6 does not have getaddrinfo().

What about an option in win32\configure.js to explicitly turn on HAVE_GETADDRINFO? 

That would be much better solution default disabled of course - (no idea about
mingw and borland addrinfo support).

Created attachment 53364 [details] [review]
Use getaddrinfo() only when compiling for Windows using MSVC with recent Platform SDK

I've asked Igor Zlatkovic about adding an option to win32\configure.js. He
dislikes this solution because he doesn't want to bloat configure.js with
options which are not relevant for all builds.

But I've thought it would be even smarter to auto-detect whether the Platform
SDK headers provide getaddrinfo():

#include <ws2tcpip.h>
#if defined(_MSC_VER) && defined(AI_PASSIVE) && \
    defined(AI_CANONNAME) && defined(AI_NUMERICHOST)
#define HAVE_GETADDRINFO
#endif

This solution only uses getaddrinfo() if the associated parameter flags are
defined (which indicates we are using a recent Platform SDK) and the Microsoft
C Compiler is used. I've tested this with VC6 and VC7.

What do you think about this solution?

Unless Rob objects it seems you should have nailed it down, though
I'm a bit surprized by how you try to detect this is a "recent" version ...

Daniel

Hi Daniel,

great! 

About detecting the "recent" version:

the "AI_*" defines are used to assemble a bitmask which controls behaviour of
getaddrinfo(). They are described in RFC2553 together with getaddrinfo() and its
associated parameter structures. 

I have assumed they will be defined always together with a prototype of
getaddrinfo() and associated parameter structures, otherwise getaddrinfo() would
be useless. The other way arround, they will never occur without getaddrinfo(),
because they are used only by this function.

Additionally, I have grep'ed trough recent and earlier versions of Windows
Platform SDK headers, to see if my assumptions where correct.

And to be completely on the safe side, I also check if we are compiling using
MSVC, because Bob said he has "no idea about mingw and borland support". (Maybe
this check could be ommited to let users of other compilers benefit from this
patch?)

Patch looks and works fine for me now. Checked the headers a bit more and it
looks like detection might be able to be simplified using:
#if defined(GetAddrInfo)

Hi Rob,

First, I had the same idea, because 
-> http://msdn.microsoft.com/library/en-us/winsock/winsock/getaddrinfo_2.asp
wrote: "Macros in the Winsock header file define the mixed-case function name
GetAddrInfo to getaddrinfo; either spelling is acceptable." 

But I can't find "GetAddrInfo" anywhere in my SDK headers. Also the following
test code
 #include <winsock2.h>
 #include <ws2tcpip.h>
 #if defined(GetAddrInfo)
 #error GetAddrInfo is defined!
 #endif
does compile without error for me. 

I'm wondering about this, maybe Microsoft has introduced the mixed-case variant
in a very recent fixpack of VC7 which I haven't applied yet. Therefor I came up
with my test of "AI_*" flags.

Just out of curiosity, can you tell me in which header you have found "GetAddrInfo"?

in ws2tcpip.h - defined as GetAddrInfoW for unicode and GetAddrInfoA otherwise

I compared my headers with installations on the systems of my colleagues, they
have GetAddrInfo in ws2tcpip.h, I don't have. I have VC7.0.x, they have 7.1.x.

Personally, I have no problem with "#if defined(GetAddrInfo)", I will upgrade my
system anyway. But testing for "AI_*" may help users of VC7.0.x to benefit from
this patch.

I think you should decide what you prefer, I have no strong preference.

Fixed in CVS. Used GetAddrInfo as i'ts simpler and VC7.0.x can always use newer
Platform SDK to support this.

Great, thanks for your support.

Am I supposed to do anything further with this bug, for example testing and then
changing to CLOSED, or what is the usual way to deal with this?

I usually CLOSE bug which are FIXED when doing a new release.
However if you could try the CVS version (or the CVS snapshot)
to confirm that would be great :-)

Daniel

I've tested a snapshot from 2005-10-14 and it works fine.