Store correct read-only flag of mounted disks.

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 f75e1ac44cda5428cb630ef2a3da1339b68a3dd6..296be15559b710d7b569642d98e07bd53e5aedb5 100644
--- a/chromeos/disks/disk_mount_manager.cc
+++ b/chromeos/disks/disk_mount_manager.cc
@@ -87,6 +87,11 @@ class DiskMountManagerImpl : public DiskMountManager {
         base::Bind(&DiskMountManagerImpl::OnMountCompleted,
                    weak_ptr_factory_.GetWeakPtr(),
                    MountEntry(MOUNT_ERROR_INTERNAL, source_path, type, "")));
+
+    // Record the access mode option passed to CrosDisks.
+    // This is needed because CrosDisks service methods doesn't return the info
+    // via DBus.
+    access_modes_.insert(std::make_pair(mount_label, access_mode));

[satorux1, 2016/08/12 02:47:51]

At this point, mount isn't complete yet. How about doing this inside
OnMountCompleted()? You can pass 'access_mode' to OnMountCompleted() function.

[yamaguchi, 2016/08/15 15:32:48]

OnMountCompleted is invoked by the signal from DBus. So it will be doable when
we review the dbus message.

   }
 
   // DiskMountManager override.
@@ -351,6 +356,15 @@ class DiskMountManagerImpl : public DiskMountManager {
       }
       Disk* disk = iter->second;
       DCHECK(disk);
+      // The is_read_only field in *disk may be incorrect when this is called
+      // from CrosDisksClientImpl::OnMountCompleted.
+      // Overwrite based on the access mode option that was passed when issuing
+      // mount command to the same mount path last time.
+      AccessModeMap::iterator it = access_modes_.find(disk->mount_path());

[satorux1, 2016/08/12 02:47:51]

access_modes_'s keys are mount labels, but here, it's accessed with
mount_path(). Are they the same thing? Would be nice to add some comment?

[yamaguchi, 2016/08/15 15:32:48]

Good catch. We need to use device path instead of mount_path, because mount_path
is not given when we insert it to the dict.

+      if (it != access_modes_.end()) {
+        disk->set_read_only(it->second ==
+                            chromeos::MOUNT_ACCESS_MODE_READ_ONLY);
+      }

[satorux1, 2016/08/12 02:47:52]

If 'access_mode' is passed to OnMountCompleted(), code around this can be as
simple as

  disk->set_read_only(access_mode == chromeos::MOUNT_ACCESS_MODE_READ_ONLY)

       disk->set_mount_path(mount_info.mount_path);
     }
   }
@@ -467,6 +481,13 @@ class DiskMountManagerImpl : public DiskMountManager {
                           disk_info.on_boot_device(),
                           disk_info.on_removable_device(),
                           disk_info.is_hidden());
+    // If the device was mounted by the instance, apply recorded parameter.
+    AccessModeMap::iterator mount_option =

[satorux1, 2016/08/12 02:47:52]

mount_option -> access_mode?

[yamaguchi, 2016/08/15 15:32:48]

Done.

+        access_modes_.find(disk->mount_path());
+    if (mount_option != access_modes_.end()) {
+      disk->set_read_only(mount_option->second ==
+                          chromeos::MOUNT_ACCESS_MODE_READ_ONLY);
+    }

[satorux1, 2016/08/12 02:47:52]

another approach: instead of introducing access_modes_, how about relying on the
previous disk info, that's deleted on the line 460? 

[yamaguchi, 2016/08/15 15:32:48]

The previous disk will be available only when this is called from the call path
of SuspendUnmountManager::SuspendDone, but not when it come from OnMountEvent
(=after mounting a new device).

     disks_.insert(std::make_pair(disk_info.device_path(), disk));
     NotifyDiskStatusUpdate(is_new ? DISK_ADDED : DISK_CHANGED, disk);
   }
@@ -634,6 +655,11 @@ class DiskMountManagerImpl : public DiskMountManager {
 
   std::unique_ptr<SuspendUnmountManager> suspend_unmount_manager_;
 
+  // Whether the instance attempted to mount a device in read-only mode for
+  // each mount label (not mount source).
+  typedef std::map<std::string, chromeos::MountAccessMode> AccessModeMap;
+  AccessModeMap access_modes_;
+
   base::WeakPtrFactory<DiskMountManagerImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DiskMountManagerImpl);