Move ADB communications to AdbClient class - to make it accessible by other components.

[ovyalov]

Summary

Move ADB communications to AdbClient class - in order to make it accessible by other LLDB components.
I'm planning to use device serial number for Android devices instead of host name within module cache - will integrate AdbClient with PlatformAndroid with a next CL.

[tberghammer]

Thanks for moving it out to a new class.

Please check that the adb port forward is set up correctly (not left over from a previous session) and also check that all of the port forwards are cleaned up after lldb exits (I suggest a manual test as full test suit might leak some adb forwards in case of a failure).

source/Plugins/Platform/Android/AdbClient.cpp

72–76

I think it isn't an error in this scenario if no device was found

110–122

What is the goal of this function? I think it will return m_device_id on success and if m_device_id is not set then it will fail.

source/Plugins/Platform/Android/AdbClient.h

36

Do we need a virtual destructor?

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

44–49

Please return an error if not exactly one device is found and remove the assert.

I think we should add asserts to check for bugs in lldb while having no connected device can be caused by almost anything.

If we found more then 1 device then (randomly) choosing one will cause some flakiness in our tests so I think it is better to report a failure here. I plan to add some way to specify the selected device from platform-android.

[ovyalov]

source/Plugins/Platform/Android/AdbClient.cpp

72–76

Removed.

110–122

Good point - removed it.

source/Plugins/Platform/Android/AdbClient.h

36

Removed it at all.

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

44–49

Makes sense - fixed.

[ovyalov]

Fixed accordingly to suggestions.
Please take another look.

Thank you.

[tberghammer]

