[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..4ca4ac096b0f3728c9ce0db72aff0088a99d6630 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,14 +14,17 @@
 
 #include "base/bind.h"
 #include "base/file_path.h"
+#include "base/memory/scoped_generic_obj.h"
+#include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
-#include "base/system_monitor/system_monitor.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/media_gallery/media_device_notifications_utils.h"
 
 namespace {
 
+using base::SystemMonitor;
+
 // List of file systems we care about.
 const char* const kKnownFileSystems[] = {
   "ext2",
@@ -35,6 +39,140 @@ const char* const kKnownFileSystems[] = {
   "vfat",
 };
 
+// Device property constants.

[Lei Zhang, 2012/08/10 10:26:33]

nit: you may want to separate the udev constants from the delimiters.

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

Done.

+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 kSerial[] = "ID_SERIAL";
+const char kSerialShort[] = "ID_SERIAL_SHORT";
+const char kSpaceDelim[] = " ";
+const char kUnderscoreDelim[] = "_";
+const char kVendor[] = "ID_VENDOR";
+const char kVendorID[] = "ID_VENDOR_ID";
+
+// Unique id prefix constants.
+const char kMtpDeviceUniqueIdPrefix[] = "mtp: ";

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

nit: I wouldn't put a space in these

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

Done.

+const char kNativeDeviceUniqueIdPrefix[] = "uuid: ";
+
+// Device mount point details.
+struct MountPointEntryInfo {
+  MountPointEntryInfo() : entry_pos(-1) {}

[Lei Zhang, 2012/08/10 10:26:33]

you don't use this ctor anywhere?

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

Removed.

+
+  MountPointEntryInfo(const std::string& mount_point, int pos)
+      : mount_point(mount_point),
+        entry_pos(pos) {
+  }
+
+  // Device mount point.

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

nit: comments here are a bit redundant

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

Done.

+  std::string mount_point;
+
+  // Entry position in mtab file.
+  int entry_pos;
+};
+
+// ScopedGenericObj functor for UdevObjectRelease().
+class ScopedReleaseUdevObject {
+ public:
+  void operator()(struct udev* udev) const {
+    udev_unref(udev);
+  }
+};
+
+// ScopedGenericObj functor for UdevDeviceObjectRelease().
+class ScopedReleaseUdevDeviceObject {
+ public:
+  void operator()(struct udev_device* device) const {
+    udev_device_unref(device);
+  }
+};
+
+// Get the device information using udev library.
+// Returns true on success, false on failure.

[Lei Zhang, 2012/08/10 10:26:33]

On success, returns true and fill in |id| and |name|.

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

Done.

+bool GetDeviceInfo(const std::string& device_path,
+                   SystemMonitor::MediaDeviceType media_device_type,
+                   std::string* id,
+                   string16* name) {
+  DCHECK(!device_path.empty());
+
+  ScopedGenericObj<struct udev*, ScopedReleaseUdevObject> udev_ptr(udev_new());
+  CHECK(udev_ptr.get());
+
+  struct stat st;
+  if (stat(device_path.c_str(), &st) < 0)
+    return false;
+
+  char device_type;
+  if (S_ISCHR(st.st_mode))
+    device_type = 'c';
+  else if (S_ISBLK(st.st_mode))
+    device_type = 'b';
+  else
+    return false;  // Not a supported type.
+
+  ScopedGenericObj<struct udev_device*, ScopedReleaseUdevDeviceObject>
+      device(udev_device_new_from_devnum(udev_ptr, device_type, st.st_rdev));
+  if (!device.get())
+    return false;
+
+  // Construct a device name using label or manufacturer(vendor and model)
+  // details.
+  std::string device_label;
+  const char* device_name = NULL;
+  if ((device_name = udev_device_get_property_value(device, kLabel)) ||
+      (device_name = udev_device_get_property_value(device, kSerial))) {
+    device_label = device_name;
+  } else {
+    // Format: VendorInfo ModelInfo
+    // Eg: KnCompany Model2010

[Lei Zhang, 2012/08/10 10:26:33]

nit: E.g.

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

Done.

+    const char *vendor_name = NULL, *model_name = NULL;

[Lei Zhang, 2012/08/10 10:26:33]

If you put these on two lines, you can avoid violating the style guide. You can
declare |model_name| a few lines down where it's actually used.

Ditto below in the next section with the same pattern.

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

Done.

+    if ((vendor_name = udev_device_get_property_value(device, kVendor)))
+      device_label = vendor_name;
+    if ((model_name = udev_device_get_property_value(device, kModel))) {
+      if (!device_label.empty())
+        device_label += kSpaceDelim;
+      device_label += model_name;
+    }
+  }
+  *name = UTF8ToUTF16(device_label);

[Lei Zhang, 2012/08/10 10:26:33]

Shall we CHECK(IsStringUTF8(device_label)) and see if we get any crashes? I
don't know if udev actually guarantees it's UTF-8.

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

Done.

+
+  // Construct a unique id using fs uuid or manufacturer(vendor and model)
+  // details. Add a schema as a prefix to the unique id.
+  switch (media_device_type) {

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

It's not the device type that defines the schema, but how we would be able to
look it up...

[Lei Zhang, 2012/08/10 10:26:33]

This implies we will be adding mtp device handling code to this class too. I'm
concerned when we do that, this class will get very complicated.

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

What vandebo@ meant is to add "UUID, VID, MID, SSID" prefix so that we know what
type of id we are using for the specified device.

E.g.:
(1) If we use FS_UUID as device unique id, we will have UUID:FFF0-000F
(2) If we use Vendor ID as device unique id, we will have VID:A091
Similary for model id and serial short id we will have MID:W23E and
SSID:QWE123123

This will be helpful while checking whether the device with an id exists or not.
We can easily identify the property from which the device id is constructed.

+    case SystemMonitor::TYPE_PATH:
+      *id = kNativeDeviceUniqueIdPrefix;
+      break;
+    case SystemMonitor::TYPE_MTP:
+      *id = kMtpDeviceUniqueIdPrefix;
+      break;
+  }
+
+  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 += kUnderscoreDelim;
+      *id += model;
+    }
+    if ((serial_short = udev_device_get_property_value(device, kSerialShort))) {
+      if (!id->empty())
+        *id += kUnderscoreDelim;
+      *id += serial_short;
+    }
+  }
+
+  return true;
+}
+
 }  // namespace
 
 namespace chrome {
@@ -46,13 +184,19 @@ MediaDeviceNotificationsLinux::MediaDeviceNotificationsLinux(
     const FilePath& path)
     : initialized_(false),
       mtab_path_(path),
-      current_device_id_(0U) {
+      get_device_info_func_(&GetDeviceInfo) {
   CHECK(!path.empty());
+  PopulateKnownFileSystemsInfo();
+}
 
-  // Put |kKnownFileSystems| in std::set to get O(log N) access time.
-  for (size_t i = 0; i < arraysize(kKnownFileSystems); ++i) {
-    known_file_systems_.insert(kKnownFileSystems[i]);
-  }
+MediaDeviceNotificationsLinux::MediaDeviceNotificationsLinux(
+    const FilePath& path,
+    GetDeviceInfoFunc get_device_info_func)
+    : initialized_(false),
+      mtab_path_(path),
+      get_device_info_func_(get_device_info_func) {
+  CHECK(!path.empty());

[Lei Zhang, 2012/08/10 10:26:33]

Move this out of both ctors and put it in an Init() method? Init() can either
contain or call PopulateKnownFileSystemsInfo().

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

Done.

+  PopulateKnownFileSystemsInfo();
 }
 
 MediaDeviceNotificationsLinux::~MediaDeviceNotificationsLinux() {
@@ -81,6 +225,13 @@ void MediaDeviceNotificationsLinux::OnFilePathChanged(const FilePath& path,
   UpdateMtab();
 }
 
+void MediaDeviceNotificationsLinux::PopulateKnownFileSystemsInfo() {
+  // Put |kKnownFileSystems| in std::set to get O(log N) access time.
+  for (size_t i = 0; i < arraysize(kKnownFileSystems); ++i) {
+    known_file_systems_.insert(kKnownFileSystems[i]);
+  }
+}
+
 void MediaDeviceNotificationsLinux::InitOnFileThread() {
   DCHECK(!initialized_);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -104,125 +255,171 @@ 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.
   // These mount points must have been removed in the new mtab.
   std::vector<std::string> mount_points_to_erase;
-  for (MountMap::const_iterator it = mtab_.begin(); it != mtab_.end(); ++it) {
+  for (MountMap::const_iterator it = mount_info_map_.begin();
+       it != mount_info_map_.end(); ++it) {
     const std::string& mount_point = it->first;
-    // |mount_point| not in |new_mtab|.
-    if (!ContainsKey(new_mtab, mount_point)) {
-      const std::string& device_id = it->second.second;
-      RemoveOldDevice(device_id);
+    const std::string& mount_device = it->second.mount_device;
+    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() || (newiter->second != mount_device)) {
+      RemoveOldDevice(it->second.device_unique_id);
       mount_points_to_erase.push_back(mount_point);
     }
   }
-  // Erase the |mtab_| entries afterwards. Erasing in the loop above using the
-  // iterator is slightly more efficient, but more tricky, since calling
-  // std::map::erase() on an iterator invalidates it.
+  // Erase the |mount_info_map_| entries afterwards. Erasing in the loop above
+  // using the iterator is slightly more efficient, but more tricky, since
+  // calling std::map::erase() on an iterator invalidates it.
   for (size_t i = 0; i < mount_points_to_erase.size(); ++i)
-    mtab_.erase(mount_points_to_erase[i]);
+    mount_info_map_.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) {
     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;
-    MountMap::iterator olditer = mtab_.find(mount_point);
+    const std::string& mount_device = newiter->second;
+    MountMap::iterator olditer = mount_info_map_.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));
-      }
+    if (olditer == mount_info_map_.end()) {
+      CheckAndAddMediaDevice(mount_device, mount_point);

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

Looks like this can be cleaned up a bit now...

if (olditer != mount_info_map_.end() && mount_device ==
olditer->second.mount_device)
  continue

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

Done.

       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.mount_device)
       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;
-    }
+    // New device mounted on existing mount.
+    CheckAndAddMediaDevice(mount_device, mount_point);
   }
 }
 
