From bf7c605d87f87c03066c384cecb0f0c91aa31403 Mon Sep 17 00:00:00 2001 From: Spencer Low Date: Tue, 11 Aug 2015 16:45:32 -0700 Subject: [PATCH] adb: improve network error info - handle_forward_request - Because we have detailed info about which syscall failed (at least on Win32), use a more generic prefix of "cannot bind listener" followed by the detailed info. - install_listener - Return string errors for a few errors even though I don't think any callers actually output the string for those errors. - Remove the printf since the callers print the message themselves. - adb_main - LOG(FATAL) calls abort() which on Windows calls the Windows Error Reporting service which pops up a dialog asking if you want a crashdump to be uploaded to Microsoft. So really, abort() is designed for app bugs. Windows isn't the only one doing this, Chromium also makes LOG(FATAL) crashdump-ready. Since an error here is not necessarily an app-bug, use a 'normal' error output API like fatal() which prints an error and just uses exit(). - sysdeps_win32.cpp - When Winsock APIs fail, make the string clarify which API failed. Use terse unix-style descriptions (like what you'd get from cp/mv/dd/etc.). - Don't trace WSAEWOULDBLOCK from recv() which is a normal occurrence. - Add a comment about WSAEWOULDBLOCK => EAGAIN. Change-Id: I58e47f49fa2f6c1b4b92a36d0c4bfe369b456f2a Signed-off-by: Spencer Low --- adb/adb.cpp | 2 +- adb/adb_listeners.cpp | 4 ++- adb/client/main.cpp | 2 +- adb/sysdeps_win32.cpp | 64 ++++++++++++++++++++++++++++++------------- 4 files changed, 50 insertions(+), 22 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index 821b785df..f4e779cde 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -847,7 +847,7 @@ int handle_forward_request(const char* service, TransportType type, const char* case INSTALL_STATUS_OK: message = "success (!)"; break; case INSTALL_STATUS_INTERNAL_ERROR: message = "internal error"; break; case INSTALL_STATUS_CANNOT_BIND: - message = android::base::StringPrintf("cannot bind to socket: %s", + message = android::base::StringPrintf("cannot bind listener: %s", error.c_str()); break; case INSTALL_STATUS_CANNOT_REBIND: diff --git a/adb/adb_listeners.cpp b/adb/adb_listeners.cpp index 1e7ce5d28..8fb2d19da 100644 --- a/adb/adb_listeners.cpp +++ b/adb/adb_listeners.cpp @@ -190,16 +190,19 @@ InstallStatus install_listener(const std::string& local_name, /* can't repurpose a smartsocket */ if(l->connect_to[0] == '*') { + *error = "cannot repurpose smartsocket"; return INSTALL_STATUS_INTERNAL_ERROR; } /* can't repurpose a listener if 'no_rebind' is true */ if (no_rebind) { + *error = "cannot rebind"; return INSTALL_STATUS_CANNOT_REBIND; } cto = strdup(connect_to); if(cto == 0) { + *error = "cannot duplicate string"; return INSTALL_STATUS_INTERNAL_ERROR; } @@ -232,7 +235,6 @@ InstallStatus install_listener(const std::string& local_name, listener->fd = local_name_to_fd(listener->local_name, error); if (listener->fd < 0) { - printf("cannot bind '%s': %s\n", listener->local_name, error->c_str()); free(listener->local_name); free(listener->connect_to); free(listener); diff --git a/adb/client/main.cpp b/adb/client/main.cpp index 73acbb0b8..e0e7e7760 100644 --- a/adb/client/main.cpp +++ b/adb/client/main.cpp @@ -154,7 +154,7 @@ int adb_main(int is_daemon, int server_port, int ack_reply_fd) { std::string error; std::string local_name = android::base::StringPrintf("tcp:%d", server_port); if (install_listener(local_name, "*smartsocket*", nullptr, 0, &error)) { - LOG(FATAL) << "Could not install *smartsocket* listener: " << error; + fatal("could not install *smartsocket* listener: %s", error.c_str()); } // Inform our parent that we are up and running. diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp index db552a2da..80d41a4cc 100644 --- a/adb/sysdeps_win32.cpp +++ b/adb/sysdeps_win32.cpp @@ -554,6 +554,9 @@ static void _socket_set_errno( const DWORD err ) { // POSIX and socket error codes, so this can only meaningfully map so much. switch ( err ) { case 0: errno = 0; break; + // Mapping WSAEWOULDBLOCK to EAGAIN is absolutely critical because + // non-blocking sockets can cause an error code of WSAEWOULDBLOCK and + // callers check specifically for EAGAIN. case WSAEWOULDBLOCK: errno = EAGAIN; break; case WSAEINTR: errno = EINTR; break; case WSAEFAULT: errno = EFAULT; break; @@ -619,8 +622,12 @@ static int _fh_socket_read(FH f, void* buf, int len) { int result = recv(f->fh_socket, reinterpret_cast(buf), len, 0); if (result == SOCKET_ERROR) { const DWORD err = WSAGetLastError(); - D("recv fd %d failed: %s\n", _fh_to_int(f), - SystemErrorCodeToString(err).c_str()); + // WSAEWOULDBLOCK is normal with a non-blocking socket, so don't trace + // that to reduce spam and confusion. + if (err != WSAEWOULDBLOCK) { + D("recv fd %d failed: %s\n", _fh_to_int(f), + SystemErrorCodeToString(err).c_str()); + } _socket_set_errno(err); result = -1; } @@ -698,14 +705,19 @@ int network_loopback_client(int port, int type, std::string* error) { s = socket(AF_INET, type, 0); if(s == INVALID_SOCKET) { - *error = SystemErrorCodeToString(WSAGetLastError()); - D("could not create socket: %s\n", error->c_str()); + *error = android::base::StringPrintf("cannot create socket: %s", + SystemErrorCodeToString(WSAGetLastError()).c_str()); + D("%s\n", error->c_str()); return -1; } f->fh_socket = s; if(connect(s, (struct sockaddr *) &addr, sizeof(addr)) == SOCKET_ERROR) { - *error = SystemErrorCodeToString(WSAGetLastError()); + // Save err just in case inet_ntoa() or ntohs() changes the last error. + const DWORD err = WSAGetLastError(); + *error = android::base::StringPrintf("cannot connect to %s:%u: %s", + inet_ntoa(addr.sin_addr), ntohs(addr.sin_port), + SystemErrorCodeToString(err).c_str()); D("could not connect to %s:%d: %s\n", type != SOCK_STREAM ? "udp" : "tcp", port, error->c_str()); return -1; @@ -747,31 +759,40 @@ static int _network_server(int port, int type, u_long interface_address, // IPv4 and IPv6. s = socket(AF_INET, type, 0); if (s == INVALID_SOCKET) { - *error = SystemErrorCodeToString(WSAGetLastError()); - D("could not create socket: %s\n", error->c_str()); + *error = android::base::StringPrintf("cannot create socket: %s", + SystemErrorCodeToString(WSAGetLastError()).c_str()); + D("%s\n", error->c_str()); return -1; } f->fh_socket = s; + // Note: SO_REUSEADDR on Windows allows multiple processes to bind to the + // same port, so instead use SO_EXCLUSIVEADDRUSE. n = 1; if (setsockopt(s, SOL_SOCKET, SO_EXCLUSIVEADDRUSE, (const char*)&n, sizeof(n)) == SOCKET_ERROR) { - *error = SystemErrorCodeToString(WSAGetLastError()); - D("setsockopt level %d optname %d failed: %s\n", - SOL_SOCKET, SO_EXCLUSIVEADDRUSE, error->c_str()); + *error = android::base::StringPrintf( + "cannot set socket option SO_EXCLUSIVEADDRUSE: %s", + SystemErrorCodeToString(WSAGetLastError()).c_str()); + D("%s\n", error->c_str()); return -1; } - if(bind(s, (struct sockaddr *) &addr, sizeof(addr)) == SOCKET_ERROR) { - *error = SystemErrorCodeToString(WSAGetLastError()); + if (bind(s, (struct sockaddr *) &addr, sizeof(addr)) == SOCKET_ERROR) { + // Save err just in case inet_ntoa() or ntohs() changes the last error. + const DWORD err = WSAGetLastError(); + *error = android::base::StringPrintf("cannot bind to %s:%u: %s", + inet_ntoa(addr.sin_addr), ntohs(addr.sin_port), + SystemErrorCodeToString(err).c_str()); D("could not bind to %s:%d: %s\n", type != SOCK_STREAM ? "udp" : "tcp", port, error->c_str()); return -1; } if (type == SOCK_STREAM) { if (listen(s, LISTEN_BACKLOG) == SOCKET_ERROR) { - *error = SystemErrorCodeToString(WSAGetLastError()); + *error = android::base::StringPrintf("cannot listen on socket: %s", + SystemErrorCodeToString(WSAGetLastError()).c_str()); D("could not listen on %s:%d: %s\n", type != SOCK_STREAM ? "udp" : "tcp", port, error->c_str()); return -1; @@ -822,9 +843,10 @@ int network_connect(const std::string& host, int port, int type, int timeout, st // with GetProcAddress("GetAddrInfoW"). #endif if (getaddrinfo(host.c_str(), port_str, &hints, &addrinfo_ptr) != 0) { - *error = SystemErrorCodeToString(WSAGetLastError()); - D("could not resolve host '%s' and port %s: %s\n", host.c_str(), - port_str, error->c_str()); + *error = android::base::StringPrintf( + "cannot resolve host '%s' and port %s: %s", host.c_str(), + port_str, SystemErrorCodeToString(WSAGetLastError()).c_str()); + D("%s\n", error->c_str()); return -1; } std::unique_ptr @@ -837,8 +859,9 @@ int network_connect(const std::string& host, int port, int type, int timeout, st SOCKET s = socket(addrinfo->ai_family, addrinfo->ai_socktype, addrinfo->ai_protocol); if(s == INVALID_SOCKET) { - *error = SystemErrorCodeToString(WSAGetLastError()); - D("could not create socket: %s\n", error->c_str()); + *error = android::base::StringPrintf("cannot create socket: %s", + SystemErrorCodeToString(WSAGetLastError()).c_str()); + D("%s\n", error->c_str()); return -1; } f->fh_socket = s; @@ -846,7 +869,10 @@ int network_connect(const std::string& host, int port, int type, int timeout, st // TODO: Implement timeouts for Windows. Seems like the default in theory // (according to http://serverfault.com/a/671453) and in practice is 21 sec. if(connect(s, addrinfo->ai_addr, addrinfo->ai_addrlen) == SOCKET_ERROR) { - *error = SystemErrorCodeToString(WSAGetLastError()); + // TODO: Use WSAAddressToString or inet_ntop on address. + *error = android::base::StringPrintf("cannot connect to %s:%s: %s", + host.c_str(), port_str, + SystemErrorCodeToString(WSAGetLastError()).c_str()); D("could not connect to %s:%s:%s: %s\n", type != SOCK_STREAM ? "udp" : "tcp", host.c_str(), port_str, error->c_str());