Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IPv6 support under Windows #5676

Closed
vicuna opened this Issue Jul 9, 2012 · 6 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

vicuna commented Jul 9, 2012

Original bug ID: 5676
Reporter: vouillon
Assigned to: @protz
Status: closed (set by @xavierleroy on 2015-12-11T18:08:09Z)
Resolution: fixed
Priority: normal
Severity: feature
Target version: 4.01.0+dev
Fixed in version: 4.01.0+dev
Category: platform support (windows, cross-compilation, etc)
Related to: #5766
Parent of: #5398

Bug description

This patch (against trunk) adds IPv6 support under Windows.

Windows XP is required for compiling OCaml with IPv6 support. Compiled programs should work with older versions of Windows as well.

There are a lot of include order changes, as winsock2.h needs to be included before winsock.h.

The getnameinfo and getaddrinfo bindings are taken from the unix/ directory.

string_of_inet_addr and inet_addr_of_string are implemented using getnameinfo and getaddrinfo, as functions inet_pton and inet_ntop are only available in very recent versions of Windows.

Finally, the check for PF_INET6 in file socket.c is disabled. (By the way, the comment seems incorrect, as OCaml is already linked against Winsock 2.)

File attachments

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 10, 2012

Comment author: @protz

Thanks! I'll give the patch a try tomorrow and commit on trunk barring any major problems.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 11, 2012

Comment author: @protz

Disclaimer: I'm not a Unix system programming guru, esp. under Windows, so please bear with me, but, I just gave this patch a try and it doesn't seem to work fully.

open Unix;;

let a = inet_addr_of_string "2001:6b0:5:1688::200";;

val a : Unix.inet_addr =

let a = ADDR_INET (a, 8001);;

val a : Unix.sockaddr = ADDR_INET (, 8001)

let ic, oc = open_connection a;;

Exception: Unix.Unix_error (EAFNOSUPPORT, "connect", "").

Is that expected?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 11, 2012

Comment author: vouillon

I missed this function... I have uploaded an updated patch.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 13, 2012

Comment author: @protz

So I gave your patch a try and everything seems to compile/work fine as far as my light testing could tell.

Here's a few remarks on the patch:

  • you sometimes use #if defined() vs. #ifdef, is there a reason?
  • you change a lot of includes; apparently, it seems that you want "unixsupport.h" to be now included before <windows.h>; is there a reason for that too?
  • you sometimes remove the "unixsupport.h" header completely from the includes list, e.g. in "startup.c" -- is that part of a general cleanup?
  • why did you add <alloc.h> to times.c?
  • there's an extra set_close_on_exec that appeared in open_connection; I'm very much afraid that this will be a surprise for some people...

Apart from that, the patch looks very good!

Thanks,

jonathan

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 13, 2012

Comment author: vouillon

  • you sometimes use #if defined() vs. #ifdef, is there a reason?

I'm only using "#if defined()" in win32unix/socket.c, and that's
just because I took the code from unix/socket.c...

  • you change a lot of includes; apparently, it seems that you want
    "unixsupport.h" to be now included before <windows.h>; is there a reason for
    that too?

"unixsupport.h" used to include <winsock.h>, and I've changed it
to include <winsock2.h> instead. As <winsock2.h> needs to be
included before <windows.h>, I had to change a lot of includes...

An alternative would have been to remove the inclusion of
<winsock.h> from "unixsupport.h" and instead include <winsock2.h>
only when necessary. This seems to involve more changes: there are
more files that involve sockets than files that include <windows.h>.

Another alternative would be to move the inclusion of <windows.h> to
"unixsupport.h". That might be cleaner in the long run (in particular,
we avoid the hack in "winworker.h" mentioned below), but is more
invasive: <windows.h> would be included a lot more often.

  • you sometimes remove the "unixsupport.h" header completely from
    the includes list, e.g. in "startup.c" -- is that part of a general
    cleanup?

In "winworker.h", <winsock2.h> needs to be included between
#define _WIN32_WINNT 0x0400
and
#include <windows.h>

So, I have moved the inclusion of "unixsupport.h" there and
removed it from the files that include "winworker.h".

  • why did you add <alloc.h> to times.c?

There were some missing includes, including this one. Indeed,
this is not IPv6 related...

Here, times.c uses function alloc_small which is declared in <alloc.h>.

  • there's an extra set_close_on_exec that appeared in open_connection;
    I'm very much afraid that this will be a surprise for some people...

A set_close_on_exec was added in unix/unix.ml in revision 6420
(Sat Jun 19 15:33:53 2004 UTC (8 years ago) by xleroy). Indeed,
file descriptors handled through in/out_channel are expected to be
'close_on_exec' by default.

It seems more coherent to have it as well under Windows.

Do you want separate patches for the additional includes (such as
<alloc.h> in times.c) and for the extra set_close_on_exec, or is that
good enough?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 13, 2012

Comment author: @protz

Thanks for the extensive explanations and the bonus cleanups. Everything seems just fine so I just commited this on trunk as r12710

Cheers,

jonathan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.