Clean up HID backend and API.

device/hid/hid_connection_linux.cc

Index: device/hid/hid_connection_linux.cc
diff --git a/device/hid/hid_connection_linux.cc b/device/hid/hid_connection_linux.cc
index 1491ba62f929670b0e30a7eb0de1a6d4934c18cb..611e8874979ff58ec8a67e34b319b8ffb1ca195b 100644
--- a/device/hid/hid_connection_linux.cc
+++ b/device/hid/hid_connection_linux.cc
@@ -8,6 +8,8 @@
 #include <fcntl.h>
 #include <libudev.h>
 #include <linux/hidraw.h>
+#include <sys/ioctl.h>
+
 #include <string>
 
 #include "base/threading/thread_restrictions.h"
@@ -15,19 +17,36 @@
 #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();
@@ -61,11 +80,8 @@ HidConnectionLinux::HidConnectionLinux(HidDeviceInfo device_info,
       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() {
@@ -76,21 +92,21 @@ HidConnectionLinux::~HidConnectionLinux() {
 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();
 }
 
@@ -98,89 +114,91 @@ 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();
-    }
-    callback.Run(false, 0);
-    return;
-  } else {
-    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(scoped_refptr<net::IOBuffer> buffer,
-                               size_t size,
+void HidConnectionLinux::Write(uint8_t report_id,
+                               scoped_refptr<net::IOBufferWithSize> buffer,
                                const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!initialized_) {
+  // 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 {
-    int bytes = write(device_file_.GetPlatformFile(), buffer->data(), size);
-    if (bytes < 0) {
-      Disconnect();
-      callback.Run(false, 0);
-    } else {
-      callback.Run(true, bytes);
-    }
+    callback.Run(true, bytes_written);
   }
 }
 
-void HidConnectionLinux::GetFeatureReport(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;
   }
-  NOTIMPLEMENTED();
+
+  // 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(scoped_refptr<net::IOBuffer> buffer,
-                                           size_t size,
-                                           const IOCallback& callback) {
+void HidConnectionLinux::SendFeatureReport(
+    uint8_t report_id,
+    scoped_refptr<net::IOBufferWithSize> buffer,
+    const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (!initialized_) {
+  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);
-    return;
-  }
-  NOTIMPLEMENTED();
+  else
+    callback.Run(true, result);
 }
 
 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);
+  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 {
-      memcpy(request.a->data(), report.first->data(), report.second);
-      input_reports_.pop();
-      request.c.Run(true, report.second);
+      memcpy(read.buffer->data(), report.buffer->data(), report.buffer->size());
+      pending_reports_.pop();
+      read.callback.Run(true, report.buffer->size());
     }
   }
 }