mgorny marked 4 inline comments as done.
mgorny added inline comments.
================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:583-610
+ConnectionStatus ConnectionFileDescriptor::UnixAbstractSocketAccept(
+ llvm::StringRef socket_name,
+ std::function<void(llvm::StringRef local_socket_id)> socket_id_callback,
+ Status *error_ptr) {
+ Status error;
+ std::unique_ptr<Socket> listen_socket = Socket::Create(
+ Socket::ProtocolUnixAbstract, m_child_processes_inherit, error);
----------------
labath wrote:
> This is identical to `NamedSocketAccept` modulo the socket type constant,
> right? Can you merge these?
Yes; in fact, most of the functions are pretty similar. However, I'd prefer to
defer merging them until after the port predicate removal patch, as that should
also open the possibility of combining them with the TCP socket methods too.
================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:647
+ uint16_t port = listening_socket.get()->GetLocalPortNumber();
+ socket_id_callback(port != 0 ? std::to_string(port) : "");
----------------
labath wrote:
> Can this ever be zero ?
Yes, when `getsockname()` fails. I suppose we could modify it to use
`llvm::Optional<...>` or even `llvm::Expected<...>` but this is one place where
I don't think we need to care much about proper error handling.
================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:898
+ // expect the remainder to be the port.
+ if (host_port[0] == ':')
+ host_port = std::string("localhost") + host_port;
----------------
labath wrote:
> Can this string be empty? Maybe use `url_arg.startswith()`, just in case.
I don't think it can, or at least I wasn't able to get an empty string in (the
argument parser seems to reject them) but sure, no harm in being extra secure.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111964/new/
https://reviews.llvm.org/D111964
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
