ocaml / ocaml

The core OCaml system: compilers, runtime system, base libraries
https://ocaml.org
Other
5.19k stars 1.06k forks source link

Update debugger network code #13114

Closed MisterDA closed 3 weeks ago

MisterDA commented 3 weeks ago

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:

~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 commented 3 weeks ago

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:

MisterDA commented 3 weeks ago

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:~

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 commented 3 weeks ago

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.

gasche commented 3 weeks ago

My general impression:

dustanddreams commented 3 weeks ago
* 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!

MisterDA commented 3 weeks ago

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:

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?