rupprecht added a comment.
In D87333#2273506 <https://reviews.llvm.org/D87333#2273506>, @labath wrote:
> (Sorry about the delay.) Given the current requirements, I think this patch
> is fine (excellent even).
>
> That said, I'm not sure whether the original motivation for this requirement
> (avoiding dns lookups) is still relevant. These days, we communicate with the
> local debug server via `socketpair(2)` sockets (which wasn't the case back
> then), which does not require any dns lookups and is immune to a
> misconfigured hosts file.
The initial dns lookup may still fail, I think? e.g. my initial version of this
patch had:
if (llvm::Error error =
listen_socket.Listen("localhost:0", backlog).ToError())
If the hosts file has a bad value for localhost, that will create a bad socket.
That said, I don't know how to reproduce the original issue -- maybe it isn't
an issue for some other reason.
> Given that none of the things touched by this patch is extremely critical
> (`GDBRemoteCommunication::ConnectLocally` is used for reproducers and the
> rest is test code), maybe we could use this to test the water and see whether
> we can start using the network stack the way it's supposed to be used ?
Many non-reproducers tests fail w/o the change to
`GDBRemoteCommunication::ConnectLocally`, so it's at least used outside of that.
I can go either way with using `localhost` or using hard-coded localhost IPs.
Whichever way we go, I have a slight preference for keeping the tests and code
consistent, e.g. if we use hard-coded IPs in code linked into lldb to allow
people with a bad hosts file to use lldb, we should use hard-coded IPs in the
tests to allow those same users to develop on lldb.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87333/new/
https://reviews.llvm.org/D87333
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