-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;
   mntent entry;
   char buf[512];
-  int mount_position = 0;
-  typedef std::pair<std::string, std::string> MountPointAndId;
-  typedef std::map<std::string, MountPointAndId> DeviceMap;
+
+  // Keep track of mount point entry positions in mtab file.
+  int entry_pos = 0;
+
+  // Helper map to store the device mount point details.
+  // (mount device, MountPointEntryInfo)
+  typedef std::map<std::string, MountPointEntryInfo> DeviceMap;
   DeviceMap device_map;
   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);
-    if (it == device_map.end()) {
-      device_map.insert(std::make_pair(entry.mnt_fsname, mount_point_and_id));
-    } else {
-      it->second = mount_point_and_id;
-    }
+    // of the entry positions in |entry_info| and use that below to resolve
+    // multiple devices mounted at the same mount point.
+    const std::string& mount_device = entry.mnt_fsname;
+    const std::string& mount_point = entry.mnt_dir;
+    MountPointEntryInfo entry_info(mount_point, entry_pos++);
+    DeviceMap::iterator it = device_map.find(mount_device);

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

In either case below, you set the same value... seems like
device_map[mount_device] = entry_info; would be simpler.

[Lei Zhang, 2012/08/10 10:26:33]

google3 has a preference to not use map's [] operator. Apparently
util/gtl/map-util.h explains why, though I don't see it in the external version
here. {A} InsertOrUpdate() in {A} looks rather useful. Maybe we should leave
this as is, with a TODO to push map_util.h into base/ and then use
InsertOrUpdate() all over the place in this file later?

