am 9dad4ec4: Merge "Clean up the locking in usb_linux.cpp."

* commit '9dad4ec4408bf412c265ac9bb27c84bb5098e67c':
  Clean up the locking in usb_linux.cpp.

adb/Android.mk

@@ -47,6 +47,7 @@ LIBADB_TEST_SRCS := \
 LIBADB_CFLAGS := \
     $(ADB_COMMON_CFLAGS) \
     -fvisibility=hidden \
+    -std=c++14 \
 
 LIBADB_darwin_SRC_FILES := \
     fdevent.cpp \

adb/usb_linux.cpp

@@ -22,6 +22,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <linux/usb/ch9.h>
 #include <linux/usbdevice_fs.h>
 #include <linux/version.h>
 #include <stdio.h>
@@ -31,7 +32,12 @@
 #include <sys/time.h>
 #include <sys/types.h>
 #include <unistd.h>
-#include <linux/usb/ch9.h>
+
+#include <chrono>
+#include <condition_variable>
+#include <list>
+#include <mutex>
+#include <string>
 
 #include <base/file.h>
 #include <base/stringprintf.h>
@@ -40,110 +46,92 @@
 #include "adb.h"
 #include "transport.h"
 
+using namespace std::literals;
+
 /* usb scan debugging is waaaay too verbose */
 #define DBGX(x...)
 
