[LINUX] Extract the name and id of the device and send it along the device attach message.

chrome/browser/media_gallery/media_device_notifications_linux.cc

Index: chrome/browser/media_gallery/media_device_notifications_linux.cc
diff --git a/chrome/browser/media_gallery/media_device_notifications_linux.cc b/chrome/browser/media_gallery/media_device_notifications_linux.cc
index e4d61621989cc055d98b4231c146c988f87f2bfd..91fc1e85f0f31fdd4944a391e4246260688e0688 100644
--- a/chrome/browser/media_gallery/media_device_notifications_linux.cc
+++ b/chrome/browser/media_gallery/media_device_notifications_linux.cc
@@ -6,6 +6,7 @@
 
 #include "chrome/browser/media_gallery/media_device_notifications_linux.h"
 
+#include <libudev.h>
 #include <mntent.h>
 #include <stdio.h>
 
@@ -13,6 +14,7 @@
 
 #include "base/bind.h"
 #include "base/file_path.h"
+#include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/system_monitor/system_monitor.h"
@@ -35,6 +37,104 @@ const char* const kKnownFileSystems[] = {
   "vfat",
 };
 
+// Device property constants.
+const char kDevName[] = "DEVNAME";
+const char kFsUUID[] = "ID_FS_UUID";
+const char kLabel[] = "ID_FS_LABEL";
+const char kModel[] = "ID_MODEL";
+const char kModelID[] = "ID_MODEL_ID";
+const char kSeperator[] = "_";
+const char kSerial[] = "ID_SERIAL";
+const char kSerialShort[] = "ID_SERIAL_SHORT";
+const char kVendor[] = "ID_VENDOR";
+const char kVendorID[] = "ID_VENDOR_ID";
+
+// Get the device information using udev library.
+// Returns true on success, false on failure.
+bool GetDeviceInfoHelper(const std::string& dev_path,
+                         std::string* id,
+                         string16* name) {
+  DCHECK(!dev_path.empty());
+
+  // libudev-related items.
+  udev* udev_ptr;
+  udev_ptr = udev_new();

[Lei Zhang, 2012/08/09 03:47:16]

nit: combine with previous line.

[kmadhusu, 2012/08/09 08:10:03]

Done.

+  CHECK(udev_ptr);
+
+  // Get device file status.
+  struct stat st;
+  if (stat(dev_path.c_str(), &st) == -1) {

[Lei Zhang, 2012/08/09 03:47:16]

usually people do < 0

[kmadhusu, 2012/08/09 08:10:03]

Done.

+    udev_unref(udev_ptr);
+    return false;
+  }
+
+  // Identify the device type.
+  char dev_type;
+  if (S_ISCHR(st.st_mode))
+    dev_type = 'c';
+  else if (S_ISBLK(st.st_mode))
+    dev_type = 'b';
+  else
+    return false;  // Not a supported type.
+
+  // Create a new udev_device object from device ID.
+  udev_device* dev = udev_device_new_from_devnum(udev_ptr, dev_type,

[Lei Zhang, 2012/08/09 03:47:16]

Can you name this "device"? It's impossible to search for "dev" in this file. :)

[kmadhusu, 2012/08/09 08:10:03]

Done.

+                                                 st.st_rdev);
+  if (!dev) {
+    udev_unref(udev_ptr);
+    return false;
+  }
+
+  // Construct a device name using label or manufacturer(vendor and model)
+  // details.
+  std::string device_name;
+  const char* dev_name = NULL;
+  if ((dev_name = udev_device_get_property_value(dev, kLabel)) ||
+      (dev_name = udev_device_get_property_value(dev, kSerial))) {
+    device_name = dev_name;
+  } else {
+    // Format: VendorInfo_ModelInfo
+    // Eg: KnCompany_Model2010
+    const char *vendor_name = NULL, *model_name = NULL;

[Lei Zhang, 2012/08/09 03:47:16]

nit: char*

[kmadhusu, 2012/08/09 08:10:03]

Since I am doing multiple declarations on a single statement, I prefer this
style. 

+    if ((vendor_name = udev_device_get_property_value(dev, kVendor)))

[Lei Zhang, 2012/08/09 03:47:16]

nit: extra set of () here and below?

[kmadhusu, 2012/08/09 08:10:03]

Since I am using the result value of the expression as a truth value, compiler
complains if I dont add extra ().

  "error: suggest parentheses around assignment used as truth value"

+      device_name = vendor_name;
+    if ((model_name = udev_device_get_property_value(dev, kModel))) {
+      if (!device_name.empty())
+        device_name.append(kSeperator);

[Lei Zhang, 2012/08/09 03:47:16]

still got a bunch of appends.

[kmadhusu, 2012/08/09 08:10:03]

Done. I didn't find enough reference to state that operator"+=" or "+" is better
than appends. If you think we should not use assigns and appends in the code,
may be you should add a note in the c++ coding style guide with a valid reason.

+      device_name.append(model_name);
+    }
+  }
+  *name = UTF8ToUTF16(device_name);
+
+  // Construct a unique id using fs uuid or manufacturer(vendor and model)
+  // details.
+  const char* uuid  = NULL;
+  if ((uuid = udev_device_get_property_value(dev, kFsUUID))) {
+    *id = uuid;
+  } else {
+    // Format: VendorInfo_ModelInfo_SerialShortInfo
+    // Eg: Kn_DataTravel_12.10_8000000000006CB02CDB
+    const char *vendor = NULL, *model = NULL, *serial_short = NULL;
+    if ((vendor = udev_device_get_property_value(dev, kVendorID)))
+      *id = vendor;
+
+    if ((model = udev_device_get_property_value(dev, kModelID))) {
+      if (!id->empty())
+        id->append(kSeperator);
+      id->append(model);
+    }
+    if ((serial_short = udev_device_get_property_value(dev, kSerialShort))) {
+      if (!id->empty())
+        id->append(kSeperator);
+      id->append(serial_short);
+    }
+  }
+
+  udev_unref(udev_ptr);
+  udev_device_unref(dev);
+  return true;
+}
+
 }  // namespace
 
 namespace chrome {
@@ -45,8 +145,7 @@ using content::BrowserThread;
 MediaDeviceNotificationsLinux::MediaDeviceNotificationsLinux(
     const FilePath& path)
     : initialized_(false),
-      mtab_path_(path),
-      current_device_id_(0U) {
+      mtab_path_(path) {
   CHECK(!path.empty());
 
   // Put |kKnownFileSystems| in std::set to get O(log N) access time.
@@ -81,6 +180,15 @@ void MediaDeviceNotificationsLinux::OnFilePathChanged(const FilePath& path,
   UpdateMtab();
 }
 
+bool MediaDeviceNotificationsLinux::GetDeviceInfo(const std::string& dev_path,
+                                                  std::string* id,
+                                                  string16* name) {
+  if (dev_path.empty())
+    return false;
+
+  return GetDeviceInfoHelper(dev_path, id, name);

[Lei Zhang, 2012/08/09 03:47:16]

I don't understand why you need to keep udev out of
MediaDeviceNotificationsLinux, but if you do, then just reduce this method to a
1-liner.

[kmadhusu, 2012/08/09 08:10:03]

Done.

+}
+
 void MediaDeviceNotificationsLinux::InitOnFileThread() {
   DCHECK(!initialized_);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -104,7 +212,7 @@ void MediaDeviceNotificationsLinux::InitOnFileThread() {
 void MediaDeviceNotificationsLinux::UpdateMtab() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-  MountMap new_mtab;
+  MountPointDeviceMap new_mtab;
   ReadMtab(&new_mtab);
 
   // Check existing mtab entries for unaccounted mount points.
@@ -112,8 +220,11 @@ void MediaDeviceNotificationsLinux::UpdateMtab() {
   std::vector<std::string> mount_points_to_erase;
   for (MountMap::const_iterator it = mtab_.begin(); it != mtab_.end(); ++it) {
     const std::string& mount_point = it->first;
-    // |mount_point| not in |new_mtab|.
-    if (!ContainsKey(new_mtab, mount_point)) {
+    const std::string& mount_device = it->second.first;
+    MountPointDeviceMap::iterator newiter = new_mtab.find(mount_point);
+    // |mount_point| not in |new_mtab| or |mount_device| is no longer mounted in

[Lei Zhang, 2012/08/09 03:47:16]

nit: mounted in -> mounted at

[kmadhusu, 2012/08/09 08:10:03]

Done.

+    // |mount_point|.
+    if (newiter == new_mtab.end() || newiter->second == mount_device) {
       const std::string& device_id = it->second.second;
       RemoveOldDevice(device_id);
       mount_points_to_erase.push_back(mount_point);
@@ -126,63 +237,74 @@ void MediaDeviceNotificationsLinux::UpdateMtab() {
     mtab_.erase(mount_points_to_erase[i]);
 
   // Check new mtab entries against existing ones.
-  for (MountMap::iterator newiter = new_mtab.begin();
+  for (MountPointDeviceMap::iterator newiter = new_mtab.begin();
        newiter != new_mtab.end();
        ++newiter) {
+    std::string device_id;
     const std::string& mount_point = newiter->first;
-    const MountDeviceAndId& mount_device_and_id = newiter->second;
-    const std::string& mount_device = mount_device_and_id.first;
-    std::string& id = newiter->second.second;
+    const std::string& mount_device = newiter->second;
     MountMap::iterator olditer = mtab_.find(mount_point);
     // Check to see if it is a new mount point.
     if (olditer == mtab_.end()) {
       if (IsMediaDevice(mount_point)) {
-        AddNewDevice(mount_device, mount_point, &id);
-        mtab_.insert(std::make_pair(mount_point, mount_device_and_id));
+        AddNewDevice(mount_device, mount_point, &device_id);
+        mtab_.insert(std::make_pair(mount_point,
+                                    std::make_pair(mount_device, device_id)));
       }
       continue;
     }
 
     // Existing mount point. Check to see if a new device is mounted there.
-    const MountDeviceAndId& old_mount_device_and_id = olditer->second;
-    if (mount_device == old_mount_device_and_id.first)
+    if (mount_device == olditer->second.first)
       continue;
 
     // New device mounted.
-    RemoveOldDevice(old_mount_device_and_id.second);
     if (IsMediaDevice(mount_point)) {
-      AddNewDevice(mount_device, mount_point, &id);
-      olditer->second = mount_device_and_id;
+      AddNewDevice(mount_device, mount_point, &device_id);
+      olditer->second = std::make_pair(mount_device, device_id);
     }
   }
 }
 
-void MediaDeviceNotificationsLinux::ReadMtab(MountMap* mtab) {
+void MediaDeviceNotificationsLinux::ReadMtab(MountPointDeviceMap* mtab) {
   FILE* fp = setmntent(mtab_path_.value().c_str(), "r");
   if (!fp)
     return;
 
-  MountMap& new_mtab = *mtab;
+  // (mount point, entry position in mtab file)
+  typedef std::pair<std::string, int> MountEntryInfo;

[Lei Zhang, 2012/08/09 03:47:16]

You can probably fold this into the next typedef, since you don't use
MountEntryInfo anywhere else.

[kmadhusu, 2012/08/09 08:10:03]

Done.

+
+  // (mount device, MountEntryInfo)
+  typedef std::map<std::string, MountEntryInfo> DeviceMap;
+
+  // (mount point, entry position in mtab file)
+  typedef std::map<std::string, int> MountPointsInfoMap;
+
+  // Helper maps to store the device mount point details and mount point
+  // entries.
+  DeviceMap device_map;
+  MountPointsInfoMap mount_points_info_map;

[Lei Zhang, 2012/08/09 03:47:16]

you can declare this and its typedef further below, closer to where it's
actually used.

[kmadhusu, 2012/08/09 08:10:03]

Done.

+
+  MountPointDeviceMap& new_mtab = *mtab;
   mntent entry;
   char buf[512];
-  int mount_position = 0;
-  typedef std::pair<std::string, std::string> MountPointAndId;
-  typedef std::map<std::string, MountPointAndId> DeviceMap;
-  DeviceMap device_map;
+
+  // Keep track of mount point entry positions in mtab file.
+  int entry_pos = 0;
+
   while (getmntent_r(fp, &entry, buf, sizeof(buf))) {
     // We only care about real file systems.
     if (!ContainsKey(known_file_systems_, entry.mnt_type))
       continue;
-    // Add entries, but overwrite entries for the same mount device. Keep track
-    // of the entry positions in the device id field and use that below to
-    // resolve multiple devices mounted at the same mount point.
-    MountPointAndId mount_point_and_id =
-        std::make_pair(entry.mnt_dir, base::IntToString(mount_position++));
-    DeviceMap::iterator it = device_map.find(entry.mnt_fsname);
+    const std::string& mount_device = entry.mnt_fsname;
+    const std::string& mount_point = entry.mnt_dir;
+    DeviceMap::iterator it = device_map.find(mount_device);
     if (it == device_map.end()) {
-      device_map.insert(std::make_pair(entry.mnt_fsname, mount_point_and_id));
+      device_map.insert(std::make_pair(mount_device,
+                                       std::make_pair(mount_point,
+                                                      entry_pos++)));
     } else {
-      it->second = mount_point_and_id;
+      it->second = std::make_pair(mount_point, entry_pos++);
     }
   }
   endmntent(fp);
@@ -190,28 +312,25 @@ void MediaDeviceNotificationsLinux::ReadMtab(MountMap* mtab) {
   for (DeviceMap::const_iterator device_it = device_map.begin();
        device_it != device_map.end();
        ++device_it) {
-    const std::string& device = device_it->first;
+    const std::string& mount_device = device_it->first;
     const std::string& mount_point = device_it->second.first;
-    const std::string& position = device_it->second.second;
-
-    // No device at |mount_point|, save |device| to it.
-    MountMap::iterator mount_it = new_mtab.find(mount_point);
-    if (mount_it == new_mtab.end()) {
-      new_mtab.insert(std::make_pair(mount_point,
-                                     std::make_pair(device, position)));
-      continue;
+    const int entry_pos = device_it->second.second;
+    MountPointDeviceMap::iterator new_it = new_mtab.find(mount_point);

[Lei Zhang, 2012/08/09 03:47:16]

you may want to rename |new_it| and |it| to |new_mtab_it| and
|mount_position_it|, respectively.

[kmadhusu, 2012/08/09 08:10:03]

Done.

+    if (new_it == new_mtab.end()) {
+      // No device at |mount_point|, save |device| to it.
+      new_mtab.insert(std::make_pair(mount_point, mount_device));
+      mount_points_info_map.insert(std::make_pair(mount_point, entry_pos));
+    } else {
+      MountPointsInfoMap::iterator it = mount_points_info_map.find(mount_point);
+      DCHECK(it != mount_points_info_map.end());
+      // There is already a device mounted at |mount_point|. Check to see if
+      // the existing mount entry is newer than the current one.
+      if (it->second < entry_pos) {
+        // The current entry is newer, update the mount point entry.
+        it->second = entry_pos;
+        new_it->second = mount_device;
+      }
     }
-
-    // There is already a device mounted at |mount_point|. Check to see if
-    // the existing mount entry is newer than the current one.
-    std::string& existing_device = mount_it->second.first;
-    std::string& existing_position = mount_it->second.second;
-    if (existing_position > position)
-      continue;
-
-    // The current entry is newer, update the mount point entry.
-    existing_device = device;
-    existing_position = position;
   }
 }
 
@@ -219,10 +338,22 @@ void MediaDeviceNotificationsLinux::AddNewDevice(
     const std::string& mount_device,
     const std::string& mount_point,
     std::string* device_id) {
-  *device_id = base::IntToString(current_device_id_++);
+  string16 device_name;
+  if (!GetDeviceInfo(mount_device, device_id, &device_name))

[Lei Zhang, 2012/08/09 03:47:16]

You may also want to keep track of how often this fails.

[kmadhusu, 2012/08/09 08:10:03]

Done.

+    return;
+
+  // Keep track of device uuid, to see how often we receive empty values.
+  UMA_HISTOGRAM_BOOLEAN("MediaDeviceNotification.device_uuid_available",
+                        !device_id->empty());
+  UMA_HISTOGRAM_BOOLEAN("MediaDeviceNotification.device_name_available",
+                        !device_name.empty());
+
+  if (device_id->empty() || device_name.empty())
+    return;
+
   base::SystemMonitor* system_monitor = base::SystemMonitor::Get();
   system_monitor->ProcessMediaDeviceAttached(*device_id,
-                                             UTF8ToUTF16(mount_device),
+                                             device_name,
                                              SystemMonitor::TYPE_PATH,
                                              mount_point);
 }