Support remote-android with multiple connected devices.

[chaoren]

Summary

This change introduces a new URL scheme for platform connect:

adb://device-id:port

[ovyalov]

Please see my comments.

source/Plugins/Platform/Android/PlatformAndroid.cpp

189

Please check url is not null.

191

Could you add URL to error message?

193

m_device_id is already cleared at line 177.

200

Please remove this comment if it's no longer relevant.

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

95

Add "adb" constant?

98

Could you make such args transformation within PlatformAndroid::ConnectRemote to make PlatformLinux::ConnectRemote accept only "connect://" protocol?

98

Could you just update first args item in order to preserve all other args values?

[chaoren]

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

95

If I make the change in your comment below, this would only appear in PlatformAndroid::ConnectRemote, would it still be necessary?

98

Could you make such args transformation within PlatformAndroid::ConnectRemote to make PlatformLinux::ConnectRemote accept only "connect://" protocol?

But then PlatformAndroidRemoteGDBServer::ConnectRemote won't be able to see the adb scheme.

Or are we forgoing the connect scheme for Android altogether? There won't be a way to "just connect to that one device."

[ovyalov]

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

98

As an alternative we may use "connect://localhost:port" scheme for a single connected device or "connect://localhost:port/device-id" in order to connect to a particular connected device.

[chaoren]

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

98

:/ I know we're probably going to be the only people using this, but it just doesn't feel right to abuse the URI scheme like this.

Perhaps it would be more correct if we added a case in ConnectionFileDescriptor::Connect to treat adb://device-id:port as connect://localhost:port? Then it wouldn't even be necessary to rewrite the args.

[chaoren]

Update as per discussion.

[chaoren]

Accidentally included the other CL.

[ovyalov]

lgtm

[clayborg]

Looks good.

