Add callback to DiskMountManager::UnmountPath

The callback makes it easier to handle failures (in comparison to observing
mount manager for MountEvents).

TEST=existing
BUG=tested formatting still works


Review URL: https://chromiumcodereview.appspot.com/12537016

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@188161 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/extensions/file_browser_event_router.cc

@@ -559,7 +559,8 @@ void FileBrowserEventRouter::OnExternalStorageDisabledChanged() {
       LOG(INFO) << "Unmounting " << it->second.mount_path
                 << " because of policy.";
       manager->UnmountPath(it->second.mount_path,
-                           chromeos::UNMOUNT_OPTIONS_NONE);
+                           chromeos::UNMOUNT_OPTIONS_NONE,
+                           DiskMountManager::UnmountPathCallback());
     }
   }
 }
@@ -797,7 +798,9 @@ void FileBrowserEventRouter::OnDiskRemoved(
           disk->system_path_prefix());
     }
     DiskMountManager::GetInstance()->UnmountPath(
-        disk->mount_path(), chromeos::UNMOUNT_OPTIONS_LAZY);
+        disk->mount_path(),
+        chromeos::UNMOUNT_OPTIONS_LAZY,
+        DiskMountManager::UnmountPathCallback());
   }
 }
 

chrome/browser/chromeos/extensions/file_browser_private_api.cc

@@ -1659,8 +1659,14 @@ void RemoveMountFunction::GetSelectedFileInfoResponse(
     SendResponse(false);
     return;
   }
-  DiskMountManager::GetInstance()->UnmountPath(files[0].local_path.value(),
+
-                                               chromeos::UNMOUNT_OPTIONS_NONE);
+  // TODO(tbarzic): Send response when callback is received, it would make more
+  // sense than remembering issued unmount requests in file manager and showing
+  // errors for them when MountCompleted event is received.
+  DiskMountManager::GetInstance()->UnmountPath(
+      files[0].local_path.value(),
+      chromeos::UNMOUNT_OPTIONS_NONE,
+      DiskMountManager::UnmountPathCallback());
   SendResponse(true);
 }
 

chrome/browser/chromeos/extensions/file_browser_private_apitest.cc

@@ -245,13 +245,13 @@ class ExtensionFileBrowserPrivateApiTest : public ExtensionApiTest {
 IN_PROC_BROWSER_TEST_F(ExtensionFileBrowserPrivateApiTest, FileBrowserMount) {
   // We will call fileBrowserPrivate.unmountVolume once. To test that method, we
   // check that UnmountPath is really called with the same value.
-  EXPECT_CALL(*disk_mount_manager_mock_, UnmountPath(_, _))
+  EXPECT_CALL(*disk_mount_manager_mock_, UnmountPath(_, _,  _))
       .Times(0);
   EXPECT_CALL(*disk_mount_manager_mock_,
               UnmountPath(
                   chromeos::CrosDisksClient::GetArchiveMountPoint().AppendASCII(
                       "archive_mount_path").AsUTF8Unsafe(),
-                  chromeos::UNMOUNT_OPTIONS_NONE)).Times(1);
+                  chromeos::UNMOUNT_OPTIONS_NONE, _)).Times(1);
 
   EXPECT_CALL(*disk_mount_manager_mock_, disks())
       .WillRepeatedly(ReturnRef(volumes_));

chrome/browser/chromeos/imageburner/burn_device_handler_unittest.cc

@@ -86,7 +86,8 @@ class FakeDiskMountManager : public disks::DiskMountManager {
                          const std::string& mount_label,
                          MountType type) OVERRIDE {}
   virtual void UnmountPath(const std::string& mount_path,
-                           UnmountOptions options) OVERRIDE {}
+                           UnmountOptions options,
+                           const UnmountPathCallback& callback) OVERRIDE {}
   virtual void FormatMountedDevice(const std::string& mount_path) OVERRIDE {}
   virtual void UnmountDeviceRecursively(
       const std::string& device_path,

chromeos/disks/disk_mount_manager.cc

@@ -83,14 +83,17 @@ class DiskMountManagerImpl : public DiskMountManager {
 
   // DiskMountManager override.
   virtual void UnmountPath(const std::string& mount_path,
-                           UnmountOptions options) OVERRIDE {
+                           UnmountOptions options,
+                           const UnmountPathCallback& callback) OVERRIDE {
     UnmountChildMounts(mount_path);
     cros_disks_client_->Unmount(mount_path, options,
                                 base::Bind(&DiskMountManagerImpl::OnUnmountPath,
                                            weak_ptr_factory_.GetWeakPtr(),
+                                           callback,
                                            true),
                                 base::Bind(&DiskMountManagerImpl::OnUnmountPath,
                                            weak_ptr_factory_.GetWeakPtr(),
+                                           callback,
                                            false));
   }
 
@@ -111,15 +114,11 @@ class DiskMountManagerImpl : public DiskMountManager {
       return;
     }
 
-    if (formatting_pending_.find(device_path) != formatting_pending_.end()) {
+    UnmountPath(disk->second->mount_path(),
-      LOG(ERROR) << "Formatting is already pending: " << mount_path;
+                UNMOUNT_OPTIONS_NONE,
-      OnFormatDevice(device_path, false);
+                base::Bind(&DiskMountManagerImpl::OnUnmountPathForFormat,
-      return;
+                           weak_ptr_factory_.GetWeakPtr(),
-    }
+                           device_path));
-
-    // Formatting process continues, after unmounting.
-    formatting_pending_.insert(device_path);
-    UnmountPath(disk->second->mount_path(), UNMOUNT_OPTIONS_NONE);
   }
 
   // DiskMountManager override.
@@ -258,7 +257,10 @@ class DiskMountManagerImpl : public DiskMountManager {
          ++it) {
       if (StartsWithASCII(it->second.source_path, mount_path,
                           true /*case sensitive*/)) {
-        UnmountPath(it->second.mount_path, UNMOUNT_OPTIONS_NONE);
+        // TODO(tbarzic): Handle the case where this fails.
+        UnmountPath(it->second.mount_path,
+                    UNMOUNT_OPTIONS_NONE,
+                    UnmountPathCallback());
       }
     }
   }
