[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..439eb8129122f5b08ac3aa26bef02fc4e5b818d1 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,8 +14,10 @@
 
 #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/string_util.h"
 #include "base/system_monitor/system_monitor.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/media_gallery/media_device_notifications_utils.h"
@@ -35,6 +38,103 @@ 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,

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

nit: device_path

[kmadhusu, 2012/08/09 23:41:29]

Done.

+                         std::string* id,
+                         string16* name) {
+  DCHECK(!dev_path.empty());
+
+  // libudev-related items.

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

remove comment (what, not why)

[kmadhusu, 2012/08/09 23:41:29]

Done.

+  udev* udev_ptr = udev_new();

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

This should be a scoped object.  If there isn't already a helper in out udev
code you can use ScopedGenericObj

[kmadhusu, 2012/08/09 23:41:29]

Done. Thanks for pointing out ScopedGenericObj.

+  CHECK(udev_ptr);
+
+  // Get device file status.

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

remove comment

[kmadhusu, 2012/08/09 23:41:29]

Done.

+  struct stat st;
+  if (stat(dev_path.c_str(), &st) < 0) {
+    udev_unref(udev_ptr);
+    return false;
+  }
+
+  // Identify the device type.

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

remove comment

[kmadhusu, 2012/08/09 23:41:29]

Done.

+  char dev_type;

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

nit: consistent naming -> device_type

[kmadhusu, 2012/08/09 23:41:29]

Done.

+  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.

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

remove comment

[kmadhusu, 2012/08/09 23:41:29]

Done.

+  udev_device* device = udev_device_new_from_devnum(udev_ptr, dev_type,

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

Same here

[kmadhusu, 2012/08/09 23:41:29]

Done.

+                                                    st.st_rdev);
+  if (!device) {
+    udev_unref(udev_ptr);
+    return false;
+  }
+
+  // Construct a device name using label or manufacturer(vendor and model)

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

Name should be something that helps the user understand what device we're
talking about.  So we might want to pass type into this function and do
different things for different types.  i.e. for MTP devices or even removable
devices, the device manufacturer and model is probably a good name.  But for
things that are usually mounted, then the path is probably the best "name" for
the device.

[kmadhusu, 2012/08/09 23:41:29]

Label, Serial, Vendor and Model properties return manufacturer details which
very much helps the user to identify the device. 

Sample usb device properties:
ID_VENDOR=Kingston
ID_MODEL=DataTraveler_2.0
ID_SERIAL=Kingston_DataTraveler_2.0_89900000000000006CB02CDB-0:0
ID_FS_LABEL=KINGSTON

+  // details.
+  std::string device_name;
+  const char* dev_name = NULL;

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

nit: device_name

[kmadhusu, 2012/08/09 23:41:29]

Done.

+  if ((dev_name = udev_device_get_property_value(device, kLabel)) ||
+      (dev_name = udev_device_get_property_value(device, kSerial))) {

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

what are the ownership semantics of udev_device_get_property_value?  i.e. does
the result need to be freed?

[kmadhusu, 2012/08/10 19:41:01]

I ran Chromium under ASAN and I did not find any memory leaks. So this code is
fine.

+    device_name = dev_name;
+  } else {
+    // Format: VendorInfo_ModelInfo
+    // Eg: KnCompany_Model2010

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

why an underscore?  name is for presentation to humans, so a space would be more
appropriate.

[kmadhusu, 2012/08/09 23:41:29]

No specific reason to use underscore. As we discussed, changed the separator to
space.

+    const char *vendor_name = NULL, *model_name = NULL;
+    if ((vendor_name = udev_device_get_property_value(device, kVendor)))
+      device_name = vendor_name;
+    if ((model_name = udev_device_get_property_value(device, kModel))) {
+      if (!device_name.empty())
+        device_name += kSeperator;
+      device_name += model_name;
+    }
+  }
+  *name = UTF8ToUTF16(device_name);
+
+  // Construct a unique id using fs uuid or manufacturer(vendor and model)
+  // details.

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

We'll eventually want to have a function that given this id will tell us if the
device is attached to the system.  So you might want to add a schema to the
start of the id string. i.e. uuid: mtp: etc.

[kmadhusu, 2012/08/09 23:41:29]

Done.

+  const char* uuid  = NULL;
+  if ((uuid = udev_device_get_property_value(device, 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(device, kVendorID)))
+      *id = vendor;
+
+    if ((model = udev_device_get_property_value(device, kModelID))) {
+      if (!id->empty())
+        *id += kSeperator;
+      *id += model;
+    }
+    if ((serial_short = udev_device_get_property_value(device, kSerialShort))) {
+      if (!id->empty())
+        *id += kSeperator;
+      *id += serial_short;
+    }
+  }
+
+  udev_unref(udev_ptr);
+  udev_device_unref(device);
+  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,12 @@ void MediaDeviceNotificationsLinux::OnFilePathChanged(const FilePath& path,
   UpdateMtab();
 }
 
+bool MediaDeviceNotificationsLinux::GetDeviceInfo(const std::string& dev_path,
+                                                  std::string* id,
+                                                  string16* name) {
+  return GetDeviceInfoHelper(dev_path, id, name);
+}
+
 void MediaDeviceNotificationsLinux::InitOnFileThread() {
   DCHECK(!initialized_);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -104,7 +209,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 +217,12 @@ 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 at
+    // |mount_point|.
+    if (newiter == new_mtab.end() ||
+        base::strcasecmp(newiter->second.c_str(), mount_device.c_str()) != 0) {

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

When I use "==", the unit tests are failing. So, I added base::strcasecmp()
check.

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

case should be consistent, so lets understand what's going wrong instead of
working around it.

[kmadhusu, 2012/08/09 23:41:29]

I made a silly mistake in patch 6 that made the tests to fail. Removed
strcasecmp call and use the comparison operator.

       const std::string& device_id = it->second.second;
       RemoveOldDevice(device_id);
       mount_points_to_erase.push_back(mount_point);
@@ -126,92 +235,98 @@ 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;

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

Move this to just before line 248 and just before line 261.  They don't share
the value and we like to declare things just before we use them.

Actually later comment makes this one moot.

[kmadhusu, 2012/08/09 23:41:29]

Done.

     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);

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

Seems like AddNewDevice should be responsible for adding it into mtab_.  That
way you don't need to pop device_id back up to this scope.

[kmadhusu, 2012/08/09 23:41:29]

Done.

+        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;
+  // Helper map to store the device mount point details.
+  // (mount device, (mount point, entry position in mtab file))
+  typedef std::map<std::string,
+                   std::pair<std::string, int> > DeviceMap;

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

don't use a pair, use a struct

[kmadhusu, 2012/08/09 23:41:29]

Done.

+  DeviceMap device_map;

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

Move declaration of device_map to just before while loop.

[kmadhusu, 2012/08/09 23:41:29]

Done.

+
+  MountPointDeviceMap& new_mtab = *mtab;

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

Move new_mtab declaration to just before the for loop.

[kmadhusu, 2012/08/09 23:41:29]

Done.

   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

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

We probably want to keep the comment about overwriting earlier entries with
later ones.

[kmadhusu, 2012/08/09 23:41:29]

Done.

-    // 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);
 
+  // Helper map to store  mount point entries.
+  // (mount point, entry position in mtab file)
+  typedef std::map<std::string, int> MountPointsInfoMap;

[vandebo (ex-Chrome), 2012/08/09 17:52:35]

To make things simpler, I'd suggest pulling this loop apart into two loops.  The
first one would resolve the mount point collisions and the second would just
populate the new_mtab.

At the least, it's worth adding a comment here saying what we're going to do....
We need to resolve mount entries that mount different devices to the same mount
point where last mount wins.

[kmadhusu, 2012/08/09 23:41:29]

Done.

[vandebo (ex-Chrome), 2012/08/10 00:32:58]

I guess it wasn't simpler.

[kmadhusu, 2012/08/10 19:41:01]

As we discussed, reverted to my previous version of code.

+  MountPointsInfoMap mount_points_info_map;
+
   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_mtab_it = new_mtab.find(mount_point);
+    if (new_mtab_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 mount_position_it =
+          mount_points_info_map.find(mount_point);
+      DCHECK(mount_position_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 (mount_position_it->second < entry_pos) {
+        // The current entry is newer, update the mount point entry.
+        mount_position_it->second = entry_pos;
+        new_mtab_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 +334,28 @@ 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;
+  bool result = GetDeviceInfo(mount_device, device_id, &device_name);
+
+  // Keep track of GetDeviceInfo result, to see how often we fail to get device
+  // details.
+  UMA_HISTOGRAM_BOOLEAN("MediaDeviceNotification.device_info_available",

[vandebo (ex-Chrome), 2012/08/09 17:54:47]

Can we distinguish these from the CrOS ones in the dashboard?  If not, we should
give them a different name.

[kmadhusu, 2012/08/09 23:41:29]

We can use the same name. While viewing, we can filter the platform specific
results in the dashboard.

+                        result);
+  if (!result)
+    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);
 }