Make the Linux System Monitor implementation track all devices

chrome/browser/media_gallery/removable_device_notifications_linux.cc

Index: chrome/browser/media_gallery/removable_device_notifications_linux.cc
diff --git a/chrome/browser/media_gallery/media_device_notifications_linux.cc b/chrome/browser/media_gallery/removable_device_notifications_linux.cc
similarity index 48%
rename from chrome/browser/media_gallery/media_device_notifications_linux.cc
rename to chrome/browser/media_gallery/removable_device_notifications_linux.cc
index 012636e5999c1be0ac24c4cca3a341ead52f6abd..a3516ad3fc307eb74fa2d204e605cefa56f12b82 100644
--- a/chrome/browser/media_gallery/media_device_notifications_linux.cc
+++ b/chrome/browser/media_gallery/removable_device_notifications_linux.cc
@@ -2,15 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// MediaDeviceNotificationsLinux implementation.
+// RemovableDeviceNotificationsLinux implementation.
 
-#include "chrome/browser/media_gallery/media_device_notifications_linux.h"
+#include "chrome/browser/media_gallery/removable_device_notifications_linux.h"
 
 #include <libudev.h>
 #include <mntent.h>
 #include <stdio.h>
 
-#include <vector>
+#include <list>
 
 #include "base/bind.h"
 #include "base/file_path.h"
