Monitor udev for hidraw devices instead of hid devices.

device/hid/hid_service_linux.cc

Index: device/hid/hid_service_linux.cc
diff --git a/device/hid/hid_service_linux.cc b/device/hid/hid_service_linux.cc
index 470afa82a4d4231f3cb4a5e96f7122d474e8b1e9..1d6e4284ce5e394b0a1e940f657635a88e3cff38 100644
--- a/device/hid/hid_service_linux.cc
+++ b/device/hid/hid_service_linux.cc
@@ -27,8 +27,7 @@ namespace device {
 
 namespace {
 
-const char kHIDSubSystem[] = "hid";
-const char kHidrawSubsystem[] = "hidraw";
+const char kHidrawSubSystem[] = "hidraw";

[mschilder1, 2014/07/17 01:52:38]

as const, should this be kHIDRAWSUBSYSTEM?

[Reilly Grant (use Gerrit), 2014/07/17 03:22:53]

Google C++ style says kHidrawSubSystem (or kHidrawSubsystem) and Chromium style
says HIDRAW_SUBSYSTEM. I've seen the Google style in a bunch of places so for
consistency I think this should be left as is.

 const char kHIDID[] = "HID_ID";
 const char kHIDName[] = "HID_NAME";
 const char kHIDUnique[] = "HID_UNIQ";
@@ -53,11 +52,7 @@ scoped_refptr<HidConnection> HidServiceLinux::Connect(
           device_info.device_id);
 
   if (device) {
-    std::string dev_node;
-    if (!FindHidrawDevNode(device.get(), &dev_node)) {
-      LOG(ERROR) << "Cannot open HID device as hidraw device.";
-      return NULL;
-    }
+    std::string dev_node = udev_device_get_devnode(device.get());
     return new HidConnectionLinux(device_info, dev_node);
   }
 
@@ -77,7 +72,7 @@ void HidServiceLinux::OnDeviceAdded(udev_device* device) {
   if (!device_path)
     return;
   const char* subsystem = udev_device_get_subsystem(device);
-  if (!subsystem || strcmp(subsystem, kHIDSubSystem) != 0)
+  if (!subsystem || strcmp(subsystem, kHidrawSubSystem) != 0)
     return;
 
   HidDeviceInfo device_info;
@@ -86,7 +81,12 @@ void HidServiceLinux::OnDeviceAdded(udev_device* device) {
   uint32_t int_property = 0;
   const char* str_property = NULL;
 
-  const char* hid_id = udev_device_get_property_value(device, kHIDID);
+  udev_device *parent = udev_device_get_parent(device);
+  if (!parent) {

[mschilder1, 2014/07/17 01:52:38]

inconsistent if(){ expr; } vs. if () expr; in this file / method.
What do the rules say again?
Certainly not tolerate inconsistency ;-)

[Reilly Grant (use Gerrit), 2014/07/17 03:22:53]

The rules say it is my choice. I dislike inconsistency but also have a strong
inclination to avoid single-line ifs.

+    return;
+  }
+
+  const char* hid_id = udev_device_get_property_value(parent, kHIDID);
   if (!hid_id)
     return;
 
@@ -104,21 +104,16 @@ void HidServiceLinux::OnDeviceAdded(udev_device* device) {
     device_info.product_id = int_property;
   }
 
-  str_property = udev_device_get_property_value(device, kHIDUnique);
+  str_property = udev_device_get_property_value(parent, kHIDUnique);
   if (str_property != NULL)
     device_info.serial_number = str_property;
 
-  str_property = udev_device_get_property_value(device, kHIDName);
+  str_property = udev_device_get_property_value(parent, kHIDName);
   if (str_property != NULL)
     device_info.product_name = str_property;
 
-  std::string dev_node;
-  if (!FindHidrawDevNode(device, &dev_node)) {
-    LOG(ERROR) << "Cannot find device node for HID device.";
-    return;
-  }
-
-  int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
+  const std::string dev_node = udev_device_get_devnode(device);
+  const int flags = base::File::FLAG_OPEN | base::File::FLAG_READ;
 
   base::File device_file(base::FilePath(dev_node), flags);
   if (!device_file.IsValid()) {
@@ -162,44 +157,4 @@ void HidServiceLinux::OnDeviceRemoved(udev_device* device) {
     RemoveDevice(device_path);
 }
 
-bool HidServiceLinux::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;
-  }
-  std::string parent_path(udev_device_get_devpath(parent));
-  if (parent_path.length() == 0 || *parent_path.rbegin() != '/')
-    parent_path += '/';
-  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());
-    std::string device_path = udev_device_get_devpath(hid_dev.get());
-    if (raw_path &&
-        !device_path.compare(0, parent_path.length(), parent_path)) {
-      std::string sub_path = device_path.substr(parent_path.length());
-      if (sub_path.substr(0, sizeof(kHidrawSubsystem) - 1) ==
-          kHidrawSubsystem) {
-        *result = raw_path;
-        return true;
-      }
-    }
-  }
-
-  return false;
-}
-
 }  // namespace device