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..d227a90664e1fa25bfd3acb8b9803bd059957dd6 100644
--- a/chromeos/disks/disk_mount_manager.cc
+++ b/chromeos/disks/disk_mount_manager.cc
@@ -112,6 +112,52 @@ class DiskMountManagerImpl : public DiskMountManager {
                                            mount_path));
   }
 
+  void RemountAllRemovableDrives(MountAccessMode mode) override {
+    // TODO(yamaguchi): Retry for tentative remount failures.
+    for (auto it = disks_.begin(); it != disks_.end(); ++it) {

[hashimoto, 2016/10/26 06:05:12]

How about using range-based for?
  for (const auto& disk : disks_)

[yamaguchi, 2016/10/27 06:34:55]

Done.

+      RemountRemovableDrive(it->second->mount_path(), mode);
+    }
+  }
+
+  void RemountRemovableDrive(const std::string& mount_path,

[hashimoto, 2016/10/26 06:05:12]

This method should be private.

[yamaguchi, 2016/10/27 06:34:55]

Done.

+                             MountAccessMode access_mode) {
+    MountPointMap::const_iterator mount_point = mount_points_.find(mount_path);
+    if (mount_point == mount_points_.end()) {
+      LOG(ERROR) << "Mount point with path \"" << mount_path << "\" not found.";

[hashimoto, 2016/10/26 06:05:12]

Don't we need to call OnMountCompleted with an error code on failures?

[yamaguchi, 2016/10/27 06:34:55]

Done.

+      return;
+    }
+
+    std::string device_path = mount_point->second.source_path;

[hashimoto, 2016/10/26 06:05:12]

const std::string&?

[yamaguchi, 2016/10/27 06:34:55]

Done.

+    DiskMap::const_iterator disk = disks_.find(device_path);

[hashimoto, 2016/10/26 06:05:12]

In RemountAllRemovableDrives(), we already know which Disk object corresponds to
the mount point.
Why do we need to find the disk again?

Also, the read-only check below shouldn't be done earlier with the Disk object?

[yamaguchi, 2016/10/27 06:34:55]

That's right. Fixed.

+    if (disk == disks_.end()) {
+      LOG(ERROR) << "Device with path \"" << device_path << "\" not found.";
+      return;
+    }
+    if (disk->second->is_read_only_hardware()) {
+      // A read-only device can be mounted in RO mode only. No need to remount.
+      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;

[hashimoto, 2016/10/26 06:05:12]

I'm not familiar with this access mode management code so please also ask
satorux@ to review this part.

Isn't there a plan to get this info via D-Bus?
I'm not sure how robust the current implementation is against unexpected events
like Chrome crash which can result in inconsistency between the actual mount
state and Chrome's internal state.

[yamaguchi, 2016/10/27 06:34:55]

Yes, storing to access_modes_ here is a tentative solution.

We haven't surfaced the one in cros_disks as DBus API yet.
https://chromium-review.googlesource.com/#/c/373320/

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

Seems the said patch was already submitted.
What else needs to be done?

I'm not familiar with this access_modes_ management code, but it looks quite
fragile to me.
For example, what happens when RemountAllRemovableDrives() gets called twice
with different access mode values, quickly enough?
For the first OnMountCompleted call, the Disk object should reflect the first
access mode, but IIUC it mistakenly reflects the second access mode.

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

Sorry for confusing you. That change is part of it, but we still need another CL
to update the DBus message schema and fill data there, and it's not done yet. So
this change is still based on the tentative solution.

[hashimoto, 2016/10/28 08:55:37]

The said change should be made as early as possible.
As I said above, this class has race bugs as long as we use this map.

+
+    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, "")));
+  }
+
   // DiskMountManager override.
   void FormatMountedDevice(const std::string& mount_path) override {
     MountPointMap::const_iterator mount_point = mount_points_.find(mount_path);
@@ -358,18 +404,14 @@ 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.
+        // Lookup the mount option passed when invoking Mount() by source path.

[hashimoto, 2016/10/26 06:05:12]

I couldn't understand what this comment means.
What does "the mount option passed" mean?
What does "when invoking Mount() by source path" mean?

[yamaguchi, 2016/10/27 06:34:55]

Revised the comments. Hope it makes more sense now.

         // |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/26 06:05:12]

It's not clear to me what this change is doing (and why this change is needed).
Could you put a number of words in the change description to make it clear?

[yamaguchi, 2016/10/27 06:34:55]

This mechanism to update the Disk object is revised because the older logic
assumed that disk object is generated by addition of device and then updated
only once for mounting (and also historically, because we didn't have separate
fields for RO-by-policy and RO-by-hardware).
Added a brief comment about this in the change description.

+          disk->set_write_disabled_by_policy(it->second ==
+                                             MOUNT_ACCESS_MODE_READ_ONLY);
         }
         disk->set_mount_path(mount_info.mount_path);
       }