Clean up HID backend and API.

device/hid/hid_connection_linux.cc

 // Copyright (c) 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "device/hid/hid_connection_linux.h"
 
 #include <errno.h>
 #include <fcntl.h>
 #include <libudev.h>
 #include <linux/hidraw.h>
+#include <sys/ioctl.h>
+
 #include <string>
 
 #include "base/threading/thread_restrictions.h"
 #include "base/tuple.h"
 #include "device/hid/hid_service.h"
 #include "device/hid/hid_service_linux.h"
 
+// These are already defined in newer versions of linux/hidraw.h.
+#ifndef HIDIOCSFEATURE
+#define HIDIOCSFEATURE(len) _IOC(_IOC_WRITE | _IOC_READ, 'H', 0x06, len)
+#endif
+#ifndef HIDIOCGFEATURE
+#define HIDIOCGFEATURE(len) _IOC(_IOC_WRITE | _IOC_READ, 'H', 0x07, len)
+#endif
 
 namespace device {
 
 namespace {
 
+// Copies a buffer into a new one a report ID byte inserted at the front.

[Mark Mentovai, 2014/02/26 17:41:44]

This comment is missing a word?

[Ken Rockot(use gerrit already), 2014/02/26 22:36:39]

Done.

+scoped_refptr<net::IOBufferWithSize> CopyBufferWithReportId(
+    scoped_refptr<net::IOBufferWithSize> buffer,
+    uint8_t report_id) {
+  scoped_refptr<net::IOBufferWithSize> new_buffer(
+      new net::IOBufferWithSize(buffer->size() + 1));
+  new_buffer->data()[0] = report_id;
+  memcpy(new_buffer->data() + 1, buffer->data(), buffer->size());
+  return new_buffer;
+}
+
 const char kHidrawSubsystem[] = "hidraw";
 
 }  // namespace
 
 HidConnectionLinux::HidConnectionLinux(HidDeviceInfo device_info,
                                        ScopedUdevDevicePtr udev_raw_device)
-    : HidConnection(device_info),
-      initialized_(false) {
+    : HidConnection(device_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   udev_device* dev = udev_raw_device.get();
   std::string dev_node;
   if (!FindHidrawDevNode(dev, &dev_node)) {
     LOG(ERROR) << "Cannot open HID device as hidraw device.";
     return;
   }
 
   int flags = base::File::FLAG_OPEN |
@@ skipping 13 matching lines @@
     return;
   }
   device_file_ = device_file.Pass();
 
   if (!base::MessageLoopForIO::current()->WatchFileDescriptor(
       device_file_.GetPlatformFile(),
       true,
       base::MessageLoopForIO::WATCH_READ_WRITE,
       &device_file_watcher_,
       this)) {
-    LOG(ERROR) << "Cannot start watching file descriptor.";
-    return;
+    LOG(ERROR) << "Failed to start watching device file.";
   }
-
-  initialized_ = true;
 }
 
 HidConnectionLinux::~HidConnectionLinux() {
   DCHECK(thread_checker_.CalledOnValidThread());
   Disconnect();
 }
 
 void HidConnectionLinux::OnFileCanReadWithoutBlocking(int fd) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_EQ(fd, device_file_.GetPlatformFile());
-  DCHECK(initialized_);
 
   uint8 buffer[1024] = {0};
-  int bytes = read(device_file_.GetPlatformFile(), buffer, 1024);
-  if (bytes < 0) {
+  int bytes_read = read(device_file_.GetPlatformFile(), buffer, 1024);
+  if (bytes_read < 0) {
     if (errno == EAGAIN) {
       return;
     }
     Disconnect();
     return;
   }
-  scoped_refptr<net::IOBuffer> io_buffer(new net::IOBuffer(bytes));
-  memcpy(io_buffer->data(), buffer, bytes);
-  input_reports_.push(std::make_pair(io_buffer, bytes));
 
+  PendingHidReport report;
+  report.buffer = new net::IOBufferWithSize(bytes_read);
+  memcpy(report.buffer->data(), buffer, bytes_read);
+  pending_reports_.push(report);
   ProcessReadQueue();
 }
 
 void HidConnectionLinux::OnFileCanWriteWithoutBlocking(int fd) {}
 
 void HidConnectionLinux::Disconnect() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!initialized_)
-    return;
-
-  initialized_ = false;
   device_file_watcher_.StopWatchingFileDescriptor();
   device_file_.Close();
-  while (!read_queue_.empty()) {
-    PendingRequest callback = read_queue_.front();
-    read_queue_.pop();
-    callback.c.Run(false, 0);
+  while (!pending_reads_.empty()) {
+    PendingHidRead pending_read = pending_reads_.front();
+    pending_reads_.pop();
+    pending_read.callback.Run(false, 0);
   }
 }
 
