From cd35664cdc5097bf38fa2f12aa310855b3c123af Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Wed, 3 May 2017 17:25:34 -0700 Subject: [PATCH] Improve udev failure diagnostics. A couple of folks had trouble understanding the existing message. Before: 8XV7N15917000596 no permissions (udev requires plugdev group membership); see [http://developer.android.com/tools/device.html] After: 8XV7N15917000596 no permissions (user buttmunch is not in the plugdev group); see [http://developer.android.com/tools/device.html] This also fixes a libusb regression where we wouldn't show anything for devices where we don't have permissions. Bug: http://b/37707122 Test: ran "adb devices" as user buttmunch Change-Id: I2fcd735ff4178145432b532a6e4dc8c93b2743fd --- adb/client/usb_libusb.cpp | 107 +++++++++++++++++++++++++------------- adb/diagnose_usb.cpp | 43 ++++++++------- 2 files changed, 93 insertions(+), 57 deletions(-) diff --git a/adb/client/usb_libusb.cpp b/adb/client/usb_libusb.cpp index 20610ee46..a32c60545 100644 --- a/adb/client/usb_libusb.cpp +++ b/adb/client/usb_libusb.cpp @@ -159,6 +159,20 @@ static std::string get_device_address(libusb_device* device) { libusb_get_device_address(device)); } +static std::string get_device_serial_path(libusb_device* device) { + uint8_t ports[7]; + int port_count = libusb_get_port_numbers(device, ports, 7); + if (port_count < 0) return ""; + + std::string path = + StringPrintf("/sys/bus/usb/devices/%d-%d", libusb_get_bus_number(device), ports[0]); + for (int port = 1; port < port_count; ++port) { + path += StringPrintf(".%d", ports[port]); + } + path += "/serial"; + return path; +} + static bool endpoint_is_output(uint8_t endpoint) { return (endpoint & LIBUSB_ENDPOINT_DIR_MASK) == LIBUSB_ENDPOINT_OUT; } @@ -291,49 +305,67 @@ static void poll_for_devices() { } } - libusb_device_handle* handle_raw; + bool writable = true; + libusb_device_handle* handle_raw = nullptr; rc = libusb_open(device, &handle_raw); - if (rc != 0) { - LOG(WARNING) << "failed to open usb device at " << device_address << ": " - << libusb_error_name(rc); - continue; - } - unique_device_handle handle(handle_raw); - LOG(DEBUG) << "successfully opened adb device at " << device_address << ", " - << StringPrintf("bulk_in = %#x, bulk_out = %#x", bulk_in, bulk_out); - - device_serial.resize(255); - rc = libusb_get_string_descriptor_ascii( - handle_raw, device_desc.iSerialNumber, - reinterpret_cast(&device_serial[0]), device_serial.length()); if (rc == 0) { - LOG(WARNING) << "received empty serial from device at " << device_address; - continue; - } else if (rc < 0) { - LOG(WARNING) << "failed to get serial from device at " << device_address - << libusb_error_name(rc); - continue; - } - device_serial.resize(rc); + LOG(DEBUG) << "successfully opened adb device at " << device_address << ", " + << StringPrintf("bulk_in = %#x, bulk_out = %#x", bulk_in, bulk_out); - // WARNING: this isn't released via RAII. - rc = libusb_claim_interface(handle.get(), interface_num); - if (rc != 0) { - LOG(WARNING) << "failed to claim adb interface for device '" << device_serial << "'" - << libusb_error_name(rc); - continue; - } - - for (uint8_t endpoint : {bulk_in, bulk_out}) { - rc = libusb_clear_halt(handle.get(), endpoint); - if (rc != 0) { - LOG(WARNING) << "failed to clear halt on device '" << device_serial - << "' endpoint 0x" << std::hex << endpoint << ": " + device_serial.resize(255); + rc = libusb_get_string_descriptor_ascii( + handle_raw, device_desc.iSerialNumber, + reinterpret_cast(&device_serial[0]), device_serial.length()); + if (rc == 0) { + LOG(WARNING) << "received empty serial from device at " << device_address; + continue; + } else if (rc < 0) { + LOG(WARNING) << "failed to get serial from device at " << device_address << libusb_error_name(rc); - libusb_release_interface(handle.get(), interface_num); continue; } + device_serial.resize(rc); + + // WARNING: this isn't released via RAII. + rc = libusb_claim_interface(handle.get(), interface_num); + if (rc != 0) { + LOG(WARNING) << "failed to claim adb interface for device '" << device_serial + << "'" << libusb_error_name(rc); + continue; + } + + for (uint8_t endpoint : {bulk_in, bulk_out}) { + rc = libusb_clear_halt(handle.get(), endpoint); + if (rc != 0) { + LOG(WARNING) << "failed to clear halt on device '" << device_serial + << "' endpoint 0x" << std::hex << endpoint << ": " + << libusb_error_name(rc); + libusb_release_interface(handle.get(), interface_num); + continue; + } + } + } else { + LOG(WARNING) << "failed to open usb device at " << device_address << ": " + << libusb_error_name(rc); + writable = false; + +#if defined(__linux__) + // libusb doesn't think we should be messing around with devices we don't have + // write access to, but Linux at least lets us get the serial number anyway. + if (!android::base::ReadFileToString(get_device_serial_path(device), + &device_serial)) { + // We don't actually want to treat an unknown serial as an error because + // devices aren't able to communicate a serial number in early bringup. + // http://b/20883914 + device_serial = "unknown"; + } + device_serial = android::base::Trim(device_serial); +#else + // On Mac OS and Windows, we're screwed. But I don't think this situation actually + // happens on those OSes. + continue; +#endif } auto result = std::make_unique(device_address, device_serial, @@ -346,7 +378,8 @@ static void poll_for_devices() { usb_handles[device_address] = std::move(result); } - register_usb_transport(usb_handle_raw, device_serial.c_str(), device_address.c_str(), 1); + register_usb_transport(usb_handle_raw, device_serial.c_str(), device_address.c_str(), + writable); LOG(INFO) << "registered new usb device '" << device_serial << "'"; } diff --git a/adb/diagnose_usb.cpp b/adb/diagnose_usb.cpp index 0f067b0ec..9f721bf5f 100644 --- a/adb/diagnose_usb.cpp +++ b/adb/diagnose_usb.cpp @@ -25,13 +25,14 @@ #if defined(__linux__) #include +#include #endif static const char kPermissionsHelpUrl[] = "http://developer.android.com/tools/device.html"; -// Returns a message describing any potential problems we find with udev, or nullptr if we can't -// find plugdev information (i.e. udev is not installed). -static const char* GetUdevProblem() { +// Returns a message describing any potential problems we find with udev, or an empty string if we +// can't find plugdev information (i.e. udev is not installed). +static std::string GetUdevProblem() { #if defined(__linux__) errno = 0; group* plugdev_group = getgrnam("plugdev"); @@ -41,43 +42,45 @@ static const char* GetUdevProblem() { perror("failed to read plugdev group info"); } // We can't give any generally useful advice here, just let the caller print the help URL. - return nullptr; + return ""; } - // getgroups(2) indicates that the group_member() may not check the egid so we check it + // getgroups(2) indicates that the GNU group_member(3) may not check the egid so we check it // additionally just to be sure. if (group_member(plugdev_group->gr_gid) || getegid() == plugdev_group->gr_gid) { // The user is in plugdev so the problem is likely with the udev rules. - return "verify udev rules"; + return "user in plugdev group; are your udev rules wrong?"; } - return "udev requires plugdev group membership"; + passwd* pwd = getpwuid(getuid()); + return android::base::StringPrintf("user %s is not in the plugdev group", + pwd ? pwd->pw_name : "?"); #else - return nullptr; + return ""; #endif } // Short help text must be a single line, and will look something like: -// no permissions (reason); see +// +// no permissions (reason); see [URL] std::string UsbNoPermissionsShortHelpText() { std::string help_text = "no permissions"; - const char* problem = GetUdevProblem(); - if (problem != nullptr) { - help_text += android::base::StringPrintf(" (%s)", problem); - } + std::string problem(GetUdevProblem()); + if (!problem.empty()) help_text += " (" + problem + ")"; return android::base::StringPrintf("%s; see [%s]", help_text.c_str(), kPermissionsHelpUrl); } -// Long help text can span multiple lines and should provide more detailed information. +// Long help text can span multiple lines but doesn't currently provide more detailed information: +// +// insufficient permissions for device: reason +// See [URL] for more information std::string UsbNoPermissionsLongHelpText() { std::string header = "insufficient permissions for device"; - const char* problem = GetUdevProblem(); - if (problem != nullptr) { - header += android::base::StringPrintf(": %s", problem); - } + std::string problem(GetUdevProblem()); + if (!problem.empty()) header += ": " + problem; - return android::base::StringPrintf("%s.\nSee [%s] for more information.", - header.c_str(), kPermissionsHelpUrl); + return android::base::StringPrintf("%s\nSee [%s] for more information", header.c_str(), + kPermissionsHelpUrl); }