{A}
http://code.google.com/p/protobuf/source/browse/trunk/src/google/protobuf/stu...

[vandebo (ex-Chrome), 2012/08/10 17:25:10]

I audited base and found only six uses of insert on a map in four files.  Two of
those uses actually want to know if the value was already present and one was
added by you.  That's in contrast to the 47 occurrences of "std::map" in base. 
So the predominate style seems to be to use [] and not insert.

I looked at the reasons in map-util.h and none of them apply in this particular
context.  If you think Chrome should be using the insert syntax, it should
probably be a considered decision (chromium-dev) and appropriate support
routines added.

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

Done.

+    if (it == device_map.end())
+      device_map.insert(std::make_pair(mount_device, entry_info));
+    else
+      it->second = entry_info;
   }
   endmntent(fp);
 
+  // Helper map to store  mount point entries.
+  // (mount point, entry position in mtab file)
+  typedef std::map<std::string, int> MountPointsInfoMap;
+  MountPointsInfoMap mount_points_info_map;
+  // Handle mount point collisions.
   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_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 std::string& mount_point = device_it->second.mount_point;
+    const int entry_pos = device_it->second.entry_pos;
+    // Add entries, but overwrite entries for the same mount point. Keep track
+    // of the entry positions and use that information to resolve multiple
+    // devices mounted at the same mount point.
+    MountPointsInfoMap::iterator mount_position_it =
+        mount_points_info_map.find(mount_point);
+    if (mount_position_it == mount_points_info_map.end()) {
+      // The current entry is newer, update the mount point entry.
+      mount_points_info_map.insert(std::make_pair(mount_point, entry_pos));
+    } else {
+      // 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)
+        mount_position_it->second = entry_pos;
     }
