adb: fix two device offline problems.
When device goes offline, user usually has to manually replug the
usb device. This patch tries to solve two offline situations, all
because when adb on host is killed, the adbd on device is not notified.
1. When adb server is killed while pushing a large file to device,
the device is still reading the unfinished large message. So the
device thinks of the CNXN message as part of the previous unfinished
message, so it doesn't reply and the device is in offline state.
The solution is to add a write_msg_lock in atransport struct. And it
kicks the transport only after sending a whole message. By kicking
all transports before exit, we ensure that we don't write part of
a message to any device. So next time we start adb server, the device
should be waiting for a new message.
2. When adb server is killed while pulling a large file from device,
the device is still trying to send the unfinished large message. So
adb on host usually reads data with EOVERFLOW error. This is because
adb on host is reading less than one packet sent from device.
The solution is to use buffered read on host. The max packet size
of bulk transactions in USB 3.0 is 1024 bytes. By preparing an at least
1024 bytes buffer when reading, EOVERFLOW no longer occurs. And teach
adb host to ignore wrong messages.
To be safe, this patch doesn't change any logic on device.
Bug: http://b/32952319
Test: run python -m unittest -q test_device.DeviceOfflineTest
Test: on linux/mac/windows with bullhead, ryu.
Change-Id: Ib149d30028a62a6f03857b8a95ab5a1d6e9b9c4e
diff --git a/adb/transport.cpp b/adb/transport.cpp
index 4686841..cc8c162 100644
--- a/adb/transport.cpp
+++ b/adb/transport.cpp
@@ -33,6 +33,7 @@
#include <android-base/logging.h>
#include <android-base/parsenetaddress.h>
+#include <android-base/quick_exit.h>
#include <android-base/stringprintf.h>
#include <android-base/strings.h>
@@ -41,6 +42,7 @@
#include "adb_trace.h"
#include "adb_utils.h"
#include "diagnose_usb.h"
+#include "fdevent.h"
static void transport_unref(atransport *t);
@@ -209,6 +211,11 @@
put_apacket(p);
break;
}
+#if ADB_HOST
+ if (p->msg.command == 0) {
+ continue;
+ }
+#endif
}
D("%s: received remote packet, sending to transport", t->serial);
@@ -271,7 +278,11 @@
if (active) {
D("%s: transport got packet, sending to remote", t->serial);
ATRACE_NAME("write_transport write_remote");
- t->write_to_remote(p, t);
+ if (t->Write(p) != 0) {
+ D("%s: remote write failed for transport", t->serial);
+ put_apacket(p);
+ break;
+ }
} else {
D("%s: transport ignoring packet while offline", t->serial);
}
@@ -493,7 +504,7 @@
}
/* don't create transport threads for inaccessible devices */
- if (t->connection_state != kCsNoPerm) {
+ if (t->GetConnectionState() != kCsNoPerm) {
/* initial references are the two threads */
t->ref_count = 2;
@@ -538,6 +549,15 @@
transport_registration_func, 0);
fdevent_set(&transport_registration_fde, FDE_READ);
+#if ADB_HOST
+ android::base::at_quick_exit([]() {
+ // To avoid only writing part of a packet to a transport after exit, kick all transports.
+ std::lock_guard<std::mutex> lock(transport_lock);
+ for (auto t : transport_list) {
+ t->Kick();
+ }
+ });
+#endif
}
/* the fdevent select pump is single threaded */
@@ -600,7 +620,7 @@
}
atransport* acquire_one_transport(TransportType type, const char* serial, bool* is_ambiguous,
- std::string* error_out) {
+ std::string* error_out, bool accept_any_state) {
atransport* result = nullptr;
if (serial) {
@@ -615,7 +635,7 @@
std::unique_lock<std::mutex> lock(transport_lock);
for (const auto& t : transport_list) {
- if (t->connection_state == kCsNoPerm) {
+ if (t->GetConnectionState() == kCsNoPerm) {
#if ADB_HOST
*error_out = UsbNoPermissionsLongHelpText();
#endif
@@ -664,7 +684,7 @@
lock.unlock();
// Don't return unauthorized devices; the caller can't do anything with them.
- if (result && result->connection_state == kCsUnauthorized) {
+ if (result && result->GetConnectionState() == kCsUnauthorized && !accept_any_state) {
*error_out = "device unauthorized.\n";
char* ADB_VENDOR_KEYS = getenv("ADB_VENDOR_KEYS");
*error_out += "This adb server's $ADB_VENDOR_KEYS is ";
@@ -676,7 +696,7 @@
}
// Don't return offline devices; the caller can't do anything with them.
- if (result && result->connection_state == kCsOffline) {
+ if (result && result->GetConnectionState() == kCsOffline && !accept_any_state) {
*error_out = "device offline";
result = nullptr;
}
@@ -688,16 +708,38 @@
return result;
}
+int atransport::Write(apacket* p) {
+#if ADB_HOST
+ std::lock_guard<std::mutex> lock(write_msg_lock_);
+#endif
+ return write_func_(p, this);
+}
+
void atransport::Kick() {
if (!kicked_) {
kicked_ = true;
CHECK(kick_func_ != nullptr);
+#if ADB_HOST
+ // On host, adb server should avoid writing part of a packet, so don't
+ // kick a transport whiling writing a packet.
+ std::lock_guard<std::mutex> lock(write_msg_lock_);
+#endif
kick_func_(this);
}
}
+ConnectionState atransport::GetConnectionState() const {
+ return connection_state_;
+}
+
+void atransport::SetConnectionState(ConnectionState state) {
+ check_main_thread();
+ connection_state_ = state;
+}
+
const std::string atransport::connection_state_name() const {
- switch (connection_state) {
+ ConnectionState state = GetConnectionState();
+ switch (state) {
case kCsOffline:
return "offline";
case kCsBootloader:
@@ -963,10 +1005,10 @@
void register_usb_transport(usb_handle* usb, const char* serial, const char* devpath,
unsigned writeable) {
- atransport* t = new atransport();
+ atransport* t = new atransport((writeable ? kCsOffline : kCsNoPerm));
D("transport: %p init'ing for usb_handle %p (sn='%s')", t, usb, serial ? serial : "");
- init_usb_transport(t, usb, (writeable ? kCsOffline : kCsNoPerm));
+ init_usb_transport(t, usb);
if (serial) {
t->serial = strdup(serial);
}
@@ -987,12 +1029,13 @@
void unregister_usb_transport(usb_handle* usb) {
std::lock_guard<std::mutex> lock(transport_lock);
transport_list.remove_if(
- [usb](atransport* t) { return t->usb == usb && t->connection_state == kCsNoPerm; });
+ [usb](atransport* t) { return t->usb == usb && t->GetConnectionState() == kCsNoPerm; });
}
int check_header(apacket* p, atransport* t) {
if (p->msg.magic != (p->msg.command ^ 0xffffffff)) {
- VLOG(RWX) << "check_header(): invalid magic";
+ VLOG(RWX) << "check_header(): invalid magic command = " << std::hex << p->msg.command
+ << ", magic = " << p->msg.magic;
return -1;
}
@@ -1020,4 +1063,11 @@
keys_.pop_front();
return result;
}
+bool atransport::SetSendConnectOnError() {
+ if (has_send_connect_on_error_) {
+ return false;
+ }
+ has_send_connect_on_error_ = true;
+ return true;
+}
#endif