Update debugger network code

[MisterDA]

The OCaml debugger sets up a socket between the debugger and the runtime of the process being debugged. The user has some control over the socket: she can specify an IPv4 address or host, or a path to a Unix domain socket. They are selected with ocamldebug -s [address] option, or interactively with set socket [address], and with the CAML_DEBUG_SOCKET environment variable to the remotely debugged process.

With some warnings enabled, C compilers now complain that some network functions are deprecated (e.g., gethostbyname is replaced by getaddrinfo), because the functions don't support IPv6.

This PR:

• replaces uses of gethostbyname with getaddrinfo in the runtime and in ocamldebug;
• allows using IPv6 addresses, specified using the bracketed syntax [::1]:8080, and IPv6 hosts;
• allows using Unix domain socket on Windows;
• modernizes and cleans up the network code.

My code tries to detect whether a colon : separating an IP address and a port has been specified; if not it considers that the user has specified an Unix domain socket (a path in the filesystem). Then, if the user has specified an implicit path or something that doesn't look like an IPv6 address, my code also picks an Unix domain socket. All the remaining cases (IPv4 address, IPv6 address, hostname; followed by a port) are handled by getaddrinfo after some little processing.

I've tested this code on macOS and Windows, with IPv4, IPv6 addresses and hosts, and absolute and relative paths for Unix domain sockets.

[gasche]

Both [foo]bar and foo:bar are valid paths for files or directories on my Linux system, which means that some valid paths cannot be used for local sockets.

This was already an existing issue with the previous code (I think?) which used the "does it contain a :?" criterion.

I think that the following approaches would fix it, if we decide that it is worth fixing:

• have a convention to clarify that something really is a path name, for example starting with / or ./ (for absolute and relative paths)
• enrich the UI to specify the communication channel with an explicit type, for example --socket-path and --socket-addr as refinements of the -s option, etc.

[MisterDA]

Both [foo]bar and foo:bar are valid paths for files or directories on my Linux system

I probably wasn't clear enough in the PR description (and I discovered Filename.is_implicit just after submitting). On my macOS, [foo]bar is correctly identified as a file path. foo:bar is identified as an address, with foo being the host, and bar the protocol name associated to a port number that getaddrinfo should retrieve, if I'm not mistaken.

I think I see the problem this way: almost anything can be a path, but there are things that are clearly not pairs of (host, port), if they contain directory separators and square brackets. I think it's better to deal with the obvious cases, let getaddrinfo sort the rest; and the user can always specify a relative path ./ to get out of this mess. We should never identify a valid host:port as a path, that would be a bug. Then there's the problem of error reporting; if I wrote localhost:8080x I probably wasn't trying to create a domain socket.

have a convention to clarify that something really is a path name, for example starting with / or ./ (for absolute and relative paths)

Yes. Non-implicit paths are well recognized as such.

Return true if the file name is relative and does not start with an explicit reference to the current directory (./ or ../ in Unix), false if it starts with an explicit reference to the root directory or the current directory.

With this code, I consider that the address looks like a file path if:

• it doesn't contain :, or
• it is not implicit and it does not look like an IPv6 address (enclosed in square brackets).

Possible refinements, but a bit more annoying to implement, include checking whether the address contains a directory separator, and whether the segment after the last colon parses as an integer.

enrich the UI to specify the communication channel with an explicit type, for example --socket-path and --socket-addr as refinements of the -s option, etc.

That would be the clearest option, but what should we do about the -s option? And we would also need to give refinements to the CAML_DEBUG_SOCKET environment variable on the other side.

[MisterDA]

My main goal was to update the network code. I found out while hacking that I could add IPv6 and Unix domain sockets support too (on Windows). If this feature is too complicated to sort out, I could remove it (the last commit!) from this PR and re-submit later.

[ghost]

There is no guarantee that n - 1 is >= 0 here (and same with the n - 2 computation further down).

[MisterDA]

I'm now checking that there's a least 4 bytes in the address ([::]) before accessing it.

[gasche]

My general impression:

• this looks reasonable overall (my question about path vs. address confusion was a nitpick)
• it was not a wise choice to couple a refactoring/cleanup with new features ; next time I would recommend sending separate PRs
• nothing in the PR is critical correctness-wise, and I think it is beneficial to share knowledge of this codebase
• all this considered, I am in favor of merging the PR once someone is done looking at the details, and it looks like @dustanddreams is doing this work

[ghost]

* all this considered, I am in favor of merging the PR once someone is done looking at the details, and it looks like @dustanddreams is doing this work

But you have no proof that I am not @MisterDA's sock puppet!

[gasche]

I find the logic to select the socket type not clear enough. The code should be clearer. Maybe we also want a comment that explains what the logic is (it is not documented anywhere so far?), but this might not be necessary if the code is readable enough as its own specification.

[gasche]

I find the logic in this block of code too hard to follow. Before your change the idea was rather clear: either there is a : and we handle this as an IP (V4) address, or there isn't and we assume a unix socket path. Now what is the logic, and is it apparent in the code?

For example I think that it is not clear what not (Filename.is_implicit ...) is testing for, and maybe this could be helped with variable naming: let is_an_explicit_path = not (Filename.is_implicit address) in ....

[MisterDA]

Well a path like C:\TMP\my.sock or ./my:sock is obviously not a host:port. not (Filename.is_implicit ...) caches paths that are rooted or (syntactically) relative to the current directory (they begin with ./ or ../).
If we only look at whether the address contains a : to decide whether it's an Unix domain socket or an inet address, then we'll miss absolute paths on Windows.
I'll look for a simpler compromise.

[gasche]

I'm not saying that the previous logic was better -- I agree it was worse, especially on Windows. I am asking for a clear description of what the new logic is, and code that matches this clear description.

[MisterDA]

I find the logic to select the socket type not clear enough. The code should be clearer.

I hope to have made it simpler and clearer.

Maybe we also want a comment that explains what the logic is (it is not documented anywhere so far?), but this might not be necessary if the code is readable enough as its own specification.

I hope that the code is self-sufficient, although it does not justify the choices made. I now consider that the string is a Unix domain socket path if:

• if it does not contain a : (can't be a host:port pair);
• it starts with an explicit reference to a root directory or the current directory (it's an explicit path).
  I then let getaddrinfo sort out the rest after splitting host and port strings. I also try to have both the OCaml and C code behave identically.

Sorry for the back and forth, I tried to be too clever without achieving a satisfying result. Are the code and the explanations satisfying now? Do you think that the code covers reasonable use-cases and has sane failure conditions?

[gasche]

Yes, I think this is reasonable now. Thanks! (See minor comments.)