[Lldb-commits] [PATCH] Move ADB communications to AdbClient class - to make it accessible by other components.

[Tamas Berghammer]

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).


================
Comment at: source/Plugins/Platform/Android/AdbClient.cpp:71-75
@@ +70,7 @@
+    response.split (devices, "\n");
+    if (devices.empty ())
+    {
+        error.SetErrorString ("Wrong number of devices returned from ADB");
+        return error;
+    }
+
----------------
I think it isn't an error in this scenario if no device was found

================
Comment at: source/Plugins/Platform/Android/AdbClient.cpp:109-121
@@ +108,15 @@
+
+Error
+AdbClient::GetSerialNumber (std::string& sn)
+{
+    auto error = SendDeviceMessage ("get-serialno");
+    if (error.Fail ())
+        return error;
+
+    error = ReadResponseStatus ();
+    if (error.Fail ())
+        return error;
+
+    return ReadMessage (sn);
+}
+
----------------
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.

================
Comment at: source/Plugins/Platform/Android/AdbClient.h:35
@@ +34,3 @@
+
+    virtual ~AdbClient () = default;
+
----------------
Do we need a virtual destructor?

================
Comment at: source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp:36-41
@@ +35,8 @@
+    AdbClient::DeviceIDList connect_devices;
+    auto error = adb.GetDevices (connect_devices);
+    if (error.Fail ())
+        return error;
+
+    assert (!connect_devices.empty ());
+    device_id = connect_devices.front ();
+    if (log)
----------------
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.

http://reviews.llvm.org/D8535

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/