Fix adb -d/-e error reporting.

If -d/-e fail, get-serialno and friends will now report an error
and return a failure status code on exit.

Also fix the behavior of -d/-e with $ANDROID_SERIAL --- -d/-e
should override $ANDROID_SERIAL, not the other way round.

I'm deleting my own comment here about always returning "unknown"
for scripts. I can't find any evidence that there are scripts
relying on that, so I think my comment meant "I fear that there
are scripts doing so".

Bug: http://b/24403699
Change-Id: Ie13a751f1137abcfe0cc6c46a0630ba5e02db676

7 files changed, 109 insertions, 91 deletions

diff --git a/adb.cpp b/adb.cpp
index 1c1683e..3dcf282 100644
--- a/adb.cpp
+++ b/adb.cpp
@@ -904,7 +904,7 @@ int handle_forward_request(const char* service, TransportType type, const char*
         }
 
         std::string error_msg;
-        atransport* transport = acquire_one_transport(kCsAny, type, serial, &error_msg);
+        atransport* transport = acquire_one_transport(type, serial, nullptr, &error_msg);
         if (!transport) {
             SendFail(reply_fd, error_msg);
             return 1;
@@ -990,13 +990,13 @@ int handle_host_request(const char* service, TransportType type,
             serial = service;
         }
 
-        std::string error_msg;
-        atransport* t = acquire_one_transport(kCsAny, type, serial, &error_msg);
+        std::string error;
+        atransport* t = acquire_one_transport(type, serial, nullptr, &error);
         if (t != nullptr) {
             s->transport = t;
             SendOkay(reply_fd);
         } else {
-            SendFail(reply_fd, error_msg);
+            SendFail(reply_fd, error);
         }
         return 1;
     }
@@ -1014,12 +1014,12 @@ int handle_host_request(const char* service, TransportType type,
     }
 
     if (!strcmp(service, "features")) {
-        std::string error_msg;
-        atransport* t = acquire_one_transport(kCsAny, type, serial, &error_msg);
+        std::string error;
+        atransport* t = acquire_one_transport(type, serial, nullptr, &error);
         if (t != nullptr) {
             SendOkay(reply_fd, FeatureSetToString(t->features()));
         } else {
-            SendFail(reply_fd, error_msg);
+            SendFail(reply_fd, error);
         }
         return 0;
     }
@@ -1049,29 +1049,41 @@ int handle_host_request(const char* service, TransportType type,
         return SendOkay(reply_fd, android::base::StringPrintf("disconnected %s", address.c_str()));
     }
 
-    // returns our value for ADB_SERVER_VERSION
+    // Returns our value for ADB_SERVER_VERSION.
     if (!strcmp(service, "version")) {
         return SendOkay(reply_fd, android::base::StringPrintf("%04x", ADB_SERVER_VERSION));
     }
 
     // These always report "unknown" rather than the actual error, for scripts.
     if (!strcmp(service, "get-serialno")) {
-        std::string ignored;
-        atransport* t = acquire_one_transport(kCsAny, type, serial, &ignored);
-        return SendOkay(reply_fd, (t && t->serial) ? t->serial : "unknown");
+        std::string error;
+        atransport* t = acquire_one_transport(type, serial, nullptr, &error);
+        if (t) {
+            return SendOkay(reply_fd, t->serial ? t->serial : "unknown");
+        } else {
+            return SendFail(reply_fd, error);
+        }
     }
     if (!strcmp(service, "get-devpath")) {
-        std::string ignored;
-        atransport* t = acquire_one_transport(kCsAny, type, serial, &ignored);
-        return SendOkay(reply_fd, (t && t->devpath) ? t->devpath : "unknown");
+        std::string error;
+        atransport* t = acquire_one_transport(type, serial, nullptr, &error);
+        if (t) {
+            return SendOkay(reply_fd, t->devpath ? t->devpath : "unknown");
+        } else {
+            return SendFail(reply_fd, error);
+        }
     }
     if (!strcmp(service, "get-state")) {
-        std::string ignored;
-        atransport* t = acquire_one_transport(kCsAny, type, serial, &ignored);
-        return SendOkay(reply_fd, t ? t->connection_state_name() : "unknown");
+        std::string error;
+        atransport* t = acquire_one_transport(type, serial, nullptr, &error);
+        if (t) {
+            return SendOkay(reply_fd, t->connection_state_name());
+        } else {
+            return SendFail(reply_fd, error);
+        }
     }
 