@@ -33,6 +33,9 @@ using content::BrowserThread;
 
 namespace {
 
+static RemovableDeviceNotificationsLinux*
+    g_removable_device_notifications_linux = NULL;
+
 // List of file systems we care about.
 const char* const kKnownFileSystems[] = {
   "ext2",
@@ -53,16 +56,41 @@ 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 kRemovableSysAttr[] = "removable";
 const char kSerial[] = "ID_SERIAL";
 const char kSerialShort[] = "ID_SERIAL_SHORT";
 const char kVendor[] = "ID_VENDOR";
 const char kVendorID[] = "ID_VENDOR_ID";
 
-// Device mount point details.
-struct MountPointEntryInfo {
-  std::string mount_point;
-  int entry_pos;
-};
+// (mount point, mount device)
+// A mapping from mount point to mount device, as extracted from the mtab
+// file.
+typedef std::map<FilePath, FilePath> MountPointDeviceMap;
+
+// Reads mtab file entries into |mtab|.
+void ReadMtab(FilePath mtab_path,

[Lei Zhang, 2012/08/28 00:05:04]

Why not just keep this as a class method?

If you are going to keep it as an anonymous function, const-ref |mtab_path| and
|interesting_file_systems|.

[vandebo (ex-Chrome), 2012/08/28 00:33:32]

The type declarations in the header are pretty confusing and take a lot of
effort to understand.  By moving this to an anonymous function,
MountPoitnDeviceMap can be moved out of the header.
Done.

+              std::set<std::string> interesting_file_systems,
+              MountPointDeviceMap* mtab) {
+  FILE* fp = setmntent(mtab_path.value().c_str(), "r");
+  if (!fp)
+    return;
+
+  mtab->clear();

[Lei Zhang, 2012/08/28 00:05:04]

probably should do this first

[vandebo (ex-Chrome), 2012/08/28 00:33:32]

Done.

+
+  mntent entry;
+  char buf[512];
+
+  // We return the same device mounted to multiple locations, but hide
+  // devices that have been mounted over.
+  while (getmntent_r(fp, &entry, buf, sizeof(buf))) {
+    // We only care about real file systems.
+    if (!ContainsKey(interesting_file_systems, entry.mnt_type))
+      continue;
+
+    (*mtab)[FilePath(entry.mnt_dir)] = FilePath(entry.mnt_fsname);
+  }
+  endmntent(fp);
+}
 
 // ScopedGenericObj functor for UdevObjectRelease().
 class ScopedReleaseUdevObject {
@@ -96,8 +124,8 @@ std::string GetUdevDevicePropertyValue(struct udev_device* udev_device,
 
 // Get the device information using udev library.
 // On success, returns true and fill in |id| and |name|.

[Lei Zhang, 2012/08/28 00:05:04]

nit: comment is out of date

[vandebo (ex-Chrome), 2012/08/28 00:33:32]

Done.

-bool GetDeviceInfo(const std::string& device_path, std::string* id,
-                   string16* name) {
+bool GetDeviceInfo(const FilePath& device_path, std::string* unique_id,
+                   string16* name, bool* removable) {
   DCHECK(!device_path.empty());
 
   ScopedUdevObject udev_obj(udev_new());
@@ -105,7 +133,7 @@ bool GetDeviceInfo(const std::string& device_path, std::string* id,
     return false;
 
   struct stat device_stat;
-  if (stat(device_path.c_str(), &device_stat) < 0)
+  if (stat(device_path.value().c_str(), &device_stat) < 0)
     return false;
 
   char device_type;
@@ -149,9 +177,10 @@ bool GetDeviceInfo(const std::string& device_path, std::string* id,
 
   // Construct a device id using label or manufacturer (vendor and model)
   // details.
-  if (id) {
-    std::string unique_id = GetUdevDevicePropertyValue(device, kFsUUID);
-    if (unique_id.empty()) {
+  if (unique_id) {
+    std::string uuid = GetUdevDevicePropertyValue(device, kFsUUID);
+    *unique_id = kFSUniqueIdPrefix + uuid;

[Lei Zhang, 2012/08/28 00:05:04]

I have a feeling you are doing this to make the code more readable. How about we
put the inside of this if block in a helper function instead?

[vandebo (ex-Chrome), 2012/08/28 00:33:32]

Done.

+    if (uuid.empty()) {
       // If one of the vendor, model, serial information is missing, its value
       // in the string is empty.
       // Format: VendorModelSerial:VendorInfo:ModelInfo:SerialShortInfo
@@ -163,44 +192,63 @@ bool GetDeviceInfo(const std::string& device_path, std::string* id,
       if (vendor.empty() && model.empty() && serial_short.empty())
         return false;
 
-      unique_id = base::StringPrintf("%s%s%s%s%s%s",
-                                     kVendorModelSerialPrefix,
-                                     vendor.c_str(),
-                                     kNonSpaceDelim,
-                                     model.c_str(),
-                                     kNonSpaceDelim,
-                                     serial_short.c_str());
-    } else {
-      unique_id = kFSUniqueIdPrefix + unique_id;
+      *unique_id = base::StringPrintf("%s%s%s%s%s%s",
+                                      kVendorModelSerialPrefix,
+                                      vendor.c_str(),
+                                      kNonSpaceDelim,
+                                      model.c_str(),
+                                      kNonSpaceDelim,
+                                      serial_short.c_str());
     }
-    *id = MediaStorageUtil::MakeDeviceId(
-        MediaStorageUtil::USB_MASS_STORAGE_WITH_DCIM, unique_id);
+  }
+
+  if (removable) {
+    // |parent_device| is owned by |device| and does not need to be cleaned up.
+    struct udev_device* parent_device =
+        udev_device_get_parent_with_subsystem_devtype(device, "block", "disk");

[Lei Zhang, 2012/08/28 00:05:04]

nit: make block and disk constants or use remove below instead of
kRemovableSysAttr.

[vandebo (ex-Chrome), 2012/08/28 00:33:32]

Done.

+    const char* value = udev_device_get_sysattr_value(parent_device,
+                                                      kRemovableSysAttr);
+    *removable = false;
+    if (value && atoi(value) == 1)

[Lei Zhang, 2012/08/28 00:05:04]

*removable = (value && atoi(value) == 1)

[vandebo (ex-Chrome), 2012/08/28 00:33:32]

Done.

+      *removable = true;
   }
   return true;
 }
 
 }  // namespace
 
-MediaDeviceNotificationsLinux::MediaDeviceNotificationsLinux(
+RemovableDeviceNotificationsLinux::RemovableDeviceNotificationsLinux(
     const FilePath& path)
     : initialized_(false),
       mtab_path_(path),
       get_device_info_func_(&GetDeviceInfo) {
+  DCHECK(g_removable_device_notifications_linux == NULL);

[Lei Zhang, 2012/08/28 00:05:04]

DCHECK(!g_removable_device_notifications_linux);

[vandebo (ex-Chrome), 2012/08/28 00:33:32]

Done. And below.

+  g_removable_device_notifications_linux = this;
 }
 
-MediaDeviceNotificationsLinux::MediaDeviceNotificationsLinux(
+RemovableDeviceNotificationsLinux::RemovableDeviceNotificationsLinux(
     const FilePath& path,
     GetDeviceInfoFunc get_device_info_func)
     : initialized_(false),
       mtab_path_(path),
       get_device_info_func_(get_device_info_func) {
+  DCHECK(g_removable_device_notifications_linux == NULL);
+  g_removable_device_notifications_linux = this;
 }
 
-MediaDeviceNotificationsLinux::~MediaDeviceNotificationsLinux() {
+RemovableDeviceNotificationsLinux::~RemovableDeviceNotificationsLinux() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  DCHECK(g_removable_device_notifications_linux != NULL);

[Lei Zhang, 2012/08/28 00:05:04]

DCHECK_EQ(this, g_removable_device_notifications_linux)

[vandebo (ex-Chrome), 2012/08/28 00:33:32]

Done.

+  g_removable_device_notifications_linux = NULL;
 }
 
-void MediaDeviceNotificationsLinux::Init() {
+// static
+RemovableDeviceNotificationsLinux*
+RemovableDeviceNotificationsLinux::GetInstance() {
+  return g_removable_device_notifications_linux;
+}
+
+void RemovableDeviceNotificationsLinux::Init() {
   DCHECK(!mtab_path_.empty());
 
   // Put |kKnownFileSystems| in std::set to get O(log N) access time.
@@ -209,11 +257,66 @@ void MediaDeviceNotificationsLinux::Init() {
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
-      base::Bind(&MediaDeviceNotificationsLinux::InitOnFileThread, this));
+      base::Bind(&RemovableDeviceNotificationsLinux::InitOnFileThread, this));
+}
+
+FilePath RemovableDeviceNotificationsLinux::GetDeviceMountPoint(
+    const std::string& device_id) const {
+
+  MediaStorageUtil::Type type;
+  MediaStorageUtil::CrackDeviceId(device_id, &type, NULL);
+  if (type == MediaStorageUtil::USB_MTP)
+    return FilePath();
+
+  FilePath mount_device = FilePath();

[Lei Zhang, 2012/08/28 00:05:04]

you don't need to init a FilePath to an empty FilePath.

[vandebo (ex-Chrome), 2012/08/28 00:33:32]

Done.

+  for (MountMap::const_iterator it = mount_info_map_.begin();
+       it != mount_info_map_.end();
+       ++it) {
+    if (it->second.device_id == device_id) {
+      mount_device = it->second.mount_device;
+      break;
+    }
+  }
+  if (mount_device.empty())
+    return mount_device;
+
+  const ReferencedMountPoint& referenced_info =
+      mount_priority_map_.find(mount_device)->second;
+  for (ReferencedMountPoint::const_iterator it = referenced_info.begin();
+       it != referenced_info.end();
+       ++it) {
+    if (it->second)
+      return it->first;
+  }
+  // If none of them are default, just return the first.
+  return FilePath(
+      mount_priority_map_.find(mount_device)->second.begin()->first);
 }
 
-void MediaDeviceNotificationsLinux::OnFilePathChanged(const FilePath& path,
-                                                      bool error) {
+std::string RemovableDeviceNotificationsLinux::GetDeviceIdForPath(
+    const FilePath& path, FilePath* mount_point) const {
+  if (!path.IsAbsolute())
+    return std::string();
+
+  FilePath current = path;
+  for (current = path;
+       mount_info_map_.find(current) == mount_info_map_.end() &&
+          current != current.DirName();
+       current = current.DirName()) {}
+
+  if (mount_info_map_.find(current) == mount_info_map_.end())
+    return std::string();
+
+  if (mount_point) {
+    *mount_point = FilePath();
+    current.AppendRelativePath(path, mount_point);
+  }
+
+  return mount_info_map_.find(current)->second.device_id;
+}
+
+void RemovableDeviceNotificationsLinux::OnFilePathChanged(const FilePath& path,
+                                                          bool error) {
   if (path != mtab_path_) {
     // This cannot happen unless FilePathWatcher is buggy. Just ignore this
     // notification and do nothing.
@@ -228,17 +331,17 @@ void MediaDeviceNotificationsLinux::OnFilePathChanged(const FilePath& path,
   UpdateMtab();
 }
 
-void MediaDeviceNotificationsLinux::InitOnFileThread() {
+void RemovableDeviceNotificationsLinux::InitOnFileThread() {
   DCHECK(!initialized_);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   initialized_ = true;
 
   // The callback passed to Watch() has to be unretained. Otherwise
-  // MediaDeviceNotificationsLinux will live longer than expected, and
+  // RemovableDeviceNotificationsLinux will live longer than expected, and
   // FilePathWatcher will get in trouble at shutdown time.
   bool ret = file_watcher_.Watch(
       mtab_path_,
-      base::Bind(&MediaDeviceNotificationsLinux::OnFilePathChanged,
+      base::Bind(&RemovableDeviceNotificationsLinux::OnFilePathChanged,
                  base::Unretained(this)));
   if (!ret) {
     LOG(ERROR) << "Adding watch for " << mtab_path_.value() << " failed";
@@ -248,116 +351,98 @@ void MediaDeviceNotificationsLinux::InitOnFileThread() {
   UpdateMtab();
 }
 
-void MediaDeviceNotificationsLinux::UpdateMtab() {
+void RemovableDeviceNotificationsLinux::UpdateMtab() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   MountPointDeviceMap new_mtab;
-  ReadMtab(&new_mtab);
+  ReadMtab(mtab_path_, known_file_systems_, &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;
+  std::list<FilePath> mount_points_to_erase;
+  std::list<FilePath> multiple_mounted_devices_needing_reattachment;
   for (MountMap::const_iterator old_iter = mount_info_map_.begin();
        old_iter != mount_info_map_.end(); ++old_iter) {
-    const std::string& mount_point = old_iter->first;
-    const std::string& mount_device = old_iter->second.mount_device;
+    const FilePath& mount_point = old_iter->first;
+    const FilePath& mount_device = old_iter->second.mount_device;
     MountPointDeviceMap::iterator new_iter = new_mtab.find(mount_point);
     // |mount_point| not in |new_mtab| or |mount_device| is no longer mounted at
     // |mount_point|.
     if (new_iter == new_mtab.end() || (new_iter->second != mount_device)) {
-      RemoveOldDevice(old_iter->second.device_id);
+      MountPriorityMap::iterator priority =
+          mount_priority_map_.find(mount_device);
+      DCHECK(priority != mount_priority_map_.end());
+      ReferencedMountPoint::const_iterator has_priority =
+          priority->second.find(mount_point);
+      if (old_iter->second.has_dcim) {
+        DCHECK(has_priority != priority->second.end());
+        if (has_priority->second)
+          RemoveMediaMount(old_iter->second.device_id);
+        if (mount_priority_map_.find(mount_device)->second.size() > 1)
+          multiple_mounted_devices_needing_reattachment.push_back(mount_device);
+      }
+      priority->second.erase(mount_point);
+      if (mount_priority_map_.find(mount_device)->second.empty())
+        mount_priority_map_.erase(mount_device);
       mount_points_to_erase.push_back(mount_point);
     }
   }
+
   // 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)
-    mount_info_map_.erase(mount_points_to_erase[i]);
+  for (std::list<FilePath>::const_iterator it = mount_points_to_erase.begin();
+       it != mount_points_to_erase.end();
+       ++it) {
+    mount_info_map_.erase(*it);
+  }
+
+  // For any multiply mounted device where the mount that we had notified
+  // got detached, send a notification of attachment for one of the other
+  // mount points.
+  for (std::list<FilePath>::const_iterator it =
+           multiple_mounted_devices_needing_reattachment.begin();
+       it != multiple_mounted_devices_needing_reattachment.end();
+       ++it) {
+    const FilePath& mount_point = mount_priority_map_[*it].begin()->first;
+    mount_priority_map_[*it].begin()->second = true;
+    DCHECK(mount_info_map_[mount_point].has_dcim);
+    base::SystemMonitor::Get()->ProcessRemovableStorageAttached(
+        mount_info_map_[mount_point].device_id,
+        mount_info_map_[mount_point].device_name,
+        mount_point.value());
+  }
 
   // Check new mtab entries against existing ones.
   for (MountPointDeviceMap::iterator new_iter = new_mtab.begin();
        new_iter != new_mtab.end(); ++new_iter) {
-    const std::string& mount_point = new_iter->first;
-    const std::string& mount_device = new_iter->second;
+    const FilePath& mount_point = new_iter->first;
+    const FilePath& mount_device = new_iter->second;
     MountMap::iterator old_iter = mount_info_map_.find(mount_point);
     if (old_iter == mount_info_map_.end() ||
         old_iter->second.mount_device != mount_device) {
       // New mount point found or an existing mount point found with a new
       // device.
-      CheckAndAddMediaDevice(mount_device, mount_point);
+      AddNewMount(mount_device, mount_point);
     }
   }
 }
 
-void MediaDeviceNotificationsLinux::ReadMtab(MountPointDeviceMap* mtab) {
-  FILE* fp = setmntent(mtab_path_.value().c_str(), "r");
-  if (!fp)
+void RemovableDeviceNotificationsLinux::AddNewMount(
+    const FilePath& mount_device, const FilePath& mount_point) {
+  if (mount_priority_map_.find(mount_device) != mount_priority_map_.end()) {
+    const FilePath& other_mount_point =
+        mount_priority_map_[mount_device].begin()->first;
+    mount_priority_map_[mount_device][mount_point] = false;
+    mount_info_map_[mount_point] = mount_info_map_[other_mount_point];
     return;
-
-  mntent entry;
-  char buf[512];
-
-  // 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 |entry_info| and use that below to resolve
-    // multiple devices mounted at the same mount point.
-    MountPointEntryInfo entry_info;
-    entry_info.mount_point = entry.mnt_dir;
-    entry_info.entry_pos = entry_pos++;
-    device_map[entry.mnt_fsname] = 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;
-  MountPointDeviceMap& new_mtab = *mtab;
-  for (DeviceMap::const_iterator device_it = device_map.begin();
-       device_it != device_map.end();
-       ++device_it) {
-    // 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.
-    const std::string& mount_device = device_it->first;
-    const std::string& mount_point = device_it->second.mount_point;
-    const int entry_pos = device_it->second.entry_pos;
-    MountPointDeviceMap::iterator new_it = new_mtab.find(mount_point);
-    MountPointsInfoMap::iterator mount_point_info_map_it =
-        mount_points_info_map.find(mount_point);
-
-    // New mount point entry found or there is already a device mounted at
-    // |mount_point| and the existing mount entry is older than the current one.
-    if (new_it == new_mtab.end() ||
-        (mount_point_info_map_it != mount_points_info_map.end() &&
-         mount_point_info_map_it->second < entry_pos)) {
-      new_mtab[mount_point] = mount_device;
-      mount_points_info_map[mount_point] = entry_pos;
-    }
   }
-}
 
-void MediaDeviceNotificationsLinux::CheckAndAddMediaDevice(
-    const std::string& mount_device,
-    const std::string& mount_point) {
-  if (!IsMediaDevice(mount_point))
-    return;
-
-  std::string id;
+  std::string unique_id;
   string16 name;
-  bool result = get_device_info_func_(mount_device, &id, &name);
+  bool removable;
+  bool result = get_device_info_func_(mount_device, &unique_id, &name,
+                                      &removable);
 
   // Keep track of GetDeviceInfo result, to see how often we fail to get device
   // details.
@@ -368,22 +453,42 @@ void MediaDeviceNotificationsLinux::CheckAndAddMediaDevice(
 
   // Keep track of device uuid, to see how often we receive empty values.
   UMA_HISTOGRAM_BOOLEAN("MediaDeviceNotification.device_uuid_available",
-                        !id.empty());
+                        !unique_id.empty());
   UMA_HISTOGRAM_BOOLEAN("MediaDeviceNotification.device_name_available",
                         !name.empty());
 
-  if (id.empty() || name.empty())
+  if (unique_id.empty() || name.empty())
     return;
 
-  MountDeviceAndId mount_device_and_id;
-  mount_device_and_id.mount_device = mount_device;
-  mount_device_and_id.device_id = id;
-  mount_info_map_[mount_point] = mount_device_and_id;
+  bool has_dcim = IsMediaDevice(mount_point.value());
+  MediaStorageUtil::Type type;
+  if (removable) {
+    if (has_dcim) {
+      type = MediaStorageUtil::USB_MASS_STORAGE_WITH_DCIM;
+    } else {
+      type = MediaStorageUtil::USB_MASS_STORAGE_NO_DCIM;
+    }
+  } else {
+    type = MediaStorageUtil::OTHER_MASS_STORAGE;
+  }
+  std::string device_id = MediaStorageUtil::MakeDeviceId(type, unique_id);
 
-  SystemMonitor::Get()->ProcessRemovableStorageAttached(id, name, mount_point);
+  MountPointInfo mount_point_info;
+  mount_point_info.mount_device = mount_device;
+  mount_point_info.device_id = device_id;
+  mount_point_info.device_name = name;
+  mount_point_info.has_dcim = has_dcim;
+
+  mount_info_map_[mount_point] = mount_point_info;
+  mount_priority_map_[mount_device][mount_point] = has_dcim;
+
+  if (mount_point_info.has_dcim) {
+    SystemMonitor::Get()->ProcessRemovableStorageAttached(device_id, name,
+                                                          mount_point.value());
+  }
 }
 
-void MediaDeviceNotificationsLinux::RemoveOldDevice(
+void RemovableDeviceNotificationsLinux::RemoveMediaMount(
     const std::string& device_id) {
   SystemMonitor::Get()->ProcessRemovableStorageDetached(device_id);
 }