LGTM, but please add back the xcode project files and the Makefile changes (included in #1, removed from #2)

[ovyalov]

Added missed lldb.xcodeproj/project.pbxpro - thanks for catching this!

[ovyalov]

AFFECTED FILES

/lldb/trunk/lldb.xcodeproj/project.pbxproj
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.cpp
/lldb/trunk/source/Plugins/Platform/Android/AdbClient.h
/lldb/trunk/source/Plugins/Platform/Android/CMakeLists.txt
/lldb/trunk/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

USERS

ovyalov (Author)

http://reviews.llvm.org/rL233021

source/Plugins/Platform/Android/AdbClient.h

+//===-- AdbClient.h ---------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef liblldb_AdbClient_h_
+#define liblldb_AdbClient_h_
+
+// C Includes
+
+// C++ Includes
+
+#include <list>
+#include <string>
+
+// Other libraries and framework includes
+// Project includes
+
+#include "lldb/Core/Error.h"
+
+namespace lldb_private {
+
+class AdbClient
+{
+public:
+    using DeviceIDList = std::list<std::string>;
+
+    AdbClient () = default;
+    explicit AdbClient (const std::string &device_id);
+
+    void
+    SetDeviceID (const std::string& device_id);
+
+    Error
+    GetDevices (DeviceIDList &device_list);
+
+    Error
+    SetPortForwarding (const uint16_t port);
+
+    Error
+    DeletePortForwarding (const uint16_t port);
+
+private:
+    Error
+    Connect ();
+
+    Error
+    SendMessage (const std::string &packet);
+
+    Error
+    SendDeviceMessage (const std::string &packet);
+
+    Error
+    ReadMessage (std::string &message);
+
+    Error
+    ReadResponseStatus ();
+
+    std::string m_device_id;
+    ConnectionFileDescriptor m_conn;
+};
+
+} // namespace lldb_private
+
+#endif  // liblldb_AdbClient_h_

source/Plugins/Platform/Android/AdbClient.cpp

+//===-- AdbClient.cpp -------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+// Other libraries and framework includes
+#include "lldb/Host/ConnectionFileDescriptor.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/STLExtras.h"
+
+// Project includes
+#include "AdbClient.h"
+
+#include <sstream>
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace {
+
+const uint32_t kConnTimeout = 10000; // 10 ms
+const char * kOKAY = "OKAY";
+const char * kFAIL = "FAIL";
+
+}  // namespace
+
+AdbClient::AdbClient (const std::string &device_id)
+    : m_device_id (device_id)
+{
+}
+
+void
+AdbClient::SetDeviceID (const std::string& device_id)
+{
+    m_device_id = device_id;
+}
+
+Error
+AdbClient::Connect ()
+{
+    Error error;
+    m_conn.Connect ("connect://localhost:5037", &error);
+
+    return error;
+}
+
+Error
+AdbClient::GetDevices (DeviceIDList &device_list)
+{
+    device_list.clear ();
+
+    auto error = SendMessage ("host:devices");
+    if (error.Fail ())
+        return error;
+
+    error = ReadResponseStatus ();
+    if (error.Fail ())
+        return error;
+
+    std::string in_buffer;
+    error = ReadMessage (in_buffer);
+
+    llvm::StringRef response (in_buffer);
+    llvm::SmallVector<llvm::StringRef, 4> devices;
+    response.split (devices, "\n", -1, false);
+
+    for (const auto device: devices)
+        device_list.push_back (device.split ('\t').first);
+
+    return error;
+}
+
+Error
+AdbClient::SetPortForwarding (const uint16_t port)
+{
+    char message[48];
+    snprintf (message, sizeof (message), "forward:tcp:%d;tcp:%d", port, port);
+
+    const auto error = SendDeviceMessage (message);
+    if (error.Fail ())
+        return error;
+
+    return ReadResponseStatus ();
+}
+
+Error
+AdbClient::DeletePortForwarding (const uint16_t port)
+{
+    char message[32];
+    snprintf (message, sizeof (message), "killforward:tcp:%d", port);
+
+    const auto error = SendDeviceMessage (message);
+    if (error.Fail ())
+        return error;
+
+    return ReadResponseStatus ();
+}
+
+Error
+AdbClient::SendMessage (const std::string &packet)
+{
+    auto error = Connect ();
+    if (error.Fail ())
+        return error;
+
+    char length_buffer[5];
+    snprintf (length_buffer, sizeof (length_buffer), "%04zx", packet.size ());
+
+    ConnectionStatus status;
+
+    m_conn.Write (length_buffer, 4, status, &error);
+    if (error.Fail ())
+        return error;
+
+    m_conn.Write (packet.c_str (), packet.size (), status, &error);
+    return error;
+}
+
+Error
+AdbClient::SendDeviceMessage (const std::string &packet)
+{
+    std::ostringstream msg;
+    msg << "host-serial:" << m_device_id << ":" << packet;
+    return SendMessage (msg.str ());
+}
+
+Error
+AdbClient::ReadMessage (std::string &message)
+{
+    message.clear ();
+
+    char buffer[5];
+    buffer[4] = 0;
+
+    Error error;
+    ConnectionStatus status;
+
+    m_conn.Read (buffer, 4, kConnTimeout, status, &error);
+    if (error.Fail ())
+        return error;
+
+    size_t packet_len = 0;
+    sscanf (buffer, "%zx", &packet_len);
+    std::string result (packet_len, 0);
+    m_conn.Read (&result[0], packet_len, kConnTimeout, status, &error);
+    if (error.Success ())
+        result.swap (message);
+
+    return error;
+}
+
+Error
+AdbClient::ReadResponseStatus()
+{
+    char buffer[5];
+
+    static const size_t packet_len = 4;
+    buffer[packet_len] = 0;
+
+    Error error;
+    ConnectionStatus status;
+
+    m_conn.Read (buffer, packet_len, kConnTimeout, status, &error);
+    if (error.Fail ())
+      return error;
+
+    if (strncmp (buffer, kOKAY, packet_len) != 0)
+    {
+        if (strncmp (buffer, kFAIL, packet_len) == 0)
+        {
+            std::string error_message;
+            error = ReadMessage (error_message);
+            if (error.Fail ())
+                return error;
+            error.SetErrorString (error_message.c_str ());
+        }
+        else
+            error.SetErrorStringWithFormat ("\"%s\" expected from adb, received: \"%s\"", kOKAY, buffer);
+    }
+
+    return error;
+}

source/Plugins/Platform/Android/CMakeLists.txt

 set(LLVM_NO_RTTI 1)
 
 add_lldb_library(lldbPluginPlatformAndroid
+  AdbClient.cpp
   PlatformAndroid.cpp
   PlatformAndroidRemoteGDBServer.cpp
   )

source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

 //===-- PlatformAndroidRemoteGDBServer.cpp ----------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // Other libraries and framework includes
 #include "lldb/Core/Error.h"
+#include "lldb/Core/Log.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "llvm/ADT/StringRef.h"
 
 // Project includes
+#include "AdbClient.h"
 #include "PlatformAndroidRemoteGDBServer.h"
 #include "Utility/UriParser.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 static const lldb::pid_t g_remote_platform_pid = 0; // Alias for the process id of lldb-platform
-static const uint32_t g_adb_timeout = 10000; // 10 ms
-
-static void
-SendMessageToAdb (Connection& conn, const std::string& packet, Error& error)
-{
-    ConnectionStatus status;
-
-    char length_buffer[5];
-    snprintf (length_buffer, sizeof (length_buffer), "%04zx", packet.size());
-
-    conn.Write (length_buffer, 4, status, &error);
-    if (error.Fail ())
-        return;
-
-    conn.Write (packet.c_str(), packet.size(), status, &error);
-}
-
-static std::string
-ReadMessageFromAdb (Connection& conn, bool has_okay, Error& error)
-{
-    ConnectionStatus status;
-
-    char buffer[5];
-    buffer[4] = 0;
-
-    if (has_okay)
-    {
-        conn.Read (buffer, 4, g_adb_timeout, status, &error);
-        if (error.Fail ())
-            return "";
-
-        if (strncmp (buffer, "OKAY", 4) != 0)
-        {
-            error.SetErrorStringWithFormat ("\"OKAY\" expected from adb, received: \"%s\"", buffer);
-            return "";
-        }
-    }
-
-    conn.Read (buffer, 4, g_adb_timeout, status, &error);
-    if (error.Fail())
-        return "";
-
-    size_t packet_len = 0;
-    sscanf(buffer, "%zx", &packet_len);
-    std::string result(packet_len, 0);
-    conn.Read (&result[0], packet_len, g_adb_timeout, status, &error);
-    if (error.Fail ())
-        return "";
-
-    return result;
-}
 
 static Error
 ForwardPortWithAdb (uint16_t port, std::string& device_id)
 {
-    Error error;
+    Log *log(lldb_private::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
-        std::unique_ptr<ConnectionFileDescriptor> conn (new ConnectionFileDescriptor ());
-        if (conn->Connect ("connect://localhost:5037", &error) != eConnectionStatusSuccess)
-            return error;
+    AdbClient adb;
 
-        SendMessageToAdb (*conn, "host:devices", error);
-        if (error.Fail ())
-            return error;
-        std::string in_buffer = ReadMessageFromAdb (*conn, true, error);
-
-        llvm::StringRef deviceList(in_buffer);
-        std::pair<llvm::StringRef, llvm::StringRef> devices = deviceList.split ('\n');
-        if (devices.first.size () == 0 || devices.second.size () > 0)
-        {
-            error.SetErrorString ("Wrong number of devices returned from ADB");
-            return error;
-        }
-
-        device_id = devices.first.split ('\t').first;
-    }
-
-    {
-        // Forward the port to the (only) connected device
-        std::unique_ptr<ConnectionFileDescriptor> conn (new ConnectionFileDescriptor ());
-        if (conn->Connect ("connect://localhost:5037", &error) != eConnectionStatusSuccess)
-            return error;
-
-        char port_buffer[32];
-        snprintf (port_buffer, sizeof (port_buffer), "tcp:%d;tcp:%d", port, port);
+    AdbClient::DeviceIDList connect_devices;
+    auto error = adb.GetDevices (connect_devices);
 
-        std::string out_buffer = "host-serial:" + device_id + ":forward:" + port_buffer;
-        SendMessageToAdb (*conn, out_buffer, error);
-        if (error.Fail ())
-            return error;
+    if (connect_devices.size () != 1)
+        return Error ("Expected a single connected device, got instead %" PRIu64, connect_devices.size ());
 
-        std::string in_buffer = ReadMessageFromAdb (*conn, false, error);
-        if (in_buffer != "OKAY")
-            error.SetErrorString (in_buffer.c_str ());
-    }
+    device_id = connect_devices.front ();
+    if (log)
+        log->Printf("Connected to Android device \"%s\"", device_id.c_str ());
 
-    return error;
+    adb.SetDeviceID (device_id);
+    return adb.SetPortForwarding (port);
 }
 
 static Error
 DeleteForwardPortWithAdb (uint16_t port, const std::string& device_id)
 {
-    Error error;
-
-    std::unique_ptr<ConnectionFileDescriptor> conn (new ConnectionFileDescriptor ());
-    if (conn->Connect ("connect://localhost:5037", &error) != eConnectionStatusSuccess)
-        return error;
-
-    char port_buffer[16];
-    snprintf (port_buffer, sizeof (port_buffer), "tcp:%d", port);
-
-    std::string out_buffer = "host-serial:" + device_id + ":killforward:" + port_buffer;
-    SendMessageToAdb (*conn, out_buffer, error);
-    if (error.Fail ())
-        return error;
-
-    std::string in_buffer = ReadMessageFromAdb (*conn, true, error);
-    if (in_buffer != "OKAY")
-        error.SetErrorString (in_buffer.c_str ());
-
-    return error;
+    AdbClient adb (device_id);
+    return adb.DeletePortForwarding (port);
 }
 
 PlatformAndroidRemoteGDBServer::PlatformAndroidRemoteGDBServer ()
 {
 }
 
 PlatformAndroidRemoteGDBServer::~PlatformAndroidRemoteGDBServer ()
 {