[Lldb-commits] [PATCH] D111321: [lldb] [ConnectionFileDescriptorPosix] Refactor scheme matching
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4b46a4134385: [lldb] [ConnectionFileDescriptorPosix]
Refactor scheme matching (authored by mgorny).
Herald added a project: LLDB.
Changed prior to commit:
https://reviews.llvm.org/D111321?vs=377948=378148#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111321/new/
https://reviews.llvm.org/D111321
Files:
lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
Index: lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
===
--- lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
+++ lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
@@ -120,9 +120,9 @@
m_device_id = std::string(host);
m_socket_namespace.reset();
- if (scheme == ConnectionFileDescriptor::UNIX_CONNECT_SCHEME)
+ if (scheme == "unix-connect")
m_socket_namespace = AdbClient::UnixSocketNamespaceFileSystem;
- else if (scheme == ConnectionFileDescriptor::UNIX_ABSTRACT_CONNECT_SCHEME)
+ else if (scheme == "unix-abstract-connect")
m_socket_namespace = AdbClient::UnixSocketNamespaceAbstract;
std::string connect_url;
Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -50,27 +50,6 @@
using namespace lldb;
using namespace lldb_private;
-const char *ConnectionFileDescriptor::LISTEN_SCHEME = "listen";
-const char *ConnectionFileDescriptor::ACCEPT_SCHEME = "accept";
-const char *ConnectionFileDescriptor::UNIX_ACCEPT_SCHEME = "unix-accept";
-const char *ConnectionFileDescriptor::CONNECT_SCHEME = "connect";
-const char *ConnectionFileDescriptor::TCP_CONNECT_SCHEME = "tcp-connect";
-const char *ConnectionFileDescriptor::UDP_SCHEME = "udp";
-const char *ConnectionFileDescriptor::UNIX_CONNECT_SCHEME = "unix-connect";
-const char *ConnectionFileDescriptor::UNIX_ABSTRACT_CONNECT_SCHEME =
-"unix-abstract-connect";
-const char *ConnectionFileDescriptor::FD_SCHEME = "fd";
-const char *ConnectionFileDescriptor::FILE_SCHEME = "file";
-
-static llvm::Optional GetURLAddress(llvm::StringRef url,
- llvm::StringRef scheme) {
- if (!url.consume_front(scheme))
-return llvm::None;
- if (!url.consume_front("://"))
-return llvm::None;
- return url;
-}
-
ConnectionFileDescriptor::ConnectionFileDescriptor(bool child_processes_inherit)
: Connection(), m_pipe(), m_mutex(), m_shutting_down(false),
@@ -154,133 +133,41 @@
OpenCommandPipe();
- if (!path.empty()) {
-llvm::Optional addr;
-if ((addr = GetURLAddress(path, LISTEN_SCHEME))) {
- // listen://HOST:PORT
- return SocketListenAndAccept(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, ACCEPT_SCHEME))) {
- // unix://SOCKNAME
- return NamedSocketAccept(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UNIX_ACCEPT_SCHEME))) {
- // unix://SOCKNAME
- return NamedSocketAccept(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, CONNECT_SCHEME))) {
- return ConnectTCP(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, TCP_CONNECT_SCHEME))) {
- return ConnectTCP(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UDP_SCHEME))) {
- return ConnectUDP(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UNIX_CONNECT_SCHEME))) {
- // unix-connect://SOCKNAME
- return NamedSocketConnect(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UNIX_ABSTRACT_CONNECT_SCHEME))) {
- // unix-abstract-connect://SOCKNAME
- return UnixAbstractSocketConnect(*addr, error_ptr);
-}
-#if LLDB_ENABLE_POSIX
-else if ((addr = GetURLAddress(path, FD_SCHEME))) {
- // Just passing a native file descriptor within this current process that
- // is already opened (possibly from a service or other source).
- int fd = -1;
-
- if (!addr->getAsInteger(0, fd)) {
-// We have what looks to be a valid file descriptor, but we should make
-// sure it is. We currently are doing this by trying to get the flags
-// from the file descriptor and making sure it isn't a bad fd.
-errno = 0;
-int flags = ::fcntl(fd, F_GETFL, 0);
-if (flags == -1 || errno == EBADF) {
- if (error_ptr)
-error_ptr->SetErrorStringWithFormat("stale file descriptor: %s",
-path.str().c_str());
- m_read_sp.reset();
-
[Lldb-commits] [PATCH] D111321: [lldb] [ConnectionFileDescriptorPosix] Refactor scheme matching
labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.
thanks
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:142-143
+ auto method =
+ llvm::StringSwitch>(scheme)
+ .Case("listen", ::SocketListenAndAccept)
Maybe just drop std::function?
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:164-171
if (error_ptr)
error_ptr->SetErrorStringWithFormat("unsupported connection URL: '%s'",
path.str().c_str());
return eConnectionStatusError;
}
if (error_ptr)
error_ptr->SetErrorString("invalid connect arguments");
Could you also please convert this to early returns, while you're inside. I
spent 10 minutes looking at this, and I'm still not sure whether the error
strings are correctly related to the individual checks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111321/new/
https://reviews.llvm.org/D111321
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D111321: [lldb] [ConnectionFileDescriptorPosix] Refactor scheme matching
mgorny updated this revision to Diff 377948.
mgorny added a comment.
Rebase to match the original termios code without other of my patches.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111321/new/
https://reviews.llvm.org/D111321
Files:
lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
Index: lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
===
--- lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
+++ lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
@@ -120,9 +120,9 @@
m_device_id = std::string(host);
m_socket_namespace.reset();
- if (scheme == ConnectionFileDescriptor::UNIX_CONNECT_SCHEME)
+ if (scheme == "unix-connect")
m_socket_namespace = AdbClient::UnixSocketNamespaceFileSystem;
- else if (scheme == ConnectionFileDescriptor::UNIX_ABSTRACT_CONNECT_SCHEME)
+ else if (scheme == "unix-abstract-connect")
m_socket_namespace = AdbClient::UnixSocketNamespaceAbstract;
std::string connect_url;
Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -50,27 +50,6 @@
using namespace lldb;
using namespace lldb_private;
-const char *ConnectionFileDescriptor::LISTEN_SCHEME = "listen";
-const char *ConnectionFileDescriptor::ACCEPT_SCHEME = "accept";
-const char *ConnectionFileDescriptor::UNIX_ACCEPT_SCHEME = "unix-accept";
-const char *ConnectionFileDescriptor::CONNECT_SCHEME = "connect";
-const char *ConnectionFileDescriptor::TCP_CONNECT_SCHEME = "tcp-connect";
-const char *ConnectionFileDescriptor::UDP_SCHEME = "udp";
-const char *ConnectionFileDescriptor::UNIX_CONNECT_SCHEME = "unix-connect";
-const char *ConnectionFileDescriptor::UNIX_ABSTRACT_CONNECT_SCHEME =
-"unix-abstract-connect";
-const char *ConnectionFileDescriptor::FD_SCHEME = "fd";
-const char *ConnectionFileDescriptor::FILE_SCHEME = "file";
-
-static llvm::Optional GetURLAddress(llvm::StringRef url,
- llvm::StringRef scheme) {
- if (!url.consume_front(scheme))
-return llvm::None;
- if (!url.consume_front("://"))
-return llvm::None;
- return url;
-}
-
ConnectionFileDescriptor::ConnectionFileDescriptor(bool child_processes_inherit)
: Connection(), m_pipe(), m_mutex(), m_shutting_down(false),
@@ -155,125 +134,33 @@
OpenCommandPipe();
if (!path.empty()) {
-llvm::Optional addr;
-if ((addr = GetURLAddress(path, LISTEN_SCHEME))) {
- // listen://HOST:PORT
- return SocketListenAndAccept(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, ACCEPT_SCHEME))) {
- // unix://SOCKNAME
- return NamedSocketAccept(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UNIX_ACCEPT_SCHEME))) {
- // unix://SOCKNAME
- return NamedSocketAccept(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, CONNECT_SCHEME))) {
- return ConnectTCP(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, TCP_CONNECT_SCHEME))) {
- return ConnectTCP(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UDP_SCHEME))) {
- return ConnectUDP(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UNIX_CONNECT_SCHEME))) {
- // unix-connect://SOCKNAME
- return NamedSocketConnect(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UNIX_ABSTRACT_CONNECT_SCHEME))) {
- // unix-abstract-connect://SOCKNAME
- return UnixAbstractSocketConnect(*addr, error_ptr);
-}
+llvm::StringRef scheme;
+std::tie(scheme, path) = path.split("://");
+
+if (!path.empty()) {
+ auto method =
+ llvm::StringSwitch>(scheme)
+ .Case("listen", ::SocketListenAndAccept)
+ .Cases("accept", "unix-accept",
+ ::NamedSocketAccept)
+ .Cases("connect", "tcp-connect",
+ ::ConnectTCP)
+ .Case("udp", ::ConnectUDP)
+ .Case("unix-connect",
+::NamedSocketConnect)
+ .Case("unix-abstract-connect",
+::UnixAbstractSocketConnect)
#if LLDB_ENABLE_POSIX
-else if ((addr = GetURLAddress(path, FD_SCHEME))) {
- // Just passing a native file descriptor within this current process that
- // is already opened (possibly from a service or other source).
- int fd = -1;
-
- if (!addr->getAsInteger(0, fd)) {
-// We have what looks to be a valid file descriptor, but we should make
-// sure it is. We currently are doing this by trying to get the flags
-// from the file descriptor and making sure it
[Lldb-commits] [PATCH] D111321: [lldb] [ConnectionFileDescriptorPosix] Refactor scheme matching
mgorny created this revision.
mgorny added reviewers: labath, teemperor, krytarowski, emaste.
mgorny requested review of this revision.
Move the POSIX-specific fd:// and file:// scheme handling into
separate methods. Replace the custom GetURLAddress() matching with
splitting into scheme and path, and matching scheme via
llvm::StringSwitch.
https://reviews.llvm.org/D111321
Files:
lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h
lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
Index: lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
===
--- lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
+++ lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp
@@ -120,9 +120,9 @@
m_device_id = std::string(host);
m_socket_namespace.reset();
- if (scheme == ConnectionFileDescriptor::UNIX_CONNECT_SCHEME)
+ if (scheme == "unix-connect")
m_socket_namespace = AdbClient::UnixSocketNamespaceFileSystem;
- else if (scheme == ConnectionFileDescriptor::UNIX_ABSTRACT_CONNECT_SCHEME)
+ else if (scheme == "unix-abstract-connect")
m_socket_namespace = AdbClient::UnixSocketNamespaceAbstract;
std::string connect_url;
Index: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
===
--- lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -50,27 +50,6 @@
using namespace lldb;
using namespace lldb_private;
-const char *ConnectionFileDescriptor::LISTEN_SCHEME = "listen";
-const char *ConnectionFileDescriptor::ACCEPT_SCHEME = "accept";
-const char *ConnectionFileDescriptor::UNIX_ACCEPT_SCHEME = "unix-accept";
-const char *ConnectionFileDescriptor::CONNECT_SCHEME = "connect";
-const char *ConnectionFileDescriptor::TCP_CONNECT_SCHEME = "tcp-connect";
-const char *ConnectionFileDescriptor::UDP_SCHEME = "udp";
-const char *ConnectionFileDescriptor::UNIX_CONNECT_SCHEME = "unix-connect";
-const char *ConnectionFileDescriptor::UNIX_ABSTRACT_CONNECT_SCHEME =
-"unix-abstract-connect";
-const char *ConnectionFileDescriptor::FD_SCHEME = "fd";
-const char *ConnectionFileDescriptor::FILE_SCHEME = "file";
-
-static llvm::Optional GetURLAddress(llvm::StringRef url,
- llvm::StringRef scheme) {
- if (!url.consume_front(scheme))
-return llvm::None;
- if (!url.consume_front("://"))
-return llvm::None;
- return url;
-}
-
ConnectionFileDescriptor::ConnectionFileDescriptor(bool child_processes_inherit)
: Connection(), m_pipe(), m_mutex(), m_shutting_down(false),
@@ -155,113 +134,33 @@
OpenCommandPipe();
if (!path.empty()) {
-llvm::Optional addr;
-if ((addr = GetURLAddress(path, LISTEN_SCHEME))) {
- // listen://HOST:PORT
- return SocketListenAndAccept(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, ACCEPT_SCHEME))) {
- // unix://SOCKNAME
- return NamedSocketAccept(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UNIX_ACCEPT_SCHEME))) {
- // unix://SOCKNAME
- return NamedSocketAccept(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, CONNECT_SCHEME))) {
- return ConnectTCP(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, TCP_CONNECT_SCHEME))) {
- return ConnectTCP(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UDP_SCHEME))) {
- return ConnectUDP(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UNIX_CONNECT_SCHEME))) {
- // unix-connect://SOCKNAME
- return NamedSocketConnect(*addr, error_ptr);
-} else if ((addr = GetURLAddress(path, UNIX_ABSTRACT_CONNECT_SCHEME))) {
- // unix-abstract-connect://SOCKNAME
- return UnixAbstractSocketConnect(*addr, error_ptr);
-}
+llvm::StringRef scheme;
+std::tie(scheme, path) = path.split("://");
+
+if (!path.empty()) {
+ auto method =
+ llvm::StringSwitch>(scheme)
+ .Case("listen", ::SocketListenAndAccept)
+ .Cases("accept", "unix-accept",
+ ::NamedSocketAccept)
+ .Cases("connect", "tcp-connect",
+ ::ConnectTCP)
+ .Case("udp", ::ConnectUDP)
+ .Case("unix-connect",
+::NamedSocketConnect)
+ .Case("unix-abstract-connect",
+::UnixAbstractSocketConnect)
#if LLDB_ENABLE_POSIX
-else if ((addr = GetURLAddress(path, FD_SCHEME))) {
- // Just passing a native file descriptor within this current process that
- // is already opened (possibly from a service or other source).
- int fd = -1;
-
- if (!addr->getAsInteger(0, fd)) {
-// We have what looks to be a valid file descriptor, but we should make
-
