From 424af02f363a305a349ff99e1cc253ac5bc642c9 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Fri, 29 May 2015 17:55:19 -0700 Subject: [PATCH] Fix error handling/reporting for "adb forward" and "adb reverse". We really need better infrastructure for parsing adb subcommands, but in the meantime... At least this cleans up a little more of the implementation too. Bug: http://b/20736014 Change-Id: I76209847da3724906c71924017bcb69fa31e0b49 --- adb/adb.cpp | 64 ++++++++++++-------------- adb/adb_client.cpp | 16 ++++--- adb/adb_client.h | 6 +-- adb/adb_listeners.cpp | 44 +++++++----------- adb/adb_listeners.h | 6 --- adb/commandline.cpp | 103 ++++++++++++------------------------------ 6 files changed, 86 insertions(+), 153 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index 2c959a441..9c1ead575 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -721,56 +721,50 @@ int handle_forward_request(const char* service, TransportType type, const char* return 1; } - if (!strncmp(service, "forward:",8) || - !strncmp(service, "killforward:",12)) { - char *local, *remote; - atransport *transport; - - int createForward = strncmp(service, "kill", 4); - int no_rebind = 0; - - local = strchr(service, ':') + 1; - - // Handle forward:norebind:... here - if (createForward && !strncmp(local, "norebind:", 9)) { - no_rebind = 1; - local = strchr(local, ':') + 1; + if (!strncmp(service, "forward:", 8) || !strncmp(service, "killforward:", 12)) { + // killforward:local + // forward:(norebind:)?local;remote + bool kill_forward = false; + bool no_rebind = false; + if (android::base::StartsWith(service, "killforward:")) { + kill_forward = true; + service += 12; + if (android::base::StartsWith(service, "norebind:")) { + no_rebind = true; + service += 9; + } + } else { + service += 8; } - remote = strchr(local,';'); + std::vector pieces = android::base::Split(service, ";"); - if (createForward) { - // Check forward: parameter format: ';' - if(remote == 0) { - SendFail(reply_fd, "malformed forward spec"); - return 1; - } - - *remote++ = 0; - if((local[0] == 0) || (remote[0] == 0) || (remote[0] == '*')) { - SendFail(reply_fd, "malformed forward spec"); + if (kill_forward) { + // Check killforward: parameter format: '' + if (pieces.size() != 1 || pieces[0].empty()) { + SendFail(reply_fd, android::base::StringPrintf("bad killforward: %s", service)); return 1; } } else { - // Check killforward: parameter format: '' - if (local[0] == 0) { - SendFail(reply_fd, "malformed forward spec"); + // Check forward: parameter format: ';' + if (pieces.size() != 2 || pieces[0].empty() || pieces[1].empty() || pieces[1][0] == '*') { + SendFail(reply_fd, android::base::StringPrintf("bad forward: %s", service)); return 1; } } std::string error_msg; - transport = acquire_one_transport(kCsAny, type, serial, &error_msg); + atransport* transport = acquire_one_transport(kCsAny, type, serial, &error_msg); if (!transport) { SendFail(reply_fd, error_msg); return 1; } InstallStatus r; - if (createForward) { - r = install_listener(local, remote, transport, no_rebind); + if (kill_forward) { + r = remove_listener(pieces[0].c_str(), transport); } else { - r = remove_listener(local, transport); + r = install_listener(pieces[0], pieces[1].c_str(), transport, no_rebind); } if (r == INSTALL_STATUS_OK) { #if ADB_HOST @@ -783,7 +777,7 @@ int handle_forward_request(const char* service, TransportType type, const char* std::string message; switch (r) { - case INSTALL_STATUS_OK: message = " "; break; + 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", strerror(errno)); @@ -791,7 +785,9 @@ int handle_forward_request(const char* service, TransportType type, const char* case INSTALL_STATUS_CANNOT_REBIND: message = android::base::StringPrintf("cannot rebind existing socket: %s", strerror(errno)); break; - case INSTALL_STATUS_LISTENER_NOT_FOUND: message = "listener not found"; break; + case INSTALL_STATUS_LISTENER_NOT_FOUND: + message = android::base::StringPrintf("listener '%s' not found", service); + break; } SendFail(reply_fd, message); return 1; diff --git a/adb/adb_client.cpp b/adb/adb_client.cpp index c73d73728..ef9a58697 100644 --- a/adb/adb_client.cpp +++ b/adb/adb_client.cpp @@ -256,19 +256,21 @@ error: } -int adb_command(const std::string& service, std::string* error) { - int fd = adb_connect(service, error); +bool adb_command(const std::string& service) { + std::string error; + int fd = adb_connect(service, &error); if (fd < 0) { - fprintf(stderr, "error: %s\n", error->c_str()); - return -1; + fprintf(stderr, "error: %s\n", error.c_str()); + return false; } - if (!adb_status(fd, error)) { + if (!adb_status(fd, &error)) { + fprintf(stderr, "error: %s\n", error.c_str()); adb_close(fd); - return -1; + return false; } - return 0; + return true; } bool adb_query(const std::string& service, std::string* result, std::string* error) { diff --git a/adb/adb_client.h b/adb/adb_client.h index 9895c490a..5de0638ef 100644 --- a/adb/adb_client.h +++ b/adb/adb_client.h @@ -26,9 +26,9 @@ int adb_connect(const std::string& service, std::string* error); int _adb_connect(const std::string& service, std::string* error); -// Connect to adb, connect to the named service, return 0 if the connection -// succeeded AND the service returned OKAY. -int adb_command(const std::string& service, std::string* error); +// Connect to adb, connect to the named service, returns true if the connection +// succeeded AND the service returned OKAY. Outputs any returned error otherwise. +bool adb_command(const std::string& service); // Connects to the named adb service and fills 'result' with the response. // Returns true on success; returns false and fills 'error' on failure. diff --git a/adb/adb_listeners.cpp b/adb/adb_listeners.cpp index cf193ab33..a335eeced 100644 --- a/adb/adb_listeners.cpp +++ b/adb/adb_listeners.cpp @@ -26,13 +26,12 @@ int gListenAll = 0; /* Not static because it is used in commandline.c. */ -alistener listener_list = { +static alistener listener_list = { .next = &listener_list, .prev = &listener_list, }; -void ss_listener_event_func(int _fd, unsigned ev, void *_l) -{ +static void ss_listener_event_func(int _fd, unsigned ev, void *_l) { asocket *s; if(ev & FDE_READ) { @@ -56,7 +55,7 @@ void ss_listener_event_func(int _fd, unsigned ev, void *_l) } } -void listener_event_func(int _fd, unsigned ev, void* _l) +static void listener_event_func(int _fd, unsigned ev, void* _l) { alistener* listener = reinterpret_cast(_l); asocket *s; @@ -106,38 +105,27 @@ static void free_listener(alistener* l) free(l); } -void listener_disconnect(void* listener, atransport* t) -{ +static void listener_disconnect(void* listener, atransport* t) { free_listener(reinterpret_cast(listener)); } -int local_name_to_fd(const char *name) -{ - int port; - - if(!strncmp("tcp:", name, 4)){ - int ret; - port = atoi(name + 4); - +static int local_name_to_fd(const char* name) { + if (!strncmp("tcp:", name, 4)) { + int port = atoi(name + 4); if (gListenAll > 0) { - ret = socket_inaddr_any_server(port, SOCK_STREAM); + return socket_inaddr_any_server(port, SOCK_STREAM); } else { - ret = socket_loopback_server(port, SOCK_STREAM); + return socket_loopback_server(port, SOCK_STREAM); } - - return ret; } #ifndef HAVE_WIN32_IPC /* no Unix-domain sockets on Win32 */ - // It's non-sensical to support the "reserved" space on the adb host side - if(!strncmp(name, "local:", 6)) { - return socket_local_server(name + 6, - ANDROID_SOCKET_NAMESPACE_ABSTRACT, SOCK_STREAM); - } else if(!strncmp(name, "localabstract:", 14)) { - return socket_local_server(name + 14, - ANDROID_SOCKET_NAMESPACE_ABSTRACT, SOCK_STREAM); - } else if(!strncmp(name, "localfilesystem:", 16)) { - return socket_local_server(name + 16, - ANDROID_SOCKET_NAMESPACE_FILESYSTEM, SOCK_STREAM); + // It's nonsensical to support the "reserved" space on the adb host side + if (!strncmp(name, "local:", 6)) { + return socket_local_server(name + 6, ANDROID_SOCKET_NAMESPACE_ABSTRACT, SOCK_STREAM); + } else if (!strncmp(name, "localabstract:", 14)) { + return socket_local_server(name + 14, ANDROID_SOCKET_NAMESPACE_ABSTRACT, SOCK_STREAM); + } else if (!strncmp(name, "localfilesystem:", 16)) { + return socket_local_server(name + 16, ANDROID_SOCKET_NAMESPACE_FILESYSTEM, SOCK_STREAM); } #endif diff --git a/adb/adb_listeners.h b/adb/adb_listeners.h index 9a7ded18d..67deb21c1 100644 --- a/adb/adb_listeners.h +++ b/adb/adb_listeners.h @@ -30,12 +30,6 @@ enum InstallStatus { INSTALL_STATUS_LISTENER_NOT_FOUND = -4, }; -extern alistener listener_list; - -void listener_disconnect(void* _l, atransport* t); -void listener_event_func(int _fd, unsigned ev, void *_l); -void ss_listener_event_func(int _fd, unsigned ev, void *_l); - InstallStatus install_listener(const std::string& local_name, const char* connect_to, atransport* transport, diff --git a/adb/commandline.cpp b/adb/commandline.cpp index d7bee91f3..7fbca3155 100644 --- a/adb/commandline.cpp +++ b/adb/commandline.cpp @@ -680,14 +680,7 @@ static bool wait_for_device(const char* service, TransportType t, const char* se } std::string cmd = format_host_command(service, t, serial); - std::string error; - if (adb_command(cmd, &error)) { - D("failure: %s *\n", error.c_str()); - fprintf(stderr,"error: %s\n", error.c_str()); - return false; - } - - return true; + return adb_command(cmd); } static int send_shell_command(TransportType transport_type, const char* serial, @@ -1249,90 +1242,50 @@ int adb_commandline(int argc, const char **argv) { if (argc != 1) return usage(); return send_shell_command(transport_type, serial, "shell:bugreport"); } - /* adb_command() wrapper commands */ else if (!strcmp(argv[0], "forward") || !strcmp(argv[0], "reverse")) { - std::string cmd; - char host_prefix[64]; - char reverse = (char) !strcmp(argv[0], "reverse"); - char remove = 0; - char remove_all = 0; - char list = 0; - char no_rebind = 0; - - // Parse options here. - while (argc > 1 && argv[1][0] == '-') { - if (!strcmp(argv[1], "--list")) - list = 1; - else if (!strcmp(argv[1], "--remove")) - remove = 1; - else if (!strcmp(argv[1], "--remove-all")) - remove_all = 1; - else if (!strcmp(argv[1], "--no-rebind")) - no_rebind = 1; - else { - return usage(); - } - argc--; - argv++; - } - - // Ensure we can only use one option at a time. - if (list + remove + remove_all + no_rebind > 1) { - return usage(); - } + bool reverse = !strcmp(argv[0], "reverse"); + ++argv; + --argc; + if (argc < 1) return usage(); // Determine the for this command. + std::string host_prefix; if (reverse) { - snprintf(host_prefix, sizeof host_prefix, "reverse"); + host_prefix = "reverse"; } else { if (serial) { - snprintf(host_prefix, sizeof host_prefix, "host-serial:%s", - serial); + host_prefix = android::base::StringPrintf("host-serial:%s", serial); } else if (transport_type == kTransportUsb) { - snprintf(host_prefix, sizeof host_prefix, "host-usb"); + host_prefix = "host-usb"; } else if (transport_type == kTransportLocal) { - snprintf(host_prefix, sizeof host_prefix, "host-local"); + host_prefix = "host-local"; } else { - snprintf(host_prefix, sizeof host_prefix, "host"); + host_prefix = "host"; } } - // Implement forward --list - if (list) { - if (argc != 1) { - return usage(); - } - - std::string query = android::base::StringPrintf("%s:list-forward", host_prefix); - return adb_query_command(query); - } - - // Implement forward --remove-all - else if (remove_all) { + std::string cmd; + if (strcmp(argv[0], "--list") == 0) { if (argc != 1) return usage(); - cmd = android::base::StringPrintf("%s:killforward-all", host_prefix); - } - - // Implement forward --remove - else if (remove) { + return adb_query_command(host_prefix + ":list-forward"); + } else if (strcmp(argv[0], "--remove-all") == 0) { + if (argc != 1) return usage(); + cmd = host_prefix + ":killforward-all"; + } else if (strcmp(argv[0], "--remove") == 0) { + // forward --remove if (argc != 2) return usage(); - cmd = android::base::StringPrintf("%s:killforward:%s", host_prefix, argv[1]); - } - // Or implement one of: - // forward - // forward --no-rebind - else { + cmd = host_prefix + ":killforward:" + argv[1]; + } else if (strcmp(argv[0], "--no-rebind") == 0) { + // forward --no-rebind if (argc != 3) return usage(); - const char* command = no_rebind ? "forward:norebind" : "forward"; - cmd = android::base::StringPrintf("%s:%s:%s;%s", host_prefix, command, argv[1], argv[2]); + cmd = host_prefix + ":forward:norebind:" + argv[1] + ";" + argv[2]; + } else { + // forward + if (argc != 2) return usage(); + cmd = host_prefix + ":forward:" + argv[0] + ";" + argv[1]; } - std::string error; - if (adb_command(cmd, &error)) { - fprintf(stderr, "error: %s\n", error.c_str()); - return 1; - } - return 0; + return adb_command(cmd) ? 0 : 1; } /* do_sync_*() commands */ else if (!strcmp(argv[0], "ls")) {