Merge "Make atransport be a real class."
diff --git a/adb/Android.mk b/adb/Android.mk
index 8d05a2e..bb974d8 100644
--- a/adb/Android.mk
+++ b/adb/Android.mk
@@ -16,6 +16,7 @@
ADB_COMMON_CFLAGS := \
-Wall -Wextra -Werror \
-Wno-unused-parameter \
+ -Wno-missing-field-initializers \
-DADB_REVISION='"$(adb_version)"' \
# libadb
@@ -45,7 +46,6 @@
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 e8960e3..1be83d7 100644
--- a/adb/adb.h
+++ b/adb/adb.h
@@ -20,6 +20,8 @@
#include <limits.h>
#include <sys/types.h>
+#include <base/macros.h>
+
#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 @@
};
-/* 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 @@
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 818ed97..379c702 100644
--- a/adb/transport.cpp
+++ b/adb/transport.cpp
@@ -26,6 +26,8 @@
#include <string.h>
#include <unistd.h>
+#include <list>
+
#include <base/stringprintf.h>
#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<atransport*> transport_list;
+static std::list<atransport*> pending_list;
ADB_MUTEX_DEFINE( transport_lock );
@@ -553,8 +548,7 @@
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 @@
if (t->devpath)
free(t->devpath);
- memset(t,0xee,sizeof(atransport));
- free(t);
+ delete t;
update_transports();
return;
@@ -582,7 +575,7 @@
/* 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 @@
}
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 @@
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 @@
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(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 @@
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 @@
}
/* 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 @@
}
#endif // ADB_HOST
-int register_socket_transport(int s, const char *serial, int port, int local)
-{
- atransport *t = reinterpret_cast<atransport*>(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 @@
}
#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<atransport*>(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 2b3fe3c..4b74adf 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);
}
