Add a method to remount all removable devices in DiskMountManager.

chromeos/disks/disk_mount_manager.cc

Index: chromeos/disks/disk_mount_manager.cc
diff --git a/chromeos/disks/disk_mount_manager.cc b/chromeos/disks/disk_mount_manager.cc
index 971c8ffffd84660abd8481ef63dbe6253935ec73..9b4616e3889b4f763d9c1555e8e5e7204d3df02e 100644
--- a/chromeos/disks/disk_mount_manager.cc
+++ b/chromeos/disks/disk_mount_manager.cc
@@ -112,6 +112,21 @@ class DiskMountManagerImpl : public DiskMountManager {
                                            mount_path));
   }
 
+  void RemountAllRemovableDrives(MountAccessMode mode) override {
+    // TODO(yamaguchi): Retry for tentative remount failures.
+    for (const auto& it : disks_) {

[hashimoto, 2016/10/27 08:31:52]

|it| is an inappropriate name as it's no longer an iterator.
FYI now |it|'s type is const std::pair<std::string, std::unique_ptr<Disk>>>&.

[yamaguchi, 2016/10/27 09:52:40]

Done.

+      if (it.second->is_read_only_hardware()) {
+        // Read-only devices can be mounted in RO mode only. No need to remount.
+        continue;
+      }
+      if (it.second->mount_path().empty()) {
+        // Not mounted yet.
+        continue;
+      }
+      RemountRemovableDrive(*it.second.get(), mode);

[hashimoto, 2016/10/27 08:31:52]

*it.second or *(it.second)?

[yamaguchi, 2016/10/27 09:52:40]

Done.

+    }
+  }
+
   // DiskMountManager override.
   void FormatMountedDevice(const std::string& mount_path) override {
     MountPointMap::const_iterator mount_point = mount_points_.find(mount_path);
@@ -280,6 +295,39 @@ class DiskMountManagerImpl : public DiskMountManager {
     size_t num_pending_callbacks;
   };
 
+  void RemountRemovableDrive(const Disk& disk,
+                             MountAccessMode access_mode) {
+    const std::string& mount_path = disk.mount_path();
+    MountPointMap::const_iterator mount_point = mount_points_.find(mount_path);
+    if (mount_point == mount_points_.end()) {
+      // Not in mount_points_. This happens when the mount_points ans disks_ are
+      // inconsistent.
+      LOG(ERROR) << "Mount point with path \"" << mount_path << "\" not found.";
+      OnMountCompleted(
+          MountEntry(MOUNT_ERROR_PATH_NOT_MOUNTED, disk.device_path(),
+                     MOUNT_TYPE_DEVICE, mount_path));
+      return;
+    }
+    const std::string& source_path = mount_point->second.source_path;
+
+    // Update the access mode option passed to CrosDisks.
+    // This is needed because CrosDisks service methods doesn't return the info
+    // via DBus, and must be updated before issuing Mount command as it'll be
+    // read by the handler of MountCompleted signal.
+    access_modes_[source_path] = access_mode;
+
+    cros_disks_client_->Mount(
+        mount_point->second.source_path, std::string(), std::string(),
+        access_mode, REMOUNT_OPTION_REMOUNT_EXISTING_DEVICE,
+        // When succeeds, OnMountCompleted will be called by
+        // "MountCompleted" signal instead.
+        base::Bind(&base::DoNothing),
+        base::Bind(&DiskMountManagerImpl::OnMountCompleted,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   MountEntry(MOUNT_ERROR_INTERNAL, source_path,
+                              mount_point->second.mount_type, "")));
+  }
+
   // Unmounts all mount points whose source path is transitively parented by
   // |mount_path|.
   void UnmountChildMounts(const std::string& mount_path_in) {
@@ -358,18 +406,18 @@ class DiskMountManagerImpl : public DiskMountManager {
       if (iter != disks_.end()) {  // disk might have been removed by now?
         Disk* disk = iter->second.get();
         DCHECK(disk);
-        // The is_read_only field in *disk may be incorrect when this is called
-        // from CrosDisksClientImpl::OnMountCompleted.
-        // The disk should be treated as read-only when:
-        //  - the read-only option was passed when issuing mount command
-        //  - or the device hardware is read-only.
+        // Currently the MountCompleted signal doesn't tell whether the device
+        // is mounted in read-only mode or not. Instead use the mount option
+        // recorded by DiskMountManagerImpl::MountPath().
         // |source_path| should be same as |disk->device_path| because
         // |VolumeManager::OnDiskEvent()| passes the latter to cros-disks as a
         // source path when mounting a device.
         AccessModeMap::iterator it = access_modes_.find(entry.source_path());
-        if (it != access_modes_.end() &&
-            it->second == chromeos::MOUNT_ACCESS_MODE_READ_ONLY) {
-          disk->set_write_disabled_by_policy(true);
+        if (it != access_modes_.end() && !disk->is_read_only_hardware()) {

[hashimoto, 2016/10/27 08:31:52]

Don't we need to explicitly set false when the is_read_only_hardware value
changes?
Isn't it safer to always set the value?
  disk->set_write_disabled_by_policy(it != access_modes_.end() &&
!disk->is_read_only_hardware() && it->second == MOUNT_ACCESS_MODE_READ_ONLY)

[yamaguchi, 2016/10/27 09:52:40]

I don't think we need to fill false, because we don't expect
is_read_only_hardware() changes without removing that device. The field is
initialized using the profile policy at OnGetDeviceProperties first.
However, I'd like to choose that as I think it's much clearer.

+          // The device has been mounted with the access mode we specified.
+          // Record it to a Disk object to surface it to observers.
+          disk->set_write_disabled_by_policy(it->second ==
+                                             MOUNT_ACCESS_MODE_READ_ONLY);
         }
         disk->set_mount_path(mount_info.mount_path);
       }