-    // indicates a new emulator instance has started
+    // Indicates a new emulator instance has started.
     if (!strncmp(service, "emulator:", 9)) {
         int  port = atoi(service+9);
         local_connect(port);
diff --git a/adb_client.cpp b/adb_client.cpp
index 984910d..fa8ce9e 100644
--- a/adb_client.cpp
+++ b/adb_client.cpp
@@ -209,9 +209,8 @@ int adb_connect(const std::string& service, std::string* error) {
             adb_close(fd);
 
             if (sscanf(&version_string[0], "%04x", &version) != 1) {
-                *error = android::base::StringPrintf(
-                        "cannot parse version string: %s",
-                        version_string.c_str());
+                *error = android::base::StringPrintf("cannot parse version string: %s",
+                                                     version_string.c_str());
                 return -1;
             }
         } else {
diff --git a/commandline.cpp b/commandline.cpp
index 5e5ca7f..0531cf9 100644
--- a/commandline.cpp
+++ b/commandline.cpp
@@ -1093,8 +1093,6 @@ int adb_commandline(int argc, const char **argv) {
     }
     // TODO: also try TARGET_PRODUCT/TARGET_DEVICE as a hint
 
-    const char* serial = getenv("ANDROID_SERIAL");
-
     /* Validate and assign the server port */
     const char* server_port_str = getenv("ANDROID_ADB_SERVER_PORT");
     int server_port = DEFAULT_ADB_PORT;
@@ -1108,7 +1106,9 @@ int adb_commandline(int argc, const char **argv) {
         }
     }
 
-    /* modifiers and flags */
+    // We need to check for -d and -e before we look at $ANDROID_SERIAL.
+    const char* serial = nullptr;
+
     while (argc > 0) {
         if (!strcmp(argv[0],"server")) {
             is_server = 1;
@@ -1199,6 +1199,11 @@ int adb_commandline(int argc, const char **argv) {
         argv++;
     }
 
+    // If none of -d, -e, or -s were specified, try $ANDROID_SERIAL.
+    if (transport_type == kTransportAny && serial == nullptr) {
+        serial = getenv("ANDROID_SERIAL");
+    }
+
     adb_set_transport(transport_type, serial);
     adb_set_tcp_specifics(server_port);
 
diff --git a/services.cpp b/services.cpp
index e832b1e..e24b470 100644
--- a/services.cpp
+++ b/services.cpp
@@ -363,23 +363,31 @@ struct state_info {
     ConnectionState state;
 };
 
-static void wait_for_state(int fd, void* cookie)
-{
+static void wait_for_state(int fd, void* cookie) {
     state_info* sinfo = reinterpret_cast<state_info*>(cookie);
 
     D("wait_for_state %d", sinfo->state);
 
-    std::string error_msg = "unknown error";
-    atransport* t = acquire_one_transport(sinfo->state, sinfo->transport_type, sinfo->serial,
-                                          &error_msg);
-    if (t != nullptr) {
-        SendOkay(fd);
-    } else {
-        SendFail(fd, error_msg);
+    while (true) {
+        bool is_ambiguous = false;
+        std::string error = "unknown error";
+        atransport* t = acquire_one_transport(sinfo->transport_type, sinfo->serial,
+                                              &is_ambiguous, &error);
+        if (t != nullptr && t->connection_state == sinfo->state) {
+            SendOkay(fd);
+            break;
+        } else if (!is_ambiguous) {
+            adb_sleep_ms(1000);
+            // Try again...
+        } else {
+            SendFail(fd, error);
+            break;
+        }
     }
 
-    if (sinfo->serial)
+    if (sinfo->serial) {
         free(sinfo->serial);
+    }
     free(sinfo);
     adb_close(fd);
     D("wait_for_state is done");
diff --git a/sockets.cpp b/sockets.cpp
index 8562496..f8c2f64 100644
--- a/sockets.cpp
+++ b/sockets.cpp
@@ -671,8 +671,8 @@ static int smart_socket_enqueue(asocket *s, apacket *p)
 {
     unsigned len;
 #if ADB_HOST
-    char *service = NULL;
-    char* serial = NULL;
+    char *service = nullptr;
+    char* serial = nullptr;
     TransportType type = kTransportAny;
 #endif
 
@@ -739,7 +739,7 @@ static int smart_socket_enqueue(asocket *s, apacket *p)
         type = kTransportAny;
         service += strlen("host:");
     } else {
-        service = NULL;
+        service = nullptr;
     }
 
     if (service) {
@@ -782,7 +782,7 @@ static int smart_socket_enqueue(asocket *s, apacket *p)
         SendOkay(s->peer->fd);
 
         s->peer->ready = local_socket_ready;
-        s->peer->shutdown = NULL;
+        s->peer->shutdown = nullptr;
         s->peer->close = local_socket_close;
         s->peer->peer = s2;
         s2->peer = s->peer;
@@ -795,12 +795,10 @@ static int smart_socket_enqueue(asocket *s, apacket *p)
         return 0;
     }
 #else /* !ADB_HOST */
-    if (s->transport == NULL) {
+    if (s->transport == nullptr) {
         std::string error_msg = "unknown failure";
-        s->transport =
-            acquire_one_transport(kCsAny, kTransportAny, NULL, &error_msg);
-
-        if (s->transport == NULL) {
+        s->transport = acquire_one_transport(kTransportAny, nullptr, nullptr, &error_msg);
+        if (s->transport == nullptr) {
             SendFail(s->peer->fd, error_msg);
             goto fail;
         }
@@ -822,7 +820,7 @@ static int smart_socket_enqueue(asocket *s, apacket *p)
         ** tear down
         */
     s->peer->ready = local_socket_ready_notify;
-    s->peer->shutdown = NULL;
+    s->peer->shutdown = nullptr;
     s->peer->close = local_socket_close_notify;
     s->peer->peer = 0;
         /* give him our transport and upref it */
diff --git a/transport.cpp b/transport.cpp
index 501a39a..f8cf63a 100644
--- a/transport.cpp
+++ b/transport.cpp
@@ -653,22 +653,28 @@ static int qual_match(const char *to_test,
     return !*to_test;
 }
 
-atransport* acquire_one_transport(ConnectionState state, TransportType type,
-                                  const char* serial, std::string* error_out) {
-    atransport *result = NULL;
-    int ambiguous = 0;
+atransport* acquire_one_transport(TransportType type, const char* serial,
+                                  bool* is_ambiguous, std::string* error_out) {
+    atransport* result = nullptr;
 
-retry:
-    *error_out = serial ? android::base::StringPrintf("device '%s' not found", serial) : "no devices found";
+    if (serial) {
+        *error_out = android::base::StringPrintf("device '%s' not found", serial);
+    } else if (type == kTransportLocal) {
+        *error_out = "no emulators found";
+    } else if (type == kTransportAny) {
+        *error_out = "no devices/emulators found";
+    } else {
+        *error_out = "no devices found";
+    }
 
     adb_mutex_lock(&transport_lock);
-    for (auto t : transport_list) {
+    for (const auto& t : transport_list) {
         if (t->connection_state == kCsNoPerm) {
             *error_out = "insufficient permissions for device";
             continue;
         }
 
-        /* check for matching serial number */
+        // Check for matching serial number.
         if (serial) {
             if ((t->serial && !strcmp(serial, t->serial)) ||
                 (t->devpath && !strcmp(serial, t->devpath)) ||
@@ -677,8 +683,8 @@ retry:
                 qual_match(serial, "device:", t->device, false)) {
                 if (result) {
                     *error_out = "more than one device";
-                    ambiguous = 1;
-                    result = NULL;
+                    if (is_ambiguous) *is_ambiguous = true;
+                    result = nullptr;
                     break;
                 }
                 result = t;
@@ -687,24 +693,24 @@ retry:
             if (type == kTransportUsb && t->type == kTransportUsb) {
                 if (result) {
                     *error_out = "more than one device";
-                    ambiguous = 1;
-                    result = NULL;
+                    if (is_ambiguous) *is_ambiguous = true;
+                    result = nullptr;
                     break;
                 }
                 result = t;
             } else if (type == kTransportLocal && t->type == kTransportLocal) {
                 if (result) {
                     *error_out = "more than one emulator";
-                    ambiguous = 1;
-                    result = NULL;
+                    if (is_ambiguous) *is_ambiguous = true;
+                    result = nullptr;
                     break;
                 }
                 result = t;
             } else if (type == kTransportAny) {
                 if (result) {
                     *error_out = "more than one device/emulator";
-                    ambiguous = 1;
-                    result = NULL;
+                    if (is_ambiguous) *is_ambiguous = true;
+                    result = nullptr;
                     break;
                 }
                 result = t;
@@ -713,37 +719,26 @@ retry:
     }
     adb_mutex_unlock(&transport_lock);
 
-    if (result) {
-        if (result->connection_state == kCsUnauthorized) {
-            *error_out = "device unauthorized.\n";
-            char* ADB_VENDOR_KEYS = getenv("ADB_VENDOR_KEYS");
-            *error_out += "This adb server's $ADB_VENDOR_KEYS is ";
-            *error_out += ADB_VENDOR_KEYS ? ADB_VENDOR_KEYS : "not set";
-            *error_out += "\n";
-            *error_out += "Try 'adb kill-server' if that seems wrong.\n";
-            *error_out += "Otherwise check for a confirmation dialog on your device.";
-            result = NULL;
-        }
-
-        /* offline devices are ignored -- they are either being born or dying */
-        if (result && result->connection_state == kCsOffline) {
-            *error_out = "device offline";
-            result = NULL;
-        }
+    // Don't return unauthorized devices; the caller can't do anything with them.
+    if (result && result->connection_state == kCsUnauthorized) {
+        *error_out = "device unauthorized.\n";
+        char* ADB_VENDOR_KEYS = getenv("ADB_VENDOR_KEYS");
+        *error_out += "This adb server's $ADB_VENDOR_KEYS is ";
+        *error_out += ADB_VENDOR_KEYS ? ADB_VENDOR_KEYS : "not set";
+        *error_out += "\n";
+        *error_out += "Try 'adb kill-server' if that seems wrong.\n";
+        *error_out += "Otherwise check for a confirmation dialog on your device.";
+        result = nullptr;
+    }
 
-        /* check for required connection state */
-        if (result && state != kCsAny && result->connection_state != state) {
-            *error_out = "invalid device state";
-            result = NULL;
-        }
+    // Don't return offline devices; the caller can't do anything with them.
+    if (result && result->connection_state == kCsOffline) {
+        *error_out = "device offline";
+        result = nullptr;
     }
 
     if (result) {
-        /* found one that we can take */
         *error_out = "success";
-    } else if (state != kCsAny && (serial || !ambiguous)) {
-        adb_sleep_ms(1000);
-        goto retry;
     }
 
     return result;
diff --git a/transport.h b/transport.h
index dfe8875..f41a8d4 100644
--- a/transport.h
+++ b/transport.h
@@ -122,12 +122,13 @@ private:
 
 /*
  * Obtain a transport from the available transports.
- * If state is != kCsAny, only transports in that state are considered.
- * If serial is non-NULL then only the device with that serial will be chosen.
- * If no suitable transport is found, error is set.
+ * If serial is non-null then only the device with that serial will be chosen.
+ * If multiple devices/emulators would match, *is_ambiguous (if non-null)
+ * is set to true and nullptr returned.
+ * If no suitable transport is found, error is set and nullptr returned.
  */
-atransport* acquire_one_transport(ConnectionState state, TransportType type,
-                                  const char* serial, std::string* error_out);
+atransport* acquire_one_transport(TransportType type, const char* serial,
+                                  bool* is_ambiguous, std::string* error_out);
 void kick_transport(atransport* t);
 void update_transports(void);