source/Plugins/Platform/Android/AdbClient.h

 namespace platform_android {
 
 class AdbClient
 {
 public:
     using DeviceIDList = std::list<std::string>;
 
     static Error
-    CreateByDeviceID (const char* device_id, AdbClient &adb);
+    CreateByDeviceID(const std::string &device_id, AdbClient &adb);
 
     AdbClient () = default;
     explicit AdbClient (const std::string &device_id);
 
     const std::string&
     GetDeviceID() const;
 
     Error

source/Plugins/Platform/Android/AdbClient.cpp

 
 const uint32_t kConnTimeout = 10000; // 10 ms
 const char * kOKAY = "OKAY";
 const char * kFAIL = "FAIL";
 
 }  // namespace
 
 Error
-AdbClient::CreateByDeviceID (const char* device_id, AdbClient &adb)
+AdbClient::CreateByDeviceID(const std::string &device_id, AdbClient &adb)
 {
     DeviceIDList connect_devices;
-    auto error = adb.GetDevices (connect_devices);
-    if (error.Fail ())
+    auto error = adb.GetDevices(connect_devices);
+    if (error.Fail())
         return error;
 
-    if (device_id)
+    if (device_id.empty())
     {
-        auto find_it = std::find(connect_devices.begin (), connect_devices.end (), device_id);
-        if (find_it == connect_devices.end ())
-            return Error ("Device \"%s\" not found", device_id);
+        if (connect_devices.size() != 1)
+            return Error("Expected a single connected device, got instead %" PRIu64,
+                    static_cast<uint64_t>(connect_devices.size()));
 
-        adb.SetDeviceID (*find_it);
+        adb.SetDeviceID(connect_devices.front());
     }
     else
     {
-        if (connect_devices.size () != 1)
-            return Error ("Expected a single connected device, got instead %" PRIu64, static_cast<uint64_t>(connect_devices.size ()));
+        auto find_it = std::find(connect_devices.begin(), connect_devices.end(), device_id);
+        if (find_it == connect_devices.end())
+            return Error("Device \"%s\" not found", device_id.c_str());
 
-        adb.SetDeviceID (connect_devices.front ());
+        adb.SetDeviceID(*find_it);
     }
     return error;
 }
 
 AdbClient::AdbClient (const std::string &device_id)
     : m_device_id (device_id)
 {
 }

source/Plugins/Platform/Android/PlatformAndroid.cpp

 //===-- PlatformAndroid.cpp -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // C Includes
 // C++ Includes
 // Other libraries and framework includes
 #include "lldb/Core/Log.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/HostInfo.h"
+#include "Utility/UriParser.h"
 
 // Project includes
 #include "AdbClient.h"
 #include "PlatformAndroid.h"
 #include "PlatformAndroidRemoteGDBServer.h"
 
 using namespace lldb;
 using namespace lldb_private;

 
 ConstString
 PlatformAndroid::GetPluginName()
 {
     return GetPluginNameStatic(IsHost());
 }
 
 Error
-PlatformAndroid::ConnectRemote (Args& args)
+PlatformAndroid::ConnectRemote(Args& args)
 {
-    m_device_id.clear ();
+    m_device_id.clear();
 
     if (IsHost())
     {
         return Error ("can't connect to the host platform '%s', always connected", GetPluginName().GetCString());
     }
 
     if (!m_remote_platform_sp)
         m_remote_platform_sp = PlatformSP(new PlatformAndroidRemoteGDBServer());
 
-    auto error = PlatformLinux::ConnectRemote (args);
-    if (error.Success ())
+    int port;
+    std::string scheme, host, path;
+    const char *url = args.GetArgumentAtIndex(0);
+    if (!UriParser::Parse(url, scheme, host, port, path))
+        return Error("invalid uri");
+
+    m_device_id.clear();
+    if (scheme == "adb")
+        m_device_id = host;
+
+    auto error = PlatformLinux::ConnectRemote(args);
+    if (error.Success())
     {
         // Fetch the device list from ADB and if only 1 device found then use that device
         // TODO: Handle the case when more device is available
         AdbClient adb;
-        error = AdbClient::CreateByDeviceID (nullptr, adb);
-        if (error.Fail ())
+        error = AdbClient::CreateByDeviceID(m_device_id, adb);
+        if (error.Fail())
             return error;
 
-        m_device_id = adb.GetDeviceID ();
+        m_device_id = adb.GetDeviceID();
     }
     return error;
 }
 
 const char *
 PlatformAndroid::GetCacheHostname ()
 {
     return m_device_id.c_str ();
 }

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h

public:
 
     Error
     ConnectRemote (Args& args) override;
 
     Error
     DisconnectRemote () override;
 
 protected:
-    std::map<lldb::pid_t, std::pair<uint16_t, std::string>> m_port_forwards;
+    std::string m_device_id;
+    std::map<lldb::pid_t, uint16_t> m_port_forwards;
 
     uint16_t
     LaunchGDBserverAndGetPort (lldb::pid_t &pid) override;
 
     bool
     KillSpawnedProcess (lldb::pid_t pid) override;
 
     void

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

 
 static const lldb::pid_t g_remote_platform_pid = 0; // Alias for the process id of lldb-platform
 
 static Error
 ForwardPortWithAdb (uint16_t port, std::string& device_id)
 {
     Log *log(GetLogIfAllCategoriesSet (LIBLLDB_LOG_PLATFORM));
 
-    // Fetch the device list from ADB and if only 1 device found then use that device
-    // TODO: Handle the case when more device is available
     AdbClient adb;
-    auto error = AdbClient::CreateByDeviceID (nullptr, adb);
+    auto error = AdbClient::CreateByDeviceID(device_id, adb);
     if (error.Fail ())
         return error;
 
-    device_id = adb.GetDeviceID ();
+    device_id = adb.GetDeviceID();
     if (log)
         log->Printf("Connected to Android device \"%s\"", device_id.c_str ());
 
-    return adb.SetPortForwarding (port);
+    return adb.SetPortForwarding(port);
 }
 
 static Error
 DeleteForwardPortWithAdb (uint16_t port, const std::string& device_id)
 {
     AdbClient adb (device_id);
     return adb.DeletePortForwarding (port);
 }
 
 PlatformAndroidRemoteGDBServer::PlatformAndroidRemoteGDBServer ()
 {
 }
 
 PlatformAndroidRemoteGDBServer::~PlatformAndroidRemoteGDBServer ()
 {
     for (const auto& it : m_port_forwards)
-    {
-        DeleteForwardPortWithAdb (it.second.first, it.second.second);
-    }
+        DeleteForwardPortWithAdb(it.second, m_device_id);
 }
 
 uint16_t
 PlatformAndroidRemoteGDBServer::LaunchGDBserverAndGetPort (lldb::pid_t &pid)
 {
     uint16_t port = m_gdb_client.LaunchGDBserverAndGetPort (pid, "127.0.0.1");
     if (port == 0)
         return port;
 
-    std::string device_id;
-    Error error = ForwardPortWithAdb (port, device_id);
+    Error error = ForwardPortWithAdb(port, m_device_id);
     if (error.Fail ())
         return 0;
 
-    m_port_forwards[pid] = std::make_pair (port, device_id);
+    m_port_forwards[pid] = port;
 
     return port;
 }
 
 bool
 PlatformAndroidRemoteGDBServer::KillSpawnedProcess (lldb::pid_t pid)
 {
     DeleteForwardPort (pid);
     return m_gdb_client.KillSpawnedProcess (pid);
 }
 
 Error
 PlatformAndroidRemoteGDBServer::ConnectRemote (Args& args)
 {
     if (args.GetArgumentCount () != 1)
         return Error ("\"platform connect\" takes a single argument: <connect-url>");
   
     int port;
     std::string scheme, host, path;
     const char *url = args.GetArgumentAtIndex (0);
     if (!UriParser::Parse (url, scheme, host, port, path))
         return Error ("invalid uri");
 
-    std::string device_id;
-    Error error = ForwardPortWithAdb (port, device_id);
-    if (error.Fail ())
+    m_device_id.clear();
+    if (scheme == "adb")
+    {
+        m_device_id = host;
+        args = Args{"connect://localhost:" + std::to_string(port)};
+    }
+    Error error = ForwardPortWithAdb(port, m_device_id);
+    if (error.Fail())
         return error;
 
-    m_port_forwards[g_remote_platform_pid] = std::make_pair (port, device_id);
+    m_port_forwards[g_remote_platform_pid] = port;
 
-    error = PlatformRemoteGDBServer::ConnectRemote (args);
+    error = PlatformRemoteGDBServer::ConnectRemote(args);
     if (error.Fail ())
         DeleteForwardPort (g_remote_platform_pid);
 
     return error;
 }
 
 Error
 PlatformAndroidRemoteGDBServer::DisconnectRemote ()
 {
     DeleteForwardPort (g_remote_platform_pid);
     return PlatformRemoteGDBServer::DisconnectRemote ();
 }
 
 void
 PlatformAndroidRemoteGDBServer::DeleteForwardPort (lldb::pid_t pid)
 {
-    Log *log(GetLogIfAllCategoriesSet (LIBLLDB_LOG_PLATFORM));
+    Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PLATFORM));
 
-    auto it = m_port_forwards.find (pid);
-    if (it == m_port_forwards.end ())
+    auto it = m_port_forwards.find(pid);
+    if (it == m_port_forwards.end())
         return;
 
-    const auto& forward_val = it->second;
-    const auto error = DeleteForwardPortWithAdb (forward_val.first, forward_val.second);
-    if (error.Fail ()) {
+    const auto port = it->second;
+    const auto error = DeleteForwardPortWithAdb(port, m_device_id);
+    if (error.Fail()) {
         if (log)
-            log->Printf ("Failed to delete port forwarding (pid=%" PRIu64 ", port=%d, device=%s): %s",
-                         pid, forward_val.first, forward_val.second.c_str (), error.AsCString ());
+            log->Printf("Failed to delete port forwarding (pid=%" PRIu64 ", port=%d, device=%s): %s",
+                         pid, port, m_device_id.c_str(), error.AsCString());
     }
-    m_port_forwards.erase (it);
+    m_port_forwards.erase(it);
 }