-ADB_MUTEX_DEFINE( usb_lock );
+struct usb_handle {
+    ~usb_handle() {
+      if (fd != -1) unix_close(fd);
+    }
 
-struct usb_handle
-{
-    usb_handle *prev;
-    usb_handle *next;
-
-    char fname[64];
-    int desc;
+    std::string path;
+    int fd = -1;
     unsigned char ep_in;
     unsigned char ep_out;
 
     unsigned zero_mask;
-    unsigned writeable;
+    unsigned writeable = 1;
 
-    struct usbdevfs_urb urb_in;
-    struct usbdevfs_urb urb_out;
+    usbdevfs_urb urb_in;
+    usbdevfs_urb urb_out;
 
-    int urb_in_busy;
-    int urb_out_busy;
-    int dead;
+    bool urb_in_busy = false;
+    bool urb_out_busy = false;
+    bool dead = false;
 
-    adb_cond_t notify;
-    adb_mutex_t lock;
+    std::condition_variable cv;
+    std::mutex mutex;
 
     // for garbage collecting disconnected devices
-    int mark;
+    bool mark;
 
     // ID of thread currently in REAPURB
-    pthread_t reaper_thread;
+    pthread_t reaper_thread = 0;
 };
 
-static usb_handle handle_list = {
-    .prev = &handle_list,
-    .next = &handle_list,
-};
+static std::mutex g_usb_handles_mutex;
+static std::list<usb_handle*> g_usb_handles;
 
-static int known_device(const char *dev_name)
-{
-    usb_handle *usb;
-
-    adb_mutex_lock(&usb_lock);
-    for(usb = handle_list.next; usb != &handle_list; usb = usb->next){
-        if(!strcmp(usb->fname, dev_name)) {
+static int is_known_device(const char* dev_name) {
+    std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
+    for (usb_handle* usb : g_usb_handles) {
+        if (usb->path == dev_name) {
             // set mark flag to indicate this device is still alive
-            usb->mark = 1;
-            adb_mutex_unlock(&usb_lock);
+            usb->mark = true;
             return 1;
         }
     }
-    adb_mutex_unlock(&usb_lock);
     return 0;
 }
 
-static void kick_disconnected_devices()
-{
-    usb_handle *usb;
-
-    adb_mutex_lock(&usb_lock);
+static void kick_disconnected_devices() {
+    std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
     // kick any devices in the device list that were not found in the device scan
-    for(usb = handle_list.next; usb != &handle_list; usb = usb->next){
-        if (usb->mark == 0) {
+    for (usb_handle* usb : g_usb_handles) {
+        if (!usb->mark) {
             usb_kick(usb);
         } else {
-            usb->mark = 0;
+            usb->mark = false;
         }
     }
-    adb_mutex_unlock(&usb_lock);
-
 }
 
-static inline int badname(const char *name)
-{
-    while(*name) {
-        if(!isdigit(*name++)) return 1;
+static inline bool contains_non_digit(const char* name) {
+    while (*name) {
+        if (!isdigit(*name++)) return true;
     }
-    return 0;
+    return false;
 }
 
-static void find_usb_device(const char *base,
+static void find_usb_device(const std::string& base,
         void (*register_device_callback)
-                (const char *, const char *, unsigned char, unsigned char, int, int, unsigned))
+                (const char*, const char*, unsigned char, unsigned char, int, int, unsigned))
 {
-    char busname[32], devname[32];
-    unsigned char local_ep_in, local_ep_out;
-    DIR *busdir , *devdir ;
-    struct dirent *de;
-    int fd ;
+    std::unique_ptr<DIR, int(*)(DIR*)> bus_dir(opendir(base.c_str()), closedir);
+    if (!bus_dir) return;
 
-    busdir = opendir(base);
-    if(busdir == 0) return;
+    dirent* de;
+    while ((de = readdir(bus_dir.get())) != 0) {
+        if (contains_non_digit(de->d_name)) continue;
 
-    while((de = readdir(busdir)) != 0) {
-        if(badname(de->d_name)) continue;
+        std::string bus_name = base + "/" + de->d_name;
 
-        snprintf(busname, sizeof busname, "%s/%s", base, de->d_name);
-        devdir = opendir(busname);
-        if(devdir == 0) continue;
+        std::unique_ptr<DIR, int(*)(DIR*)> dev_dir(opendir(bus_name.c_str()), closedir);
+        if (!dev_dir) continue;
 
-//        DBGX("[ scanning %s ]\n", busname);
-        while((de = readdir(devdir))) {
+        while ((de = readdir(dev_dir.get()))) {
             unsigned char devdesc[4096];
             unsigned char* bufptr = devdesc;
             unsigned char* bufend;
@@ -153,22 +141,20 @@ static void find_usb_device(const char *base,
             struct usb_endpoint_descriptor *ep1, *ep2;
             unsigned zero_mask = 0;
             unsigned vid, pid;
-            size_t desclength;
 
-            if(badname(de->d_name)) continue;
-            snprintf(devname, sizeof devname, "%s/%s", busname, de->d_name);
+            if (contains_non_digit(de->d_name)) continue;
 
-            if(known_device(devname)) {
-                DBGX("skipping %s\n", devname);
+            std::string dev_name = bus_name + "/" + de->d_name;
+            if (is_known_device(dev_name.c_str())) {
                 continue;
             }
 
-//            DBGX("[ scanning %s ]\n", devname);
-            if((fd = unix_open(devname, O_RDONLY | O_CLOEXEC)) < 0) {
+            int fd = unix_open(dev_name.c_str(), O_RDONLY | O_CLOEXEC);
+            if (fd == -1) {
                 continue;
             }
 
-            desclength = unix_read(fd, devdesc, sizeof(devdesc));
+            size_t desclength = unix_read(fd, devdesc, sizeof(devdesc));
             bufend = bufptr + desclength;
 
                 // should have device and configuration descriptors, and atleast two endpoints
@@ -188,7 +174,7 @@ static void find_usb_device(const char *base,
 
             vid = device->idVendor;
             pid = device->idProduct;
-            DBGX("[ %s is V:%04x P:%04x ]\n", devname, vid, pid);
+            DBGX("[ %s is V:%04x P:%04x ]\n", dev_name.c_str(), vid, pid);
 
                 // should have config descriptor next
             config = (struct usb_config_descriptor *)bufptr;
@@ -225,7 +211,7 @@ static void find_usb_device(const char *base,
                         struct stat st;
                         char pathbuf[128];
                         char link[256];
-                        char *devpath = NULL;
+                        char *devpath = nullptr;
 
                         DBGX("looking for bulk endpoints\n");
                             // looks like ADB...
@@ -267,6 +253,7 @@ static void find_usb_device(const char *base,
                         }
 
                             // we have a match.  now we just need to figure out which is in and which is out.
+                        unsigned char local_ep_in, local_ep_out;
                         if (ep1->bEndpointAddress & USB_ENDPOINT_DIR_MASK) {
                             local_ep_in = ep1->bEndpointAddress;
                             local_ep_out = ep2->bEndpointAddress;
@@ -293,7 +280,7 @@ static void find_usb_device(const char *base,
                             }
                         }
 
-                        register_device_callback(devname, devpath,
+                        register_device_callback(dev_name.c_str(), devpath,
                                 local_ep_in, local_ep_out,
                                 interface->bInterfaceNumber, device->iSerialNumber, zero_mask);
                         break;
@@ -304,72 +291,54 @@ static void find_usb_device(const char *base,
             } // end of while
 
             unix_close(fd);
-        } // end of devdir while
-        closedir(devdir);
-    } //end of busdir while
-    closedir(busdir);
+        }
+    }
 }
 
-static int usb_bulk_write(usb_handle *h, const void *data, int len)
-{
-    struct usbdevfs_urb *urb = &h->urb_out;
-    int res;
-    struct timeval tv;
-    struct timespec ts;
+static int usb_bulk_write(usb_handle* h, const void* data, int len) {
+    std::unique_lock<std::mutex> lock(h->mutex);
+    D("++ usb_bulk_write ++\n");
 
+    usbdevfs_urb* urb = &h->urb_out;
     memset(urb, 0, sizeof(*urb));
     urb->type = USBDEVFS_URB_TYPE_BULK;
     urb->endpoint = h->ep_out;
     urb->status = -1;
-    urb->buffer = (void*) data;
+    urb->buffer = const_cast<void*>(data);
     urb->buffer_length = len;
 
-    D("++ write ++\n");
-
-    adb_mutex_lock(&h->lock);
-    if(h->dead) {
-        res = -1;
-        goto fail;
-    }
-    do {
-        res = ioctl(h->desc, USBDEVFS_SUBMITURB, urb);
-    } while((res < 0) && (errno == EINTR));
-
-    if(res < 0) {
-        goto fail;
+    if (h->dead) {
+        errno = EINVAL;
+        return -1;
     }
 
-    res = -1;
-    h->urb_out_busy = 1;
-    for(;;) {
-        /* time out after five seconds */
-        gettimeofday(&tv, NULL);
-        ts.tv_sec = tv.tv_sec + 5;
-        ts.tv_nsec = tv.tv_usec * 1000L;
-        res = pthread_cond_timedwait(&h->notify, &h->lock, &ts);
-        if(res < 0 || h->dead) {
-            break;
+    if (TEMP_FAILURE_RETRY(ioctl(h->fd, USBDEVFS_SUBMITURB, urb)) == -1) {
+        return -1;
+    }
+
+    h->urb_out_busy = true;
+    while (true) {
+        auto now = std::chrono::system_clock::now();
+        if (h->cv.wait_until(lock, now + 5s) == std::cv_status::timeout || h->dead) {
+            // TODO: call USBDEVFS_DISCARDURB?
+            errno = ETIMEDOUT;
+            return -1;
         }
-        if(h->urb_out_busy == 0) {
-            if(urb->status == 0) {
-                res = urb->actual_length;
+        if (!h->urb_out_busy) {
+            if (urb->status != 0) {
+                errno = -urb->status;
+                return -1;
             }
-            break;
+            return urb->actual_length;
         }
     }
-fail:
-    adb_mutex_unlock(&h->lock);
-    D("-- write --\n");
-    return res;
 }
 
-static int usb_bulk_read(usb_handle *h, void *data, int len)
-{
-    struct usbdevfs_urb *urb = &h->urb_in;
-    struct usbdevfs_urb *out = NULL;
-    int res;
-
+static int usb_bulk_read(usb_handle* h, void* data, int len) {
+    std::unique_lock<std::mutex> lock(h->mutex);
     D("++ usb_bulk_read ++\n");
+
+    usbdevfs_urb* urb = &h->urb_in;
     memset(urb, 0, sizeof(*urb));
     urb->type = USBDEVFS_URB_TYPE_BULK;
     urb->endpoint = h->ep_in;
@@ -377,63 +346,58 @@ static int usb_bulk_read(usb_handle *h, void *data, int len)
     urb->buffer = data;
     urb->buffer_length = len;
 
-
-    adb_mutex_lock(&h->lock);
-    if(h->dead) {
-        res = -1;
-        goto fail;
-    }
-    do {
-        res = ioctl(h->desc, USBDEVFS_SUBMITURB, urb);
-    } while((res < 0) && (errno == EINTR));
-
-    if(res < 0) {
-        goto fail;
+    if (h->dead) {
+        errno = EINVAL;
+        return -1;
     }
 
-    h->urb_in_busy = 1;
-    for(;;) {
+    if (TEMP_FAILURE_RETRY(ioctl(h->fd, USBDEVFS_SUBMITURB, urb)) == -1) {
+        return -1;
+    }
+
+    h->urb_in_busy = true;
+    while (true) {
         D("[ reap urb - wait ]\n");
         h->reaper_thread = pthread_self();
-        adb_mutex_unlock(&h->lock);
-        res = ioctl(h->desc, USBDEVFS_REAPURB, &out);
+        int fd = h->fd;
+        lock.unlock();
+
+        // This ioctl must not have TEMP_FAILURE_RETRY because we send SIGALRM to break out.
+        usbdevfs_urb* out = nullptr;
+        int res = ioctl(fd, USBDEVFS_REAPURB, &out);
         int saved_errno = errno;
-        adb_mutex_lock(&h->lock);
+
+        lock.lock();
         h->reaper_thread = 0;
-        if(h->dead) {
-            res = -1;
-            break;
+        if (h->dead) {
+            errno = EINVAL;
+            return -1;
         }
-        if(res < 0) {
-            if(saved_errno == EINTR) {
+        if (res < 0) {
+            if (saved_errno == EINTR) {
                 continue;
             }
             D("[ reap urb - error ]\n");
-            break;
+            errno = saved_errno;
+            return -1;
         }
-        D("[ urb @%p status = %d, actual = %d ]\n",
-            out, out->status, out->actual_length);
+        D("[ urb @%p status = %d, actual = %d ]\n", out, out->status, out->actual_length);
 
-        if(out == &h->urb_in) {
+        if (out == &h->urb_in) {
             D("[ reap urb - IN complete ]\n");
-            h->urb_in_busy = 0;
-            if(urb->status == 0) {
-                res = urb->actual_length;
-            } else {
-                res = -1;
+            h->urb_in_busy = false;
+            if (urb->status != 0) {
+                errno = -urb->status;
+                return -1;
             }
-            break;
+            return urb->actual_length;
         }
-        if(out == &h->urb_out) {
+        if (out == &h->urb_out) {
             D("[ reap urb - OUT compelete ]\n");
-            h->urb_out_busy = 0;
-            adb_cond_broadcast(&h->notify);
+            h->urb_out_busy = false;
+            h->cv.notify_all();
         }
     }
-fail:
-    adb_mutex_unlock(&h->lock);
-    D("-- usb_bulk_read --\n");
-    return res;
 }
 
 
@@ -443,19 +407,15 @@ int usb_write(usb_handle *h, const void *_data, int len)
 
     unsigned char *data = (unsigned char*) _data;
     int n = usb_bulk_write(h, data, len);
-    if(n != len) {
-        D("ERROR: n = %d, errno = %d (%s)\n",
-            n, errno, strerror(errno));
+    if (n != len) {
+        D("ERROR: n = %d, errno = %d (%s)\n", n, errno, strerror(errno));
         return -1;
     }
 
-    if(h->zero_mask && !(len & h->zero_mask)) {
-            /* if we need 0-markers and our transfer
-            ** is an even multiple of the packet size,
-            ** then send the zero markers.
-            */
-        n = usb_bulk_write(h, _data, 0);
-        return n;
+    if (h->zero_mask && !(len & h->zero_mask)) {
+        // If we need 0-markers and our transfer is an even multiple of the packet size,
+        // then send a zero marker.
+        return usb_bulk_write(h, _data, 0);
     }
 
     D("-- usb_write --\n");
@@ -471,11 +431,11 @@ int usb_read(usb_handle *h, void *_data, int len)
     while(len > 0) {
         int xfer = len;
 
-        D("[ usb read %d fd = %d], fname=%s\n", xfer, h->desc, h->fname);
+        D("[ usb read %d fd = %d], path=%s\n", xfer, h->fd, h->path.c_str());
         n = usb_bulk_read(h, data, xfer);
-        D("[ usb read %d ] = %d, fname=%s\n", xfer, n, h->fname);
+        D("[ usb read %d ] = %d, path=%s\n", xfer, n, h->path.c_str());
         if(n != xfer) {
-            if((errno == ETIMEDOUT) && (h->desc != -1)) {
+            if((errno == ETIMEDOUT) && (h->fd != -1)) {
                 D("[ timeout ]\n");
                 if(n > 0){
                     data += n;
@@ -496,12 +456,11 @@ int usb_read(usb_handle *h, void *_data, int len)
     return 0;
 }
 
-void usb_kick(usb_handle *h)
-{
-    D("[ kicking %p (fd = %d) ]\n", h, h->desc);
-    adb_mutex_lock(&h->lock);
-    if(h->dead == 0) {
-        h->dead = 1;
+void usb_kick(usb_handle* h) {
+    std::lock_guard<std::mutex> lock(h->mutex);
+    D("[ kicking %p (fd = %d) ]\n", h, h->fd);
+    if (!h->dead) {
+        h->dead = true;
 
         if (h->writeable) {
             /* HACK ALERT!
@@ -517,34 +476,27 @@ void usb_kick(usb_handle *h)
             ** but this ensures that a reader blocked on REAPURB
             ** will get unblocked
             */
-            ioctl(h->desc, USBDEVFS_DISCARDURB, &h->urb_in);
-            ioctl(h->desc, USBDEVFS_DISCARDURB, &h->urb_out);
+            ioctl(h->fd, USBDEVFS_DISCARDURB, &h->urb_in);
+            ioctl(h->fd, USBDEVFS_DISCARDURB, &h->urb_out);
             h->urb_in.status = -ENODEV;
             h->urb_out.status = -ENODEV;
-            h->urb_in_busy = 0;
-            h->urb_out_busy = 0;
-            adb_cond_broadcast(&h->notify);
+            h->urb_in_busy = false;
+            h->urb_out_busy = false;
+            h->cv.notify_all();
         } else {
             unregister_usb_transport(h);
         }
     }
-    adb_mutex_unlock(&h->lock);
 }
 
-int usb_close(usb_handle *h)
-{
-    D("++ usb close ++\n");
-    adb_mutex_lock(&usb_lock);
-    h->next->prev = h->prev;
-    h->prev->next = h->next;
-    h->prev = 0;
-    h->next = 0;
+int usb_close(usb_handle* h) {
+    std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
+    g_usb_handles.remove(h);
 
-    unix_close(h->desc);
-    D("-- usb closed %p (fd = %d) --\n", h, h->desc);
-    adb_mutex_unlock(&usb_lock);
+    D("-- usb close %p (fd = %d) --\n", h, h->fd);
+
+    delete h;
 
-    free(h);
     return 0;
 }
 
@@ -557,54 +509,44 @@ static void register_device(const char* dev_name, const char* dev_path,
     // from the list when we're finally closed and everything will work out
     // fine.
     //
-    // If we have a usb_handle on the list 'o handles with a matching name, we
+    // If we have a usb_handle on the list of handles with a matching name, we
     // have no further work to do.
-    adb_mutex_lock(&usb_lock);
-    for (usb_handle* usb = handle_list.next; usb != &handle_list; usb = usb->next) {
-        if (!strcmp(usb->fname, dev_name)) {
-            adb_mutex_unlock(&usb_lock);
-            return;
+    {
+        std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
+        for (usb_handle* usb: g_usb_handles) {
+            if (usb->path == dev_name) {
+                return;
+            }
         }
     }
-    adb_mutex_unlock(&usb_lock);
 
     D("[ usb located new device %s (%d/%d/%d) ]\n", dev_name, ep_in, ep_out, interface);
-    usb_handle* usb = reinterpret_cast<usb_handle*>(calloc(1, sizeof(usb_handle)));
-    if (usb == nullptr) fatal("couldn't allocate usb_handle");
-    strcpy(usb->fname, dev_name);
+    std::unique_ptr<usb_handle> usb(new usb_handle);
+    usb->path = dev_name;
     usb->ep_in = ep_in;
     usb->ep_out = ep_out;
     usb->zero_mask = zero_mask;
-    usb->writeable = 1;
 
-    adb_cond_init(&usb->notify, 0);
-    adb_mutex_init(&usb->lock, 0);
-    // Initialize mark to 1 so we don't get garbage collected after the device
-    // scan.
-    usb->mark = 1;
-    usb->reaper_thread = 0;
+    // Initialize mark so we don't get garbage collected after the device scan.
+    usb->mark = true;
 
-    usb->desc = unix_open(usb->fname, O_RDWR | O_CLOEXEC);
-    if (usb->desc == -1) {
+    usb->fd = unix_open(usb->path.c_str(), O_RDWR | O_CLOEXEC);
+    if (usb->fd == -1) {
         // Opening RW failed, so see if we have RO access.
-        usb->desc = unix_open(usb->fname, O_RDONLY | O_CLOEXEC);
-        if (usb->desc == -1) {
-            D("[ usb open %s failed: %s]\n", usb->fname, strerror(errno));
-            free(usb);
+        usb->fd = unix_open(usb->path.c_str(), O_RDONLY | O_CLOEXEC);
+        if (usb->fd == -1) {
+            D("[ usb open %s failed: %s]\n", usb->path.c_str(), strerror(errno));
             return;
         }
         usb->writeable = 0;
     }
 
-    D("[ usb opened %s%s, fd=%d]\n", usb->fname,
-      (usb->writeable ? "" : " (read-only)"), usb->desc);
+    D("[ usb opened %s%s, fd=%d]\n",
+      usb->path.c_str(), (usb->writeable ? "" : " (read-only)"), usb->fd);
 
     if (usb->writeable) {
-        if (ioctl(usb->desc, USBDEVFS_CLAIMINTERFACE, &interface) != 0) {
-            D("[ usb ioctl(%d, USBDEVFS_CLAIMINTERFACE) failed: %s]\n",
-              usb->desc, strerror(errno));
-            unix_close(usb->desc);
-            free(usb);
+        if (ioctl(usb->fd, USBDEVFS_CLAIMINTERFACE, &interface) != 0) {
+            D("[ usb ioctl(%d, USBDEVFS_CLAIMINTERFACE) failed: %s]\n", usb->fd, strerror(errno));
             return;
         }
     }
@@ -623,14 +565,12 @@ static void register_device(const char* dev_name, const char* dev_path,
     serial = android::base::Trim(serial);
 
     // Add to the end of the active handles.
-    adb_mutex_lock(&usb_lock);
-    usb->next = &handle_list;
-    usb->prev = handle_list.prev;
-    usb->prev->next = usb;
-    usb->next->prev = usb;
-    adb_mutex_unlock(&usb_lock);
-
-    register_usb_transport(usb, serial.c_str(), dev_path, usb->writeable);
+    usb_handle* done_usb = usb.release();
+    {
+        std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
+        g_usb_handles.push_back(done_usb);
+    }
+    register_usb_transport(done_usb, serial.c_str(), dev_path, done_usb->writeable);
 }
 
 static void* device_poll_thread(void* unused) {
@@ -644,19 +584,13 @@ static void* device_poll_thread(void* unused) {
     return nullptr;
 }
 
-static void sigalrm_handler(int signo) {
-    // don't need to do anything here
-}
-
-void usb_init()
-{
-    struct sigaction    actions;
-
+void usb_init() {
+    struct sigaction actions;
     memset(&actions, 0, sizeof(actions));
     sigemptyset(&actions.sa_mask);
     actions.sa_flags = 0;
-    actions.sa_handler = sigalrm_handler;
-    sigaction(SIGALRM,& actions, NULL);
+    actions.sa_handler = [](int) {};
+    sigaction(SIGALRM, &actions, nullptr);
 
     if (!adb_thread_create(device_poll_thread, nullptr)) {
         fatal_errno("cannot create input thread");