From c7915a3470292349017f94ca066ed515babfcc23 Mon Sep 17 00:00:00 2001 From: Dan Albert Date: Mon, 18 May 2015 16:46:31 -0700 Subject: [PATCH] Make atransport be a real class. Using non-POD types in atransport means we'll need to start treating it as a real class (specifically with regards to new/delete rather than malloc/free). I've also cleaned up the home grown linked lists for transport_list and pending_list to just be std::lists. We might want to refactor that again to be an std::unordered_map keyed on serial, since that seems to be a common way to search it. Change-Id: I7f5e23cdc47944a9278099723ca029585fe52105 --- adb/Android.mk | 2 +- adb/adb.h | 103 +++++++++++++++------------ adb/transport.cpp | 154 ++++++++++++++++------------------------- adb/transport_test.cpp | 65 +++++++++++++++-- 4 files changed, 177 insertions(+), 147 deletions(-) diff --git a/adb/Android.mk b/adb/Android.mk index f3a482283..4d5ae14e5 100644 --- a/adb/Android.mk +++ b/adb/Android.mk @@ -16,6 +16,7 @@ adb_version := $(shell git -C $(LOCAL_PATH) rev-parse --short=12 HEAD 2>/dev/nul ADB_COMMON_CFLAGS := \ -Wall -Werror \ -Wno-unused-parameter \ + -Wno-missing-field-initializers \ -DADB_REVISION='"$(adb_version)"' \ # libadb @@ -45,7 +46,6 @@ LIBADB_TEST_SRCS := \ LIBADB_CFLAGS := \ $(ADB_COMMON_CFLAGS) \ - -Wno-missing-field-initializers \ -fvisibility=hidden \ LIBADB_darwin_SRC_FILES := \ diff --git a/adb/adb.h b/adb/adb.h index e8960e3b6..1be83d7ae 100644 --- a/adb/adb.h +++ b/adb/adb.h @@ -20,6 +20,8 @@ #include #include +#include + #include "adb_trace.h" #include "fdevent.h" @@ -43,7 +45,7 @@ // Increment this when we want to force users to start a new adb server. #define ADB_SERVER_VERSION 32 -struct atransport; +class atransport; struct usb_handle; struct amessage { @@ -152,20 +154,19 @@ struct adisconnect }; -/* a transport object models the connection to a remote device or emulator -** there is one transport per connected device/emulator. a "local transport" -** connects through TCP (for the emulator), while a "usb transport" through -** USB (for real devices) -** -** note that kTransportHost doesn't really correspond to a real transport -** object, it's a special value used to indicate that a client wants to -** connect to a service implemented within the ADB server itself. -*/ +// A transport object models the connection to a remote device or emulator there +// is one transport per connected device/emulator. A "local transport" connects +// through TCP (for the emulator), while a "usb transport" through USB (for real +// devices). +// +// Note that kTransportHost doesn't really correspond to a real transport +// object, it's a special value used to indicate that a client wants to connect +// to a service implemented within the ADB server itself. enum TransportType { - kTransportUsb, - kTransportLocal, - kTransportAny, - kTransportHost, + kTransportUsb, + kTransportLocal, + kTransportAny, + kTransportHost, }; #define TOKEN_SIZE 20 @@ -182,47 +183,59 @@ enum ConnectionState { kCsUnauthorized, }; -struct atransport -{ - atransport *next; - atransport *prev; +class atransport { +public: + // TODO(danalbert): We expose waaaaaaay too much stuff because this was + // historically just a struct, but making the whole thing a more idiomatic + // class in one go is a very large change. Given how bad our testing is, + // it's better to do this piece by piece. - int (*read_from_remote)(apacket *p, atransport *t); - int (*write_to_remote)(apacket *p, atransport *t); - void (*close)(atransport *t); - void (*kick)(atransport *t); + atransport() { + auth_fde = {}; + transport_fde = {}; + } - int fd; - int transport_socket; + virtual ~atransport() {} + + int (*read_from_remote)(apacket* p, atransport* t) = nullptr; + int (*write_to_remote)(apacket* p, atransport* t) = nullptr; + void (*close)(atransport* t) = nullptr; + void (*kick)(atransport* t) = nullptr; + + int fd = -1; + int transport_socket = -1; fdevent transport_fde; - int ref_count; - unsigned sync_token; - int connection_state; - int online; - TransportType type; + int ref_count = 0; + uint32_t sync_token = 0; + ConnectionState connection_state = kCsOffline; + bool online = false; + TransportType type = kTransportAny; - /* usb handle or socket fd as needed */ - usb_handle *usb; - int sfd; + // USB handle or socket fd as needed. + usb_handle* usb = nullptr; + int sfd = -1; - /* used to identify transports for clients */ - char *serial; - char *product; - char *model; - char *device; - char *devpath; - int adb_port; // Use for emulators (local transport) + // Used to identify transports for clients. + char* serial = nullptr; + char* product = nullptr; + char* model = nullptr; + char* device = nullptr; + char* devpath = nullptr; + int adb_port = -1; // Use for emulators (local transport) + bool kicked = false; - /* a list of adisconnect callbacks called when the transport is kicked */ - int kicked; - adisconnect disconnects; + // A list of adisconnect callbacks called when the transport is kicked. + adisconnect disconnects = {}; - void *key; - unsigned char token[TOKEN_SIZE]; + void* key = nullptr; + unsigned char token[TOKEN_SIZE] = {}; fdevent auth_fde; - unsigned failed_auth_attempts; + size_t failed_auth_attempts = 0; const char* connection_state_name() const; + +private: + DISALLOW_COPY_AND_ASSIGN(atransport); }; diff --git a/adb/transport.cpp b/adb/transport.cpp index 818ed97a4..379c702e8 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -26,6 +26,8 @@ #include #include +#include + #include #include "adb.h" @@ -33,15 +35,8 @@ static void transport_unref(atransport *t); -static atransport transport_list = { - .next = &transport_list, - .prev = &transport_list, -}; - -static atransport pending_list = { - .next = &pending_list, - .prev = &pending_list, -}; +static std::list transport_list; +static std::list pending_list; ADB_MUTEX_DEFINE( transport_lock ); @@ -553,8 +548,7 @@ static void transport_registration_func(int _fd, unsigned ev, void *data) adb_close(t->fd); adb_mutex_lock(&transport_lock); - t->next->prev = t->prev; - t->prev->next = t->next; + transport_list.remove(t); adb_mutex_unlock(&transport_lock); run_transport_disconnects(t); @@ -570,8 +564,7 @@ static void transport_registration_func(int _fd, unsigned ev, void *data) if (t->devpath) free(t->devpath); - memset(t,0xee,sizeof(atransport)); - free(t); + delete t; update_transports(); return; @@ -582,7 +575,7 @@ static void transport_registration_func(int _fd, unsigned ev, void *data) /* initial references are the two threads */ t->ref_count = 2; - if(adb_socketpair(s)) { + if (adb_socketpair(s)) { fatal_errno("cannot open transport socketpair"); } @@ -608,14 +601,8 @@ static void transport_registration_func(int _fd, unsigned ev, void *data) } adb_mutex_lock(&transport_lock); - /* remove from pending list */ - t->next->prev = t->prev; - t->prev->next = t->next; - /* put us on the master device list */ - t->next = &transport_list; - t->prev = transport_list.prev; - t->next->prev = t; - t->prev->next = t; + pending_list.remove(t); + transport_list.push_front(t); adb_mutex_unlock(&transport_lock); t->disconnects.next = t->disconnects.prev = &t->disconnects; @@ -740,7 +727,6 @@ static int qual_match(const char *to_test, atransport* acquire_one_transport(ConnectionState state, TransportType type, const char* serial, std::string* error_out) { - atransport *t; atransport *result = NULL; int ambiguous = 0; @@ -748,7 +734,7 @@ retry: if (error_out) *error_out = android::base::StringPrintf("device '%s' not found", serial); adb_mutex_lock(&transport_lock); - for (t = transport_list.next; t != &transport_list; t = t->next) { + for (auto t : transport_list) { if (t->connection_state == kCsNoPerm) { if (error_out) *error_out = "insufficient permissions for device"; continue; @@ -866,7 +852,8 @@ static void append_transport_info(std::string* result, const char* key, } } -static void append_transport(atransport* t, std::string* result, bool long_listing) { +static void append_transport(const atransport* t, std::string* result, + bool long_listing) { const char* serial = t->serial; if (!serial || !serial[0]) { serial = "(no serial number)"; @@ -890,7 +877,7 @@ static void append_transport(atransport* t, std::string* result, bool long_listi std::string list_transports(bool long_listing) { std::string result; adb_mutex_lock(&transport_lock); - for (atransport* t = transport_list.next; t != &transport_list; t = t->next) { + for (const auto t : transport_list) { append_transport(t, &result, long_listing); } adb_mutex_unlock(&transport_lock); @@ -898,11 +885,10 @@ std::string list_transports(bool long_listing) { } /* hack for osx */ -void close_usb_devices() -{ +void close_usb_devices() { adb_mutex_lock(&transport_lock); - for (atransport* t = transport_list.next; t != &transport_list; t = t->next) { - if ( !t->kicked ) { + for (auto t : transport_list) { + if (!t->kicked) { t->kicked = 1; t->kick(t); } @@ -911,47 +897,39 @@ void close_usb_devices() } #endif // ADB_HOST -int register_socket_transport(int s, const char *serial, int port, int local) -{ - atransport *t = reinterpret_cast(calloc(1, sizeof(atransport))); - if (t == nullptr) { - return -1; - } - - atransport *n; - char buff[32]; +int register_socket_transport(int s, const char *serial, int port, int local) { + atransport* t = new atransport(); if (!serial) { - snprintf(buff, sizeof buff, "T-%p", t); - serial = buff; + char buf[32]; + snprintf(buf, sizeof(buf), "T-%p", t); + serial = buf; } + D("transport: %s init'ing for socket %d, on port %d\n", serial, s, port); if (init_socket_transport(t, s, port, local) < 0) { - free(t); + delete t; return -1; } adb_mutex_lock(&transport_lock); - for (n = pending_list.next; n != &pending_list; n = n->next) { - if (n->serial && !strcmp(serial, n->serial)) { + for (auto transport : pending_list) { + if (transport->serial && strcmp(serial, transport->serial) == 0) { adb_mutex_unlock(&transport_lock); - free(t); + delete t; return -1; } } - for (n = transport_list.next; n != &transport_list; n = n->next) { - if (n->serial && !strcmp(serial, n->serial)) { + for (auto transport : transport_list) { + if (transport->serial && strcmp(serial, transport->serial) == 0) { adb_mutex_unlock(&transport_lock); - free(t); + delete t; return -1; } } - t->next = &pending_list; - t->prev = pending_list.prev; - t->next->prev = t; - t->prev->next = t; + pending_list.push_front(t); t->serial = strdup(serial); adb_mutex_unlock(&transport_lock); @@ -960,95 +938,83 @@ int register_socket_transport(int s, const char *serial, int port, int local) } #if ADB_HOST -atransport *find_transport(const char *serial) -{ - atransport *t; +atransport *find_transport(const char *serial) { + atransport* result = nullptr; adb_mutex_lock(&transport_lock); - for(t = transport_list.next; t != &transport_list; t = t->next) { - if (t->serial && !strcmp(serial, t->serial)) { + for (auto t : transport_list) { + if (t->serial && strcmp(serial, t->serial) == 0) { + result = t; break; } - } + } adb_mutex_unlock(&transport_lock); - if (t != &transport_list) - return t; - else - return 0; + return result; } void unregister_transport(atransport *t) { adb_mutex_lock(&transport_lock); - t->next->prev = t->prev; - t->prev->next = t->next; + transport_list.remove(t); adb_mutex_unlock(&transport_lock); kick_transport(t); transport_unref(t); } -// unregisters all non-emulator TCP transports -void unregister_all_tcp_transports() -{ - atransport *t, *next; +// Unregisters all non-emulator TCP transports. +void unregister_all_tcp_transports() { adb_mutex_lock(&transport_lock); - for (t = transport_list.next; t != &transport_list; t = next) { - next = t->next; + for (auto it = transport_list.begin(); it != transport_list.end(); ) { + atransport* t = *it; if (t->type == kTransportLocal && t->adb_port == 0) { - t->next->prev = t->prev; - t->prev->next = next; - // we cannot call kick_transport when holding transport_lock - if (!t->kicked) - { + // We cannot call kick_transport when holding transport_lock. + if (!t->kicked) { t->kicked = 1; t->kick(t); } transport_unref_locked(t); + + it = transport_list.erase(it); + } else { + ++it; } - } + } adb_mutex_unlock(&transport_lock); } #endif -void register_usb_transport(usb_handle *usb, const char *serial, const char *devpath, unsigned writeable) -{ - atransport *t = reinterpret_cast(calloc(1, sizeof(atransport))); - if (t == nullptr) fatal("cannot allocate USB atransport"); +void register_usb_transport(usb_handle* usb, const char* serial, + const char* devpath, unsigned writeable) { + atransport* t = new atransport(); + D("transport: %p init'ing for usb_handle %p (sn='%s')\n", t, usb, serial ? serial : ""); init_usb_transport(t, usb, (writeable ? kCsOffline : kCsNoPerm)); if(serial) { t->serial = strdup(serial); } - if(devpath) { + + if (devpath) { t->devpath = strdup(devpath); } adb_mutex_lock(&transport_lock); - t->next = &pending_list; - t->prev = pending_list.prev; - t->next->prev = t; - t->prev->next = t; + pending_list.push_front(t); adb_mutex_unlock(&transport_lock); register_transport(t); } // This should only be used for transports with connection_state == kCsNoPerm. -void unregister_usb_transport(usb_handle* usb) { +void unregister_usb_transport(usb_handle *usb) { adb_mutex_lock(&transport_lock); - for (atransport* t = transport_list.next; t != &transport_list; - t = t->next) { - if (t->usb == usb && t->connection_state == kCsNoPerm) { - t->next->prev = t->prev; - t->prev->next = t->next; - break; - } - } + transport_list.remove_if([usb](atransport* t) { + return t->usb == usb && t->connection_state == kCsNoPerm; + }); adb_mutex_unlock(&transport_lock); } diff --git a/adb/transport_test.cpp b/adb/transport_test.cpp index 2b3fe3c3e..4b74adf92 100644 --- a/adb/transport_test.cpp +++ b/adb/transport_test.cpp @@ -20,34 +20,85 @@ #include "adb.h" +class TestTransport : public atransport { +public: + bool operator==(const atransport& rhs) const { + EXPECT_EQ(read_from_remote, rhs.read_from_remote); + EXPECT_EQ(write_to_remote, rhs.write_to_remote); + EXPECT_EQ(close, rhs.close); + EXPECT_EQ(kick, rhs.kick); + + EXPECT_EQ(fd, rhs.fd); + EXPECT_EQ(transport_socket, rhs.transport_socket); + + EXPECT_EQ( + 0, memcmp(&transport_fde, &rhs.transport_fde, sizeof(fdevent))); + + EXPECT_EQ(ref_count, rhs.ref_count); + EXPECT_EQ(sync_token, rhs.sync_token); + EXPECT_EQ(connection_state, rhs.connection_state); + EXPECT_EQ(online, rhs.online); + EXPECT_EQ(type, rhs.type); + + EXPECT_EQ(usb, rhs.usb); + EXPECT_EQ(sfd, rhs.sfd); + + EXPECT_EQ(serial, rhs.serial); + EXPECT_EQ(product, rhs.product); + EXPECT_EQ(model, rhs.model); + EXPECT_EQ(device, rhs.device); + EXPECT_EQ(devpath, rhs.devpath); + 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))); + EXPECT_EQ(failed_auth_attempts, rhs.failed_auth_attempts); + + return true; + } +}; + TEST(transport, kick_transport) { - atransport t = {}; + TestTransport t; + // Mutate some member so we can test that the function is run. t.kick = [](atransport* trans) { trans->fd = 42; }; - atransport expected = t; + + TestTransport expected; + expected.kick = t.kick; expected.fd = 42; expected.kicked = 1; + kick_transport(&t); ASSERT_EQ(42, t.fd); ASSERT_EQ(1, t.kicked); - ASSERT_EQ(0, memcmp(&expected, &t, sizeof(atransport))); + ASSERT_EQ(expected, t); } TEST(transport, kick_transport_already_kicked) { // Ensure that the transport is not modified if the transport has already been // kicked. - atransport t = {}; + TestTransport t; t.kicked = 1; t.kick = [](atransport*) { FAIL() << "Kick should not have been called"; }; - atransport expected = t; + + TestTransport expected; + expected.kicked = 1; + expected.kick = t.kick; + kick_transport(&t); - ASSERT_EQ(0, memcmp(&expected, &t, sizeof(atransport))); + 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) { - atransport t = {}; + atransport t; run_transport_disconnects(&t); }