From 4039051d6ddb02324204930fb86d9d0fe405b3fb Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 7 Aug 2018 14:14:21 -0700 Subject: [PATCH] adb: clean up handle_host_request. Previously, we were returning the result of SendOkay/SendFail in a few places after handling a host request, which is incorrect for two reasons. First, the return type of SendOkay/SendFail is bool, and handle_host_request was expected to return 0 on success. Second, we don't care if the SendOkay fails; if we got to that point, we're done with the request, regardless of whether we succeeded to report our result. The result of this was a spurious failure result reported after the initial result, which was ignored by the adb client. Test: manually straced adb server Test: python test_adb.py Test: python test_device.py Change-Id: I7d45ba527e1faccbbae5b15e7a0d1557b0a84858 --- adb/adb.cpp | 61 +++++++++++++++++++++++++++++-------------------- adb/adb.h | 4 ++-- adb/sockets.cpp | 10 +++----- 3 files changed, 41 insertions(+), 34 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index a8fe73651..8e028f422 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -1044,7 +1044,7 @@ static int SendOkay(int fd, const std::string& s) { return 0; } -int handle_host_request(const char* service, TransportType type, const char* serial, +bool handle_host_request(const char* service, TransportType type, const char* serial, TransportId transport_id, int reply_fd, asocket* s) { if (strcmp(service, "kill") == 0) { fprintf(stderr, "adb server killed by remote request\n"); @@ -1070,7 +1070,7 @@ int handle_host_request(const char* service, TransportType type, const char* ser transport_id = strtoll(service, const_cast(&service), 10); if (*service != '\0') { SendFail(reply_fd, "invalid transport id"); - return 1; + return true; } } else if (!strncmp(service, "transport-usb", strlen("transport-usb"))) { type = kTransportUsb; @@ -1088,10 +1088,13 @@ int handle_host_request(const char* service, TransportType type, const char* ser if (t != nullptr) { s->transport = t; SendOkay(reply_fd); + + // We succesfully handled the device selection, but there's another request coming. + return false; } else { SendFail(reply_fd, error); + return true; } - return 1; } // return a list of all connected devices @@ -1101,9 +1104,9 @@ int handle_host_request(const char* service, TransportType type, const char* ser D("Getting device list..."); std::string device_list = list_transports(long_listing); D("Sending device list..."); - return SendOkay(reply_fd, device_list); + SendOkay(reply_fd, device_list); } - return 1; + return true; } if (!strcmp(service, "reconnect-offline")) { @@ -1119,7 +1122,7 @@ int handle_host_request(const char* service, TransportType type, const char* ser response.resize(response.size() - 1); } SendOkay(reply_fd, response); - return 0; + return true; } if (!strcmp(service, "features")) { @@ -1130,7 +1133,7 @@ int handle_host_request(const char* service, TransportType type, const char* ser } else { SendFail(reply_fd, error); } - return 0; + return true; } if (!strcmp(service, "host-features")) { @@ -1141,7 +1144,7 @@ int handle_host_request(const char* service, TransportType type, const char* ser } features.insert(kFeaturePushSync); SendOkay(reply_fd, FeatureSetToString(features)); - return 0; + return true; } // remove TCP transport @@ -1149,7 +1152,8 @@ int handle_host_request(const char* service, TransportType type, const char* ser const std::string address(service + 11); if (address.empty()) { kick_all_tcp_devices(); - return SendOkay(reply_fd, "disconnected everything"); + SendOkay(reply_fd, "disconnected everything"); + return true; } std::string serial; @@ -1157,21 +1161,24 @@ int handle_host_request(const char* service, TransportType type, const char* ser int port = DEFAULT_ADB_LOCAL_TRANSPORT_PORT; std::string error; if (!android::base::ParseNetAddress(address, &host, &port, &serial, &error)) { - return SendFail(reply_fd, android::base::StringPrintf("couldn't parse '%s': %s", - address.c_str(), error.c_str())); + SendFail(reply_fd, android::base::StringPrintf("couldn't parse '%s': %s", + address.c_str(), error.c_str())); + return true; } atransport* t = find_transport(serial.c_str()); if (t == nullptr) { - return SendFail(reply_fd, android::base::StringPrintf("no such device '%s'", - serial.c_str())); + SendFail(reply_fd, android::base::StringPrintf("no such device '%s'", serial.c_str())); + return true; } kick_transport(t); - return SendOkay(reply_fd, android::base::StringPrintf("disconnected %s", address.c_str())); + SendOkay(reply_fd, android::base::StringPrintf("disconnected %s", address.c_str())); + return true; } // Returns our value for ADB_SERVER_VERSION. if (!strcmp(service, "version")) { - return SendOkay(reply_fd, android::base::StringPrintf("%04x", ADB_SERVER_VERSION)); + SendOkay(reply_fd, android::base::StringPrintf("%04x", ADB_SERVER_VERSION)); + return true; } // These always report "unknown" rather than the actual error, for scripts. @@ -1179,28 +1186,31 @@ int handle_host_request(const char* service, TransportType type, const char* ser std::string error; atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error); if (t) { - return SendOkay(reply_fd, !t->serial.empty() ? t->serial : "unknown"); + SendOkay(reply_fd, !t->serial.empty() ? t->serial : "unknown"); } else { - return SendFail(reply_fd, error); + SendFail(reply_fd, error); } + return true; } if (!strcmp(service, "get-devpath")) { std::string error; atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error); if (t) { - return SendOkay(reply_fd, !t->devpath.empty() ? t->devpath : "unknown"); + SendOkay(reply_fd, !t->devpath.empty() ? t->devpath : "unknown"); } else { - return SendFail(reply_fd, error); + SendFail(reply_fd, error); } + return true; } if (!strcmp(service, "get-state")) { std::string error; atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error); if (t) { - return SendOkay(reply_fd, t->connection_state_name()); + SendOkay(reply_fd, t->connection_state_name()); } else { - return SendFail(reply_fd, error); + SendFail(reply_fd, error); } + return true; } // Indicates a new emulator instance has started. @@ -1208,7 +1218,7 @@ int handle_host_request(const char* service, TransportType type, const char* ser int port = atoi(service+9); local_connect(port); /* we don't even need to send a reply */ - return 0; + return true; } if (!strcmp(service, "reconnect")) { @@ -1219,7 +1229,8 @@ int handle_host_request(const char* service, TransportType type, const char* ser response = "reconnecting " + t->serial_name() + " [" + t->connection_state_name() + "]\n"; } - return SendOkay(reply_fd, response); + SendOkay(reply_fd, response); + return true; } if (handle_forward_request(service, @@ -1228,10 +1239,10 @@ int handle_host_request(const char* service, TransportType type, const char* ser error); }, reply_fd)) { - return 0; + return true; } - return -1; + return false; } static auto& init_mutex = *new std::mutex(); diff --git a/adb/adb.h b/adb/adb.h index e6af780c4..f434e2d18 100644 --- a/adb/adb.h +++ b/adb/adb.h @@ -210,8 +210,8 @@ extern const char* adb_device_banner; #define USB_FFS_ADB_IN USB_FFS_ADB_EP(ep2) #endif -int handle_host_request(const char* service, TransportType type, const char* serial, - TransportId transport_id, int reply_fd, asocket* s); +bool handle_host_request(const char* service, TransportType type, const char* serial, + TransportId transport_id, int reply_fd, asocket* s); void handle_online(atransport* t); void handle_offline(atransport* t); diff --git a/adb/sockets.cpp b/adb/sockets.cpp index 69b518066..15347929a 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -685,13 +685,9 @@ static int smart_socket_enqueue(asocket* s, apacket::payload_type data) { if (service) { asocket* s2; - /* some requests are handled immediately -- in that - ** case the handle_host_request() routine has sent - ** the OKAY or FAIL message and all we have to do - ** is clean up. - */ - if (handle_host_request(service, type, serial, transport_id, s->peer->fd, s) == 0) { - /* XXX fail message? */ + // Some requests are handled immediately -- in that case the handle_host_request() routine + // has sent the OKAY or FAIL message and all we have to do is clean up. + if (handle_host_request(service, type, serial, transport_id, s->peer->fd, s)) { D("SS(%d): handled host service '%s'", s->id, service); goto fail; }