-void HidConnectionLinux::Read(scoped_refptr<net::IOBuffer> buffer,
-                              size_t size,
+void HidConnectionLinux::Read(scoped_refptr<net::IOBufferWithSize> buffer,
                               const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!initialized_) {
-    DCHECK(read_queue_.empty());
-    // There might be unread reports.
-    if (!input_reports_.empty()){
-      read_queue_.push(MakeTuple(buffer, size, callback));
-      ProcessReadQueue();
-    }
+  PendingHidRead pending_read;
+  pending_read.buffer = buffer;
+  pending_read.callback = callback;
+  pending_reads_.push(pending_read);
+  ProcessReadQueue();
+}
+
+void HidConnectionLinux::Write(uint8_t report_id,
+                               scoped_refptr<net::IOBufferWithSize> buffer,
+                               const IOCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // If report ID is non-zero, insert it into a new copy of the buffer.
+  if (report_id != 0)
+    buffer = CopyBufferWithReportId(buffer, report_id);
+  int bytes_written =
+      write(device_file_.GetPlatformFile(), buffer->data(), buffer->size());

[Mark Mentovai, 2014/02/26 17:41:44]

Do you need to handle a short write or EINTR? Maybe not. I thought about this
for the read on line 97 too but it seems even less necessary there.

[Ken Rockot(use gerrit already), 2014/02/26 22:36:39]

I lean against treating short reads or writes as error conditions; the consumer
can decide. I don't see any harm in handling EINTR though, so that's done.

+  if (bytes_written < 0) {
+    Disconnect();
     callback.Run(false, 0);
-    return;
   } else {
-    read_queue_.push(MakeTuple(buffer, size, callback));
-    ProcessReadQueue();
+    callback.Run(true, bytes_written);
   }
 }
 
-void HidConnectionLinux::Write(scoped_refptr<net::IOBuffer> buffer,
-                               size_t size,
-                               const IOCallback& callback) {
+void HidConnectionLinux::GetFeatureReport(
+    uint8_t report_id,
+    scoped_refptr<net::IOBufferWithSize> buffer,
+    const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!initialized_) {
+
+  if (buffer->size() == 0) {
     callback.Run(false, 0);
     return;
-  } else {
-    int bytes = write(device_file_.GetPlatformFile(), buffer->data(), size);
-    if (bytes < 0) {
-      Disconnect();
-      callback.Run(false, 0);
+  }
+
+  // The first byte of the destination buffer is the report ID being requested.
+  buffer->data()[0] = report_id;
+  int result = ioctl(device_file_.GetPlatformFile(),
+                     HIDIOCGFEATURE(buffer->size()),
+                     buffer->data());
+  if (result < 0)
+    callback.Run(false, 0);
+  else
+    callback.Run(true, result);
+}
+
+void HidConnectionLinux::SendFeatureReport(
+    uint8_t report_id,
+    scoped_refptr<net::IOBufferWithSize> buffer,
+    const IOCallback& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (report_id != 0)
+    buffer = CopyBufferWithReportId(buffer, report_id);
+  int result = ioctl(device_file_.GetPlatformFile(),
+                     HIDIOCSFEATURE(buffer->size()),
+                     buffer->data());
+  if (result < 0)
+    callback.Run(false, 0);
+  else
+    callback.Run(true, result);
+}
+
+void HidConnectionLinux::ProcessReadQueue() {
+  while (pending_reads_.size() && pending_reports_.size()) {
+    PendingHidRead read = pending_reads_.front();
+    pending_reads_.pop();
+    PendingHidReport report = pending_reports_.front();
+    if (report.buffer->size() > read.buffer->size()) {
+      read.callback.Run(false, report.buffer->size());
     } else {
-      callback.Run(true, bytes);
+      memcpy(read.buffer->data(), report.buffer->data(), report.buffer->size());
+      pending_reports_.pop();
+      read.callback.Run(true, report.buffer->size());
     }
   }
 }
-
-void HidConnectionLinux::GetFeatureReport(scoped_refptr<net::IOBuffer> buffer,
-                                          size_t size,
-                                          const IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!initialized_) {
-    callback.Run(false, 0);
-    return;
-  }
-  NOTIMPLEMENTED();
-}
-
-void HidConnectionLinux::SendFeatureReport(scoped_refptr<net::IOBuffer> buffer,
-                                           size_t size,
-                                           const IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!initialized_) {
-    callback.Run(false, 0);
-    return;
-  }
-  NOTIMPLEMENTED();
-}
-
-void HidConnectionLinux::ProcessReadQueue() {
-  while(read_queue_.size() && input_reports_.size()) {
-    PendingRequest request = read_queue_.front();
-    read_queue_.pop();
-    PendingReport report = input_reports_.front();
-    if (report.second > request.b) {
-      request.c.Run(false, report.second);
-    } else {
-      memcpy(request.a->data(), report.first->data(), report.second);
-      input_reports_.pop();
-      request.c.Run(true, report.second);
-    }
-  }
-}
 
 bool HidConnectionLinux::FindHidrawDevNode(udev_device* parent,
                                            std::string* result) {
   udev* udev = udev_device_get_udev(parent);
   if (!udev)
       return false;
 
   ScopedUdevEnumeratePtr enumerate(udev_enumerate_new(udev));
   if (!enumerate)
     return false;
 
   if (udev_enumerate_add_match_subsystem(enumerate.get(), kHidrawSubsystem)) {
     return false;
   }
   if (udev_enumerate_scan_devices(enumerate.get())) {
     return false;
   }
 
   const char* parent_path = udev_device_get_devpath(parent);
   udev_list_entry* devices = udev_enumerate_get_list_entry(enumerate.get());
   for (udev_list_entry* i = devices; i != NULL;
       i = udev_list_entry_get_next(i)) {
     ScopedUdevDevicePtr hid_dev(
         udev_device_new_from_syspath(udev, udev_list_entry_get_name(i)));
     const char* raw_path = udev_device_get_devnode(hid_dev.get());
     if (strncmp(parent_path,
                 udev_device_get_devpath(hid_dev.get()),
                 strlen(parent_path)) == 0 &&

[Mark Mentovai, 2014/02/26 17:41:44]

Compute strlen(parent_path) outside this loop.

But: is this supposed to find paths that are children of one another on the
filesystem? If so, you should make sure that parent_path ends in a slash. If
it’s not guaranteed to, you should build a new string that does end in a slash
for the purposes of this comparison. If the path isn’t supposed to be a child
from a filesystem perspective, ignore this part of this comment.

[Ken Rockot(use gerrit already), 2014/02/26 22:36:39]

Done.

         raw_path) {
       *result = raw_path;
       return true;
     }
   }
 
   return false;
 }
 
 }  // namespace device