@@ -270,7 +272,7 @@ class DiskMountManagerImpl : public DiskMountManager {
       const std::string& mount_path) {
     if (success) {
       // Do standard processing for Unmount event.
-      OnUnmountPath(true, mount_path);
+      OnUnmountPath(UnmountPathCallback(), true, mount_path);
       LOG(INFO) << mount_path <<  " unmounted.";
     }
     // This is safe as long as all callbacks are called on the same thread as
@@ -328,10 +330,17 @@ class DiskMountManagerImpl : public DiskMountManager {
   }
 
   // Callback for UnmountPath.
-  void OnUnmountPath(bool success, const std::string& mount_path) {
+  void OnUnmountPath(const UnmountPathCallback& callback,
+                     bool success,
+                     const std::string& mount_path) {
     MountPointMap::iterator mount_points_it = mount_points_.find(mount_path);
-    if (mount_points_it == mount_points_.end())
+    if (mount_points_it == mount_points_.end()) {
+      // The path was unmounted, but not as a result of this unmount request,
+      // so return error.
+      if (!callback.is_null())
+        callback.Run(MOUNT_ERROR_INTERNAL);
       return;
+    }
 
     NotifyMountStatusUpdate(
         UNMOUNTING,
@@ -352,15 +361,17 @@ class DiskMountManagerImpl : public DiskMountManager {
         disk_iter->second->clear_mount_path();
     }
 
-    FormatTaskSet::iterator format_iter = formatting_pending_.find(path);
+    if (!callback.is_null())
-    // Check if there is a formatting scheduled.
+      callback.Run(success ? MOUNT_ERROR_NONE : MOUNT_ERROR_INTERNAL);
-    if (format_iter != formatting_pending_.end()) {
+  }
-      formatting_pending_.erase(format_iter);
+
-      if (success && disk_iter != disks_.end()) {
+  void OnUnmountPathForFormat(const std::string& device_path,
-        FormatUnmountedDevice(path);
+                              MountError error_code) {
-      } else {
+    if (error_code == MOUNT_ERROR_NONE &&
-        OnFormatDevice(path, false);
+        disks_.find(device_path) != disks_.end()) {
-      }
+      FormatUnmountedDevice(device_path);
+    } else {
+      OnFormatDevice(device_path, false);
     }
   }
 
@@ -594,14 +605,6 @@ class DiskMountManagerImpl : public DiskMountManager {
   typedef std::set<std::string> SystemPathPrefixSet;
   SystemPathPrefixSet system_path_prefixes_;
 
-  // A map from device path (e.g. /sys/devices/pci0000:00/.../sdb/sdb1)) to file
-  // path (e.g. /dev/sdb).
-  // Devices in this map are supposed to be formatted, but are currently waiting
-  // to be unmounted. When device is in this map, the formatting process HAVEN'T
-  // started yet.
-  typedef std::set<std::string> FormatTaskSet;
-  FormatTaskSet formatting_pending_;
-
   base::WeakPtrFactory<DiskMountManagerImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DiskMountManagerImpl);

chromeos/disks/disk_mount_manager.h

@@ -195,6 +195,9 @@ class CHROMEOS_EXPORT DiskMountManager {
   // finishes.
   typedef base::Callback<void(bool)> UnmountDeviceRecursivelyCallbackType;
 
+  // A callback type for UnmountPath method.
+  typedef base::Callback<void(MountError error_code)> UnmountPathCallback;
+
   // Implement this interface to be notified about disk/mount related events.
   class Observer {
    public:
@@ -237,6 +240,9 @@ class CHROMEOS_EXPORT DiskMountManager {
   virtual void RequestMountInfoRefresh() = 0;
 
   // Mounts a device.
+  // Note that the mount operation may fail. To find out the result, one should
+  // observe DiskMountManager for |Observer::OnMountEvent| event, which will be
+  // raised upon the mount operation completion.
   virtual void MountPath(const std::string& source_path,
                          const std::string& source_format,
                          const std::string& mount_label,
@@ -244,8 +250,13 @@ class CHROMEOS_EXPORT DiskMountManager {
 
   // Unmounts a mounted disk.
   // |UnmountOptions| enum defined in chromeos/dbus/cros_disks_client.h.
+  // When the method is complete, |callback| will be called and observers'
+  // |OnMountEvent| will be raised.
+  //
+  // |callback| may be empty, in which case it gets ignored.
   virtual void UnmountPath(const std::string& mount_path,
-                           UnmountOptions options) = 0;
+                           UnmountOptions options,
+                           const UnmountPathCallback& callback) = 0;
 
   // Formats Device given its mount path. Unmounts the device.
   // Example: mount_path: /media/VOLUME_LABEL

chromeos/disks/disk_mount_manager_unittest.cc

@@ -331,14 +331,15 @@ TEST_F(DiskMountManagerTest, Format_FormatFailsToStart) {
 // Tests the case where there are two format requests for the same device.
 TEST_F(DiskMountManagerTest, Format_ConcurrentFormatCalls) {
   // Set up cros disks client mocks.
-  // Only the first format request should be processed (for the other one there
+  // Only the first format request should be processed (the second unmount
-  // should be an error before device unmount is attempted).
+  // request fails because the device is already unmounted at that point).
   // CrosDisksClient will report that the format process for the first request
   // is successfully started.
   EXPECT_CALL(*mock_cros_disks_client_,
               Unmount("/device/mount_path", chromeos::UNMOUNT_OPTIONS_NONE,
                       _, _))
-      .WillOnce(MockUnmountPath(true));
+      .WillOnce(MockUnmountPath(true))
+      .WillOnce(MockUnmountPath(false));
 
   EXPECT_CALL(*mock_cros_disks_client_,
               FormatDevice("/device/source_path", "vfat", _, _))
@@ -348,19 +349,15 @@ TEST_F(DiskMountManagerTest, Format_ConcurrentFormatCalls) {
   // The observer should get two FORMAT_STARTED events, one for each format
   // request, but with different error codes (the formatting will be started
   // only for the first request).
-  // There should alos be one UNMOUNTING event, since the device should get
+  // There should be only one UNMOUNTING event. The result of the second one
-  // unmounted for the first request.
+  // should not be reported as the mount point will go away after the first
+  // request.
   //
   // Note that in this test the format completion signal will not be simulated,
   // so the observer should not get FORMAT_COMPLETED signal.
   {
     InSequence s;
 
-    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED,
-                                         chromeos::FORMAT_ERROR_UNKNOWN,
-                                         "/device/source_path"))
-        .Times(1);
-
     EXPECT_CALL(observer_,
         OnMountEvent(DiskMountManager::UNMOUNTING,
                      chromeos::MOUNT_ERROR_NONE,
@@ -368,6 +365,11 @@ TEST_F(DiskMountManagerTest, Format_ConcurrentFormatCalls) {
                            "/device/mount_path")))
         .Times(1);
 
+    EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED,
+                                         chromeos::FORMAT_ERROR_UNKNOWN,
+                                         "/device/source_path"))
+        .Times(1);
+
     EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED,
                                          chromeos::FORMAT_ERROR_NONE,
                                          "/device/source_path"))

chromeos/disks/mock_disk_mount_manager.cc

@@ -166,7 +166,7 @@ void MockDiskMountManager::SetupDefaultReplies() {
       .Times(AnyNumber());
   EXPECT_CALL(*this, MountPath(_, _, _, _))
       .Times(AnyNumber());
-  EXPECT_CALL(*this, UnmountPath(_, _))
+  EXPECT_CALL(*this, UnmountPath(_, _, _))
       .Times(AnyNumber());
   EXPECT_CALL(*this, FormatMountedDevice(_))
       .Times(AnyNumber());

chromeos/disks/mock_disk_mount_manager.h

@@ -33,7 +33,9 @@ class MockDiskMountManager : public DiskMountManager {
   MOCK_METHOD0(RequestMountInfoRefresh, void(void));
   MOCK_METHOD4(MountPath, void(const std::string&, const std::string&,
                                const std::string&, MountType));
-  MOCK_METHOD2(UnmountPath, void(const std::string&, UnmountOptions));
+  MOCK_METHOD3(UnmountPath, void(const std::string&,
+                                 UnmountOptions,
+                                 const DiskMountManager::UnmountPathCallback&));
   MOCK_METHOD1(FormatMountedDevice, void(const std::string&));
   MOCK_METHOD2(
       UnmountDeviceRecursively,