+  }
+
+  MountPointDeviceMap& new_mtab = *mtab;
+  for (DeviceMap::const_iterator device_it = device_map.begin();
+       device_it != device_map.end();
+       ++device_it) {
+    const std::string& mount_point = device_it->second.mount_point;
+    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.
-    std::string& existing_device = mount_it->second.first;
-    std::string& existing_position = mount_it->second.second;
-    if (existing_position > position)
+    // Check to see if the mount point entry is the latest one.
+    if (mount_position_it->second != device_it->second.entry_pos)
       continue;
 
-    // The current entry is newer, update the mount point entry.
-    existing_device = device;
-    existing_position = position;
+    // No device at |mount_point|, save |device| to it.
+    CHECK(new_mtab.find(mount_point) == new_mtab.end());
+    new_mtab.insert(std::make_pair(mount_point, device_it->first));
   }
 }
 
-void MediaDeviceNotificationsLinux::AddNewDevice(
+void MediaDeviceNotificationsLinux::CheckAndAddMediaDevice(
     const std::string& mount_device,
-    const std::string& mount_point,
-    std::string* device_id) {
-  *device_id = base::IntToString(current_device_id_++);
+    const std::string& mount_point) {
+  if (!IsMediaDevice(mount_point))
+    return;
+
+  // TODO(kmadhusu): Handle other device types.
+  SystemMonitor::MediaDeviceType device_type = SystemMonitor::TYPE_PATH;
+  std::string device_id;
+  string16 device_name;
+  bool result = (*get_device_info_func_)(mount_device, device_type, &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",
+                        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;
+
+  MountMap::iterator mount_info_it = mount_info_map_.find(mount_point);
+  MountDeviceAndId mount_device_and_id(mount_device, device_id);

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

why not mount_info_map_[mount_point] = mount_device_and_id;

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

Done.

+  if (mount_info_it != mount_info_map_.end()) {
+      mount_info_it->second = mount_device_and_id;
+  } else {
+    mount_info_map_.insert(std::make_pair(mount_point, mount_device_and_id));
+  }
+
   base::SystemMonitor* system_monitor = base::SystemMonitor::Get();
-  system_monitor->ProcessMediaDeviceAttached(*device_id,
-                                             UTF8ToUTF16(mount_device),
+  system_monitor->ProcessMediaDeviceAttached(device_id,
+                                             device_name,
                                              SystemMonitor::TYPE_PATH,
                                              mount_point);
 }