Merge "adb: add unit test for fdevent."
diff --git a/Android.mk b/Android.mk
index 88e0418..41016ee 100644
--- a/Android.mk
+++ b/Android.mk
@@ -198,7 +198,6 @@
ifeq ($(HOST_OS),linux)
LOCAL_LDLIBS += -lrt -ldl -lpthread
- LOCAL_CFLAGS += -DWORKAROUND_BUG6558362
endif
ifeq ($(HOST_OS),darwin)
diff --git a/adb.cpp b/adb.cpp
index 49cf123..eb01da8 100644
--- a/adb.cpp
+++ b/adb.cpp
@@ -244,11 +244,11 @@
//Close the associated usb
t->online = 0;
- // This is necessary to avoid a race condition that occured when a transport closes
+ // This is necessary to avoid a race condition that occurred when a transport closes
// while a client socket is still active.
close_all_sockets(t);
- run_transport_disconnects(t);
+ t->RunDisconnects();
}
#if DEBUG_PACKETS
@@ -670,10 +670,12 @@
}
static unsigned __stdcall _redirect_stdout_thread(HANDLE h) {
+ adb_thread_setname("stdout redirect");
return _redirect_pipe_thread(h, STD_OUTPUT_HANDLE);
}
static unsigned __stdcall _redirect_stderr_thread(HANDLE h) {
+ adb_thread_setname("stderr redirect");
return _redirect_pipe_thread(h, STD_ERROR_HANDLE);
}
diff --git a/adb.h b/adb.h
index 6855f3b..8b3359e 100644
--- a/adb.h
+++ b/adb.h
@@ -157,8 +157,6 @@
{
void (*func)(void* opaque, atransport* t);
void* opaque;
- adisconnect* next;
- adisconnect* prev;
};
@@ -229,8 +227,8 @@
void connect_to_remote(asocket *s, const char *destination);
void connect_to_smartsocket(asocket *s);
-void fatal(const char *fmt, ...);
-void fatal_errno(const char *fmt, ...);
+void fatal(const char *fmt, ...) __attribute__((noreturn));
+void fatal_errno(const char *fmt, ...) __attribute__((noreturn));
void handle_packet(apacket *p, atransport *t);
diff --git a/adb_auth_client.cpp b/adb_auth_client.cpp
index be28202..c3af024 100644
--- a/adb_auth_client.cpp
+++ b/adb_auth_client.cpp
@@ -47,7 +47,7 @@
static int framework_fd = -1;
static void usb_disconnected(void* unused, atransport* t);
-static struct adisconnect usb_disconnect = { usb_disconnected, 0, 0, 0 };
+static struct adisconnect usb_disconnect = { usb_disconnected, nullptr};
static atransport* usb_transport;
static bool needs_retry = false;
@@ -164,7 +164,6 @@
static void usb_disconnected(void* unused, atransport* t)
{
D("USB disconnect\n");
- remove_transport_disconnect(usb_transport, &usb_disconnect);
usb_transport = NULL;
needs_retry = false;
}
@@ -196,7 +195,7 @@
if (!usb_transport) {
usb_transport = t;
- add_transport_disconnect(t, &usb_disconnect);
+ t->AddDisconnect(&usb_disconnect);
}
if (framework_fd < 0) {
diff --git a/adb_listeners.cpp b/adb_listeners.cpp
index 8fb2d19..d5b1fd5 100644
--- a/adb_listeners.cpp
+++ b/adb_listeners.cpp
@@ -101,13 +101,15 @@
free((char*)l->connect_to);
if (l->transport) {
- remove_transport_disconnect(l->transport, &l->disconnect);
+ l->transport->RemoveDisconnect(&l->disconnect);
}
free(l);
}
-static void listener_disconnect(void* listener, atransport* t) {
- free_listener(reinterpret_cast<alistener*>(listener));
+static void listener_disconnect(void* arg, atransport*) {
+ alistener* listener = reinterpret_cast<alistener*>(arg);
+ listener->transport = nullptr;
+ free_listener(listener);
}
static int local_name_to_fd(const char* name, std::string* error) {
@@ -159,7 +161,7 @@
for (l = listener_list.next; l != &listener_list; l = l->next) {
if (!strcmp(local_name, l->local_name)) {
- listener_disconnect(l, l->transport);
+ free_listener(l);
return INSTALL_STATUS_OK;
}
}
@@ -174,7 +176,7 @@
// Never remove smart sockets.
if (l->connect_to[0] == '*')
continue;
- listener_disconnect(l, l->transport);
+ free_listener(l);
}
}
@@ -209,9 +211,9 @@
free((void*) l->connect_to);
l->connect_to = cto;
if (l->transport != transport) {
- remove_transport_disconnect(l->transport, &l->disconnect);
+ l->transport->RemoveDisconnect(&l->disconnect);
l->transport = transport;
- add_transport_disconnect(l->transport, &l->disconnect);
+ l->transport->AddDisconnect(&l->disconnect);
}
return INSTALL_STATUS_OK;
}
@@ -260,7 +262,7 @@
if (transport) {
listener->disconnect.opaque = listener;
listener->disconnect.func = listener_disconnect;
- add_transport_disconnect(transport, &listener->disconnect);
+ transport->AddDisconnect(&listener->disconnect);
}
return INSTALL_STATUS_OK;
diff --git a/client/main.cpp b/client/main.cpp
index f6ddeb4..8d644d9 100644
--- a/client/main.cpp
+++ b/client/main.cpp
@@ -36,42 +36,6 @@
#include "adb_listeners.h"
#include "transport.h"
-#if defined(WORKAROUND_BUG6558362) && defined(__linux__)
-static const bool kWorkaroundBug6558362 = true;
-#else
-static const bool kWorkaroundBug6558362 = false;
-#endif
-
-static void adb_workaround_affinity(void) {
-#if defined(__linux__)
- const char affinity_env[] = "ADB_CPU_AFFINITY_BUG6558362";
- const char* cpunum_str = getenv(affinity_env);
- if (cpunum_str == nullptr || *cpunum_str == '\0') {
- return;
- }
-
- char* strtol_res;
- int cpu_num = strtol(cpunum_str, &strtol_res, 0);
- if (*strtol_res != '\0') {
- fatal("bad number (%s) in env var %s. Expecting 0..n.\n", cpunum_str,
- affinity_env);
- }
-
- cpu_set_t cpu_set;
- sched_getaffinity(0, sizeof(cpu_set), &cpu_set);
- D("orig cpu_set[0]=0x%08lx\n", cpu_set.__bits[0]);
-
- CPU_ZERO(&cpu_set);
- CPU_SET(cpu_num, &cpu_set);
- sched_setaffinity(0, sizeof(cpu_set), &cpu_set);
-
- sched_getaffinity(0, sizeof(cpu_set), &cpu_set);
- D("new cpu_set[0]=0x%08lx\n", cpu_set.__bits[0]);
-#else
- // No workaround was ever implemented for the other platforms.
-#endif
-}
-
#if defined(_WIN32)
static const char kNullFileName[] = "NUL";
@@ -157,10 +121,6 @@
init_transport_registration();
- if (kWorkaroundBug6558362 && is_daemon) {
- adb_workaround_affinity();
- }
-
usb_init();
local_init(DEFAULT_ADB_LOCAL_TRANSPORT_PORT);
adb_auth_init();
diff --git a/commandline.cpp b/commandline.cpp
index b92757f..6325e64 100644
--- a/commandline.cpp
+++ b/commandline.cpp
@@ -374,6 +374,8 @@
fdi = fds[1];
free(fds);
+ adb_thread_setname("stdin reader");
+
while (true) {
/* fdi is really the client's stdin, so use read, not adb_read here */
D("stdin_read_thread(): pre unix_read(fdi=%d,...)\n", fdi);
diff --git a/services.cpp b/services.cpp
index c8c2d54..4606804 100644
--- a/services.cpp
+++ b/services.cpp
@@ -67,6 +67,7 @@
void *service_bootstrap_func(void *x)
{
stinfo* sti = reinterpret_cast<stinfo*>(x);
+ adb_thread_setname(android::base::StringPrintf("service %d", sti->fd));
sti->func(sti->fd, sti->cookie);
free(sti);
return 0;
diff --git a/sysdeps.h b/sysdeps.h
index a58a762..5918a94 100644
--- a/sysdeps.h
+++ b/sysdeps.h
@@ -111,6 +111,13 @@
return (tid != static_cast<uintptr_t>(-1L));
}
+static __inline__ int adb_thread_setname(const std::string& name) {
+ // TODO: See https://msdn.microsoft.com/en-us/library/xcb2z8hs.aspx for how to set
+ // the thread name in Windows. Unfortunately, it only works during debugging, but
+ // our build process doesn't generate PDB files needed for debugging.
+ return 0;
+}
+
static __inline__ unsigned long adb_thread_id()
{
return GetCurrentThreadId();
@@ -617,7 +624,26 @@
return (errno == 0);
}
-static __inline__ int adb_socket_setbufsize( int fd, int bufsize )
+static __inline__ int adb_thread_setname(const std::string& name) {
+#ifdef __APPLE__
+ return pthread_setname_np(name.c_str());
+#else
+ const char *s = name.c_str();
+
+ // pthread_setname_np fails rather than truncating long strings.
+ const int max_task_comm_len = 16; // including the null terminator
+ if (name.length() > (max_task_comm_len - 1)) {
+ char buf[max_task_comm_len];
+ strncpy(buf, name.c_str(), sizeof(buf) - 1);
+ buf[sizeof(buf) - 1] = '\0';
+ s = buf;
+ }
+
+ return pthread_setname_np(pthread_self(), s) ;
+#endif
+}
+
+static __inline__ int adb_socket_setbufsize(int fd, int bufsize )
{
int opt = bufsize;
return setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &opt, sizeof(opt));
diff --git a/transport.cpp b/transport.cpp
index 4dc5e4a..5a962de 100644
--- a/transport.cpp
+++ b/transport.cpp
@@ -42,36 +42,6 @@
ADB_MUTEX_DEFINE( transport_lock );
-// Each atransport contains a list of adisconnects (t->disconnects).
-// An adisconnect contains a link to the next/prev adisconnect, a function
-// pointer to a disconnect callback which takes a void* piece of user data and
-// the atransport, and some user data for the callback (helpfully named
-// "opaque").
-//
-// The list is circular. New items are added to the entry member of the list
-// (t->disconnects) by add_transport_disconnect.
-//
-// run_transport_disconnects invokes each function in the list.
-//
-// Gotchas:
-// * run_transport_disconnects assumes that t->disconnects is non-null, so
-// this can't be run on a zeroed atransport.
-// * The callbacks in this list are not removed when called, and this function
-// is not guarded against running more than once. As such, ensure that this
-// function is not called multiple times on the same atransport.
-// TODO(danalbert): Just fix this so that it is guarded once you have tests.
-void run_transport_disconnects(atransport* t)
-{
- adisconnect* dis = t->disconnects.next;
-
- D("%s: run_transport_disconnects\n", t->serial);
- while (dis != &t->disconnects) {
- adisconnect* next = dis->next;
- dis->func( dis->opaque, t );
- dis = next;
- }
-}
-
static void dump_packet(const char* name, const char* func, apacket* p) {
unsigned command = p->msg.command;
int len = p->msg.data_length;
@@ -205,25 +175,27 @@
}
}
-/* The transport is opened by transport_register_func before
-** the input and output threads are started.
-**
-** The output thread issues a SYNC(1, token) message to let
-** the input thread know to start things up. In the event
-** of transport IO failure, the output thread will post a
-** SYNC(0,0) message to ensure shutdown.
-**
-** The transport will not actually be closed until both
-** threads exit, but the input thread will kick the transport
-** on its way out to disconnect the underlying device.
-*/
-
-static void *output_thread(void *_t)
+// The transport is opened by transport_register_func before
+// the read_transport and write_transport threads are started.
+//
+// The read_transport thread issues a SYNC(1, token) message to let
+// the write_transport thread know to start things up. In the event
+// of transport IO failure, the read_transport thread will post a
+// SYNC(0,0) message to ensure shutdown.
+//
+// The transport will not actually be closed until both threads exit, but the threads
+// will kick the transport on their way out to disconnect the underlying device.
+//
+// read_transport thread reads data from a transport (representing a usb/tcp connection),
+// and makes the main thread call handle_packet().
+static void *read_transport_thread(void *_t)
{
atransport *t = reinterpret_cast<atransport*>(_t);
apacket *p;
- D("%s: starting transport output thread on fd %d, SYNC online (%d)\n",
+ adb_thread_setname(android::base::StringPrintf("<-%s",
+ (t->serial != nullptr ? t->serial : "transport")));
+ D("%s: starting read_transport thread on fd %d, SYNC online (%d)\n",
t->serial, t->fd, t->sync_token + 1);
p = get_apacket();
p->msg.command = A_SYNC;
@@ -267,19 +239,23 @@
}
oops:
- D("%s: transport output thread is exiting\n", t->serial);
+ D("%s: read_transport thread is exiting\n", t->serial);
kick_transport(t);
transport_unref(t);
return 0;
}
-static void *input_thread(void *_t)
+// write_transport thread gets packets sent by the main thread (through send_packet()),
+// and writes to a transport (representing a usb/tcp connection).
+static void *write_transport_thread(void *_t)
{
atransport *t = reinterpret_cast<atransport*>(_t);
apacket *p;
int active = 0;
- D("%s: starting transport input thread, reading from fd %d\n",
+ adb_thread_setname(android::base::StringPrintf("->%s",
+ (t->serial != nullptr ? t->serial : "transport")));
+ D("%s: starting write_transport thread, reading from fd %d\n",
t->serial, t->fd);
for(;;){
@@ -314,7 +290,7 @@
put_apacket(p);
}
- D("%s: transport input thread is exiting, fd %d\n", t->serial, t->fd);
+ D("%s: write_transport thread is exiting, fd %d\n", t->serial, t->fd);
kick_transport(t);
transport_unref(t);
return 0;
@@ -574,12 +550,12 @@
fdevent_set(&(t->transport_fde), FDE_READ);
- if (!adb_thread_create(input_thread, t)) {
- fatal_errno("cannot create input thread");
+ if (!adb_thread_create(write_transport_thread, t)) {
+ fatal_errno("cannot create write_transport thread");
}
- if (!adb_thread_create(output_thread, t)) {
- fatal_errno("cannot create output thread");
+ if (!adb_thread_create(read_transport_thread, t)) {
+ fatal_errno("cannot create read_transport thread");
}
}
@@ -588,8 +564,6 @@
transport_list.push_front(t);
adb_mutex_unlock(&transport_lock);
- t->disconnects.next = t->disconnects.prev = &t->disconnects;
-
update_transports();
}
@@ -653,23 +627,6 @@
adb_mutex_unlock(&transport_lock);
}
-void add_transport_disconnect(atransport* t, adisconnect* dis)
-{
- adb_mutex_lock(&transport_lock);
- dis->next = &t->disconnects;
- dis->prev = dis->next->prev;
- dis->prev->next = dis;
- dis->next->prev = dis;
- adb_mutex_unlock(&transport_lock);
-}
-
-void remove_transport_disconnect(atransport* t, adisconnect* dis)
-{
- dis->prev->next = dis->next;
- dis->next->prev = dis->prev;
- dis->next = dis->prev = dis;
-}
-
static int qual_match(const char *to_test,
const char *prefix, const char *qual, bool sanitize_qual)
{
@@ -844,6 +801,21 @@
return has_feature(feature) && supported_features().count(feature) > 0;
}
+void atransport::AddDisconnect(adisconnect* disconnect) {
+ disconnects_.push_back(disconnect);
+}
+
+void atransport::RemoveDisconnect(adisconnect* disconnect) {
+ disconnects_.remove(disconnect);
+}
+
+void atransport::RunDisconnects() {
+ for (auto& disconnect : disconnects_) {
+ disconnect->func(disconnect->opaque, this);
+ }
+ disconnects_.clear();
+}
+
#if ADB_HOST
static void append_transport_info(std::string* result, const char* key,
@@ -969,9 +941,9 @@
for (auto& t : transport_list) {
// TCP/IP devices have adb_port == 0.
if (t->type == kTransportLocal && t->adb_port == 0) {
- // Kicking breaks the output thread of this transport out of any read, then
- // the output thread will notify the main thread to make this transport
- // offline. Then the main thread will notify the input thread to exit.
+ // Kicking breaks the read_transport thread of this transport out of any read, then
+ // the read_transport thread will notify the main thread to make this transport
+ // offline. Then the main thread will notify the write_transport thread to exit.
// Finally, this transport will be closed and freed in the main thread.
kick_transport_locked(t);
}
diff --git a/transport.h b/transport.h
index abb26a7..3b56c55 100644
--- a/transport.h
+++ b/transport.h
@@ -19,6 +19,7 @@
#include <sys/types.h>
+#include <list>
#include <string>
#include <unordered_set>
@@ -71,9 +72,6 @@
int adb_port = -1; // Use for emulators (local transport)
bool kicked = false;
- // A list of adisconnect callbacks called when the transport is kicked.
- adisconnect disconnects = {};
-
void* key = nullptr;
unsigned char token[TOKEN_SIZE] = {};
fdevent auth_fde;
@@ -96,6 +94,10 @@
// feature.
bool CanUseFeature(const std::string& feature) const;
+ void AddDisconnect(adisconnect* disconnect);
+ void RemoveDisconnect(adisconnect* disconnect);
+ void RunDisconnects();
+
private:
// A set of features transmitted in the banner with the initial connection.
// This is stored in the banner as 'features=feature0,feature1,etc'.
@@ -103,6 +105,9 @@
int protocol_version;
size_t max_payload;
+ // A list of adisconnect callbacks called when the transport is kicked.
+ std::list<adisconnect*> disconnects_;
+
DISALLOW_COPY_AND_ASSIGN(atransport);
};
@@ -114,10 +119,7 @@
*/
atransport* acquire_one_transport(ConnectionState state, TransportType type,
const char* serial, std::string* error_out);
-void add_transport_disconnect(atransport* t, adisconnect* dis);
-void remove_transport_disconnect(atransport* t, adisconnect* dis);
void kick_transport(atransport* t);
-void run_transport_disconnects(atransport* t);
void update_transports(void);
void init_transport_registration(void);
diff --git a/transport_local.cpp b/transport_local.cpp
index 6a17497..6821cfc 100644
--- a/transport_local.cpp
+++ b/transport_local.cpp
@@ -123,6 +123,7 @@
#if ADB_HOST
static void *client_socket_thread(void *x)
{
+ adb_thread_setname("client_socket_thread");
D("transport: client_socket_thread() starting\n");
while (true) {
int port = DEFAULT_ADB_LOCAL_TRANSPORT_PORT;
@@ -146,6 +147,7 @@
socklen_t alen;
int port = (int) (uintptr_t) arg;
+ adb_thread_setname("server socket");
D("transport: server_socket_thread() starting\n");
serverfd = -1;
for(;;) {
@@ -231,6 +233,7 @@
char tmp[256];
char con_name[32];
+ adb_thread_setname("qemu socket");
D("transport: qemu_socket_thread() starting\n");
/* adb QEMUD service connection request. */
diff --git a/transport_test.cpp b/transport_test.cpp
index 743d97d..10872ac 100644
--- a/transport_test.cpp
+++ b/transport_test.cpp
@@ -51,9 +51,6 @@
EXPECT_EQ(adb_port, rhs.adb_port);
EXPECT_EQ(kicked, rhs.kicked);
- EXPECT_EQ(
- 0, memcmp(&disconnects, &rhs.disconnects, sizeof(adisconnect)));
-
EXPECT_EQ(key, rhs.key);
EXPECT_EQ(0, memcmp(token, rhs.token, TOKEN_SIZE));
EXPECT_EQ(0, memcmp(&auth_fde, &rhs.auth_fde, sizeof(fdevent)));
@@ -118,12 +115,33 @@
ASSERT_EQ(expected, t);
}
-// Disabled because the function currently segfaults for a zeroed atransport. I
-// want to make sure I understand how this is working at all before I try fixing
-// that.
-TEST(transport, DISABLED_run_transport_disconnects_zeroed_atransport) {
+static void DisconnectFunc(void* arg, atransport*) {
+ int* count = reinterpret_cast<int*>(arg);
+ ++*count;
+}
+
+TEST(transport, RunDisconnects) {
atransport t;
- run_transport_disconnects(&t);
+ // RunDisconnects() can be called with an empty atransport.
+ t.RunDisconnects();
+
+ int count = 0;
+ adisconnect disconnect;
+ disconnect.func = DisconnectFunc;
+ disconnect.opaque = &count;
+ t.AddDisconnect(&disconnect);
+ t.RunDisconnects();
+ ASSERT_EQ(1, count);
+
+ // disconnect should have been removed automatically.
+ t.RunDisconnects();
+ ASSERT_EQ(1, count);
+
+ count = 0;
+ t.AddDisconnect(&disconnect);
+ t.RemoveDisconnect(&disconnect);
+ t.RunDisconnects();
+ ASSERT_EQ(0, count);
}
TEST(transport, add_feature) {
diff --git a/usb_linux.cpp b/usb_linux.cpp
index dd22712..65b8735 100644
--- a/usb_linux.cpp
+++ b/usb_linux.cpp
@@ -572,6 +572,7 @@
}
static void* device_poll_thread(void* unused) {
+ adb_thread_setname("device poll");
D("Created device thread\n");
while (true) {
// TODO: Use inotify.
@@ -591,6 +592,6 @@
sigaction(SIGALRM, &actions, nullptr);
if (!adb_thread_create(device_poll_thread, nullptr)) {
- fatal_errno("cannot create input thread");
+ fatal_errno("cannot create device_poll thread");
}
}
diff --git a/usb_linux_client.cpp b/usb_linux_client.cpp
index dc44f16..e1d7594 100644
--- a/usb_linux_client.cpp
+++ b/usb_linux_client.cpp
@@ -209,6 +209,8 @@
struct usb_handle *usb = (struct usb_handle *)x;
int fd;
+ adb_thread_setname("usb open");
+
while (true) {
// wait until the USB device needs opening
adb_mutex_lock(&usb->lock);
@@ -403,6 +405,8 @@
{
struct usb_handle *usb = (struct usb_handle *)x;
+ adb_thread_setname("usb ffs open");
+
while (true) {
// wait until the USB device needs opening
adb_mutex_lock(&usb->lock);
diff --git a/usb_osx.cpp b/usb_osx.cpp
index 0ce85f3..8037606 100644
--- a/usb_osx.cpp
+++ b/usb_osx.cpp
@@ -403,6 +403,7 @@
void* RunLoopThread(void* unused)
{
+ adb_thread_setname("RunLoop");
InitUSB();
currentRunLoop = CFRunLoopGetCurrent();
@@ -438,7 +439,7 @@
adb_cond_init(&start_cond, NULL);
if (!adb_thread_create(RunLoopThread, nullptr)) {
- fatal_errno("cannot create input thread");
+ fatal_errno("cannot create RunLoop thread");
}
// Wait for initialization to finish
diff --git a/usb_windows.cpp b/usb_windows.cpp
index b8cc5cf..ab36475 100644
--- a/usb_windows.cpp
+++ b/usb_windows.cpp
@@ -171,6 +171,7 @@
}
void* device_poll_thread(void* unused) {
+ adb_thread_setname("Device Poll");
D("Created device thread\n");
while(1) {
@@ -208,6 +209,7 @@
// of a developer's interactive session, a window message pump is more
// appropriate.
D("Created power notification thread\n");
+ adb_thread_setname("Power Notifier");
// Window class names are process specific.
static const WCHAR kPowerNotificationWindowClassName[] =