From f18fc0879be9cd506be6b1e051feb543b0746bdb Mon Sep 17 00:00:00 2001 From: Spencer Low Date: Tue, 11 Aug 2015 17:05:02 -0700 Subject: [PATCH] adb: start-server and kill-server error output - handle_host_request - When the host:kill command comes in, shutdown the socket before calling exit(). If we don't do this, the client will output error info even though everything is working ok. - adb_connect() - If we can't parse the version string, explain this in error output and don't goto error which would try to close an fd we already closed. - If host:kill doesn't work, output error info. Don't try to close already closed fd. - adb_main() - If writing the ACK somehow has an error, output error info (I doubt this will ever get hit). - adb_commandline() - Fix typo about max port number. - Make 'adb kill-server' and 'adb start-server' output any detailed error info. Change-Id: Id1a309cc1bf516f7f49bd332b34d30f148b406da Signed-off-by: Spencer Low --- adb/adb.cpp | 6 ++++++ adb/adb_client.cpp | 18 ++++++++++++++++-- adb/client/main.cpp | 4 +++- adb/commandline.cpp | 23 +++++++++++++++++++---- 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index 548ca5445..b83d16486 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -888,6 +888,12 @@ int handle_host_request(const char* service, TransportType type, fprintf(stderr, "adb server killed by remote request\n"); fflush(stdout); SendOkay(reply_fd); + + // At least on Windows, if we exit() without shutdown(SD_SEND) or + // closesocket(), the client's next recv() will error-out with + // WSAECONNRESET and they'll never read the OKAY. + adb_shutdown(reply_fd); + exit(0); } diff --git a/adb/adb_client.cpp b/adb/adb_client.cpp index afff2ef17..4bf647c48 100644 --- a/adb/adb_client.cpp +++ b/adb/adb_client.cpp @@ -197,6 +197,7 @@ int adb_connect(const std::string& service, std::string* error) { D("adb_connect: service %s\n", service.c_str()); if (fd == -2 && __adb_server_name) { fprintf(stderr,"** Cannot start server on remote host\n"); + // error is the original network connection error return fd; } else if (fd == -2) { fprintf(stdout,"* daemon not running. starting it now on port %d *\n", @@ -204,6 +205,10 @@ int adb_connect(const std::string& service, std::string* error) { start_server: if (launch_server(__adb_server_port)) { fprintf(stderr,"* failed to start daemon *\n"); + // launch_server() has already printed detailed error info, so just + // return a generic error string about the overall adb_connect() + // that the caller requested. + *error = "cannot connect to daemon"; return -1; } else { fprintf(stdout,"* daemon started successfully *\n"); @@ -225,7 +230,10 @@ int adb_connect(const std::string& service, std::string* error) { adb_close(fd); if (sscanf(&version_string[0], "%04x", &version) != 1) { - goto error; + *error = android::base::StringPrintf( + "cannot parse version string: %s", + version_string.c_str()); + return -1; } } else { // if fd is -1, then check for "unknown host service", @@ -239,7 +247,13 @@ int adb_connect(const std::string& service, std::string* error) { if (version != ADB_SERVER_VERSION) { printf("adb server is out of date. killing...\n"); fd = _adb_connect("host:kill", error); - adb_close(fd); + if (fd >= 0) { + adb_close(fd); + } else { + // If we couldn't connect to the server or had some other error, + // report it, but still try to start the server. + fprintf(stderr, "error: %s\n", error->c_str()); + } /* XXX can we better detect its death? */ adb_sleep_ms(2000); diff --git a/adb/client/main.cpp b/adb/client/main.cpp index fca1762bd..cd06f4b34 100644 --- a/adb/client/main.cpp +++ b/adb/client/main.cpp @@ -178,7 +178,9 @@ int adb_main(int is_daemon, int server_port, int ack_reply_fd) { #else // TODO(danalbert): Can't use SendOkay because we're sending "OK\n", not // "OKAY". - android::base::WriteStringToFd("OK\n", ack_reply_fd); + if (!android::base::WriteStringToFd("OK\n", ack_reply_fd)) { + fatal_errno("error writing ACK to fd %d", ack_reply_fd); + } unix_close(ack_reply_fd); #endif close_stdin(); diff --git a/adb/commandline.cpp b/adb/commandline.cpp index 0ac3556ea..1c5b5bd40 100644 --- a/adb/commandline.cpp +++ b/adb/commandline.cpp @@ -968,7 +968,7 @@ int adb_commandline(int argc, const char **argv) { server_port = strtol(server_port_str, nullptr, 0); if (server_port <= 0 || server_port > 65535) { fprintf(stderr, - "adb: Env var ANDROID_ADB_SERVER_PORT must be a positive number less than 65535. Got \"%s\"\n", + "adb: Env var ANDROID_ADB_SERVER_PORT must be a positive number less than 65536. Got \"%s\"\n", server_port_str); return usage(); } @@ -1233,11 +1233,22 @@ int adb_commandline(int argc, const char **argv) { else if (!strcmp(argv[0], "kill-server")) { std::string error; int fd = _adb_connect("host:kill", &error); - if (fd == -1) { + if (fd == -2) { + // Failed to make network connection to server. Don't output the + // network error since that is expected. fprintf(stderr,"* server not running *\n"); + // Successful exit code because the server is already "killed". + return 0; + } else if (fd == -1) { + // Some other error. + fprintf(stderr, "error: %s\n", error.c_str()); return 1; + } else { + // Successfully connected, kill command sent, okay status came back. + // Server should exit() in a moment, if not already. + adb_close(fd); + return 0; } - return 0; } else if (!strcmp(argv[0], "sideload")) { if (argc != 2) return usage(); @@ -1421,7 +1432,11 @@ int adb_commandline(int argc, const char **argv) { } else if (!strcmp(argv[0], "start-server")) { std::string error; - return adb_connect("host:start-server", &error); + const int result = adb_connect("host:start-server", &error); + if (result < 0) { + fprintf(stderr, "error: %s\n", error.c_str()); + } + return result; } else if (!strcmp(argv[0], "backup")) { return backup(argc, argv);