From 5d3958525543d4be02c4530dba9aab964623755d Mon Sep 17 00:00:00 2001 From: Luis Hector Chavez Date: Tue, 17 Apr 2018 14:09:21 -0700 Subject: [PATCH 1/2] adb: Distinguish betwen already-connected and connection failure This change returns a different value (-EALREADY) when a connection has already been established, as opposed to a real connection failure (which still returns -1). Bug: 74411879 Test: Opened a socket, tried to adb connect to it, got "failed to connect to localhost:1337" Change-Id: Ic216ddef7f28eb43ca750f9e51d068c077d54b07 --- adb/transport.cpp | 4 ++-- adb/transport_local.cpp | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/adb/transport.cpp b/adb/transport.cpp index 92c52e23c..f5f6d2670 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -974,7 +974,7 @@ int register_socket_transport(int s, const char* serial, int port, int local) { VLOG(TRANSPORT) << "socket transport " << transport->serial << " is already in pending_list and fails to register"; delete t; - return -1; + return -EALREADY; } } @@ -983,7 +983,7 @@ int register_socket_transport(int s, const char* serial, int port, int local) { VLOG(TRANSPORT) << "socket transport " << transport->serial << " is already in transport_list and fails to register"; delete t; - return -1; + return -EALREADY; } } diff --git a/adb/transport_local.cpp b/adb/transport_local.cpp index c09fcb76d..8032421b1 100644 --- a/adb/transport_local.cpp +++ b/adb/transport_local.cpp @@ -101,7 +101,11 @@ void connect_device(const std::string& address, std::string* response) { int ret = register_socket_transport(fd, serial.c_str(), port, 0); if (ret < 0) { adb_close(fd); - *response = android::base::StringPrintf("already connected to %s", serial.c_str()); + if (ret == -EALREADY) { + *response = android::base::StringPrintf("already connected to %s", serial.c_str()); + } else { + *response = android::base::StringPrintf("failed to connect to %s", serial.c_str()); + } } else { *response = android::base::StringPrintf("connected to %s", serial.c_str()); } From 8b67c520998c6fca95c62fdd864e42647cb8da49 Mon Sep 17 00:00:00 2001 From: Luis Hector Chavez Date: Tue, 17 Apr 2018 19:25:33 -0700 Subject: [PATCH 2/2] Improve test_adb.py This change uses a context manager to create the fake ADB servers (and cleanly tear them down. Bug: 74411879 Test: python system/core/adb/test_adb.py Change-Id: I722d2c4135259b1b0ef00a1510aa8402e87ecf72 --- adb/test_adb.py | 95 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 65 insertions(+), 30 deletions(-) diff --git a/adb/test_adb.py b/adb/test_adb.py index 3bb433ddd..363002f8d 100644 --- a/adb/test_adb.py +++ b/adb/test_adb.py @@ -21,9 +21,11 @@ things. Most of these tests involve specific error messages or the help text. """ from __future__ import print_function +import binascii import contextlib import os import random +import select import socket import struct import subprocess @@ -33,6 +35,52 @@ import unittest import adb +@contextlib.contextmanager +def fake_adb_server(protocol=socket.AF_INET, port=0): + """Creates a fake ADB server that just replies with a CNXN packet.""" + + serversock = socket.socket(protocol, socket.SOCK_STREAM) + if protocol == socket.AF_INET: + serversock.bind(('127.0.0.1', port)) + else: + serversock.bind(('::1', port)) + serversock.listen(1) + + # A pipe that is used to signal the thread that it should terminate. + readpipe, writepipe = os.pipe() + + def _handle(): + rlist = [readpipe, serversock] + while True: + ready, _, _ = select.select(rlist, [], []) + for r in ready: + if r == readpipe: + # Closure pipe + os.close(r) + serversock.shutdown(socket.SHUT_RDWR) + serversock.close() + return + elif r == serversock: + # Server socket + conn, _ = r.accept() + rlist.append(conn) + else: + # Client socket + data = r.recv(1024) + if not data: + rlist.remove(r) + + port = serversock.getsockname()[1] + server_thread = threading.Thread(target=_handle) + server_thread.start() + + try: + yield port + finally: + os.close(writepipe) + server_thread.join() + + class NonApiTest(unittest.TestCase): """Tests for ADB that aren't a part of the AndroidDevice API.""" @@ -211,45 +259,32 @@ class NonApiTest(unittest.TestCase): Bug: http://b/30313466 """ - ipv4 = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - ipv4.bind(('127.0.0.1', 0)) - ipv4.listen(1) + for protocol in (socket.AF_INET, socket.AF_INET6): + try: + with fake_adb_server(protocol=protocol) as port: + output = subprocess.check_output( + ['adb', 'connect', 'localhost:{}'.format(port)]) - ipv6 = socket.socket(socket.AF_INET6, socket.SOCK_STREAM) - try: - ipv6.bind(('::1', ipv4.getsockname()[1] + 1)) - ipv6.listen(1) - except socket.error: - print("IPv6 not available, skipping") - return + self.assertEqual( + output.strip(), 'connected to localhost:{}'.format(port)) + except socket.error: + print("IPv6 not available, skipping") + continue - for s in (ipv4, ipv6): - port = s.getsockname()[1] + def test_already_connected(self): + with fake_adb_server() as port: output = subprocess.check_output( ['adb', 'connect', 'localhost:{}'.format(port)]) self.assertEqual( output.strip(), 'connected to localhost:{}'.format(port)) - s.close() - def test_already_connected(self): - s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - s.bind(('127.0.0.1', 0)) - s.listen(2) + # b/31250450: this always returns 0 but probably shouldn't. + output = subprocess.check_output( + ['adb', 'connect', 'localhost:{}'.format(port)]) - port = s.getsockname()[1] - output = subprocess.check_output( - ['adb', 'connect', 'localhost:{}'.format(port)]) - - self.assertEqual( - output.strip(), 'connected to localhost:{}'.format(port)) - - # b/31250450: this always returns 0 but probably shouldn't. - output = subprocess.check_output( - ['adb', 'connect', 'localhost:{}'.format(port)]) - - self.assertEqual( - output.strip(), 'already connected to localhost:{}'.format(port)) + self.assertEqual( + output.strip(), 'already connected to localhost:{}'.format(port)) def main(): random.seed(0)