Return unmount error from CrosDisksClient instead of success/fail.

Receiving the unmount error code will let Chrome properly handle various
types of errors, and produce better logging.

BUG=873903

Change-Id: I792a806440be6725ceba0663afc4b4ee3069d9bd
Reviewed-on: https://chromium-review.googlesource.com/1179109Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584348}

chromeos/dbus/cros_disks_client.cc

@@ -177,7 +177,7 @@ class CrosDisksClientImpl : public CrosDisksClient {
   // CrosDisksClient override.
   void Unmount(const std::string& device_path,
                UnmountOptions options,
-               VoidDBusMethodCallback callback) override {
+               UnmountCallback callback) override {
     dbus::MethodCall method_call(cros_disks::kCrosDisksInterface,
                                  cros_disks::kUnmount);
     dbus::MessageWriter writer(&method_call);
@@ -324,9 +324,9 @@ class CrosDisksClientImpl : public CrosDisksClient {
   }
 
   // Handles the result of Unmount and calls |callback| or |error_callback|.
-  void OnUnmount(VoidDBusMethodCallback callback, dbus::Response* response) {
+  void OnUnmount(UnmountCallback callback, dbus::Response* response) {
     if (!response) {
-      std::move(callback).Run(false);
+      std::move(callback).Run(MOUNT_ERROR_UNKNOWN);
       return;
     }
 
@@ -340,15 +340,12 @@ class CrosDisksClientImpl : public CrosDisksClient {
     // the response.
     dbus::MessageReader reader(response);
     uint32_t error_code = 0;
-    if (reader.PopUint32(&error_code) &&
-        CrosDisksMountErrorToChromeMountError(
-            static_cast<cros_disks::MountErrorType>(error_code)) !=
-            MOUNT_ERROR_NONE) {
-      std::move(callback).Run(false);
-      return;
+    MountError mount_error = MOUNT_ERROR_NONE;
+    if (reader.PopUint32(&error_code)) {
+      mount_error = CrosDisksMountErrorToChromeMountError(
+          static_cast<cros_disks::MountErrorType>(error_code));
     }
-
-    std::move(callback).Run(true);
+    std::move(callback).Run(mount_error);
   }
 
   // Handles the result of EnumerateDevices and EnumarateAutoMountableDevices.

chromeos/dbus/cros_disks_client.h

@@ -283,6 +283,10 @@ class CHROMEOS_EXPORT CrosDisksClient : public DBusClient {
   typedef base::Callback<void(const DiskInfo& disk_info)>
       GetDevicePropertiesCallback;
 
+  // A callback to handle the result of Unmount.
+  // The argument is the unmount error code.
+  typedef base::OnceCallback<void(MountError error_code)> UnmountCallback;
+
   class Observer {
    public:
     // Called when a mount event signal is received.
@@ -330,10 +334,10 @@ class CHROMEOS_EXPORT CrosDisksClient : public DBusClient {
                      VoidDBusMethodCallback callback) = 0;
 
   // Calls Unmount method.  On method call completion, |callback| is called
-  // with |true| on success, or with |false| otherwise.
+  // with the error code.
   virtual void Unmount(const std::string& device_path,
                        UnmountOptions options,
-                       VoidDBusMethodCallback callback) = 0;
+                       UnmountCallback callback) = 0;
 
   // Calls EnumerateDevices method.  |callback| is called after the
   // method call succeeds, otherwise, |error_callback| is called.

chromeos/dbus/fake_cros_disks_client.cc

@@ -56,7 +56,7 @@ MountError PerformFakeMount(const std::string& source_path,
 
 FakeCrosDisksClient::FakeCrosDisksClient()
     : unmount_call_count_(0),
-      unmount_success_(true),
+      unmount_error_(MOUNT_ERROR_NONE),
       format_call_count_(0),
       format_success_(true),
       rename_call_count_(0),
@@ -144,7 +144,7 @@ void FakeCrosDisksClient::DidMount(const std::string& source_path,
 
 void FakeCrosDisksClient::Unmount(const std::string& device_path,
                                   UnmountOptions options,
-                                  VoidDBusMethodCallback callback) {
+                                  UnmountCallback callback) {
   DCHECK(!callback.is_null());
 
   unmount_call_count_++;
@@ -160,10 +160,10 @@ void FakeCrosDisksClient::Unmount(const std::string& device_path,
         base::BindOnce(base::IgnoreResult(&base::DeleteFile),
                        base::FilePath::FromUTF8Unsafe(device_path),
                        true /* recursive */),
-        base::BindOnce(std::move(callback), unmount_success_));
+        base::BindOnce(std::move(callback), unmount_error_));
   } else {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback), unmount_success_));
+        FROM_HERE, base::BindOnce(std::move(callback), unmount_error_));
   }
   if (!unmount_listener_.is_null())
     unmount_listener_.Run();

chromeos/dbus/fake_cros_disks_client.h

@@ -46,7 +46,7 @@ class CHROMEOS_EXPORT FakeCrosDisksClient : public CrosDisksClient {
   // Deletes the directory created in Mount().
   void Unmount(const std::string& device_path,
                UnmountOptions options,
-               VoidDBusMethodCallback callback) override;
+               UnmountCallback callback) override;
   void EnumerateDevices(const EnumerateDevicesCallback& callback,
                         const base::Closure& error_callback) override;
   void EnumerateMountEntries(const EnumerateMountEntriesCallback& callback,
@@ -94,9 +94,7 @@ class CHROMEOS_EXPORT FakeCrosDisksClient : public CrosDisksClient {
   }
 
   // Makes the subsequent Unmount() calls fail. Unmount() succeeds by default.
-  void MakeUnmountFail() {
-    unmount_success_ = false;
-  }
+  void MakeUnmountFail(MountError error_code) { unmount_error_ = error_code; }
 
   // Sets a listener callbackif the following Unmount() call is success or not.
   // Unmount() calls the corresponding callback given as a parameter.
@@ -152,7 +150,7 @@ class CHROMEOS_EXPORT FakeCrosDisksClient : public CrosDisksClient {
   int unmount_call_count_;
   std::string last_unmount_device_path_;
   UnmountOptions last_unmount_options_;
-  bool unmount_success_;
+  MountError unmount_error_;
   base::Closure unmount_listener_;
   int format_call_count_;
   std::string last_format_device_path_;

chromeos/disks/disk_mount_manager.cc

@@ -388,10 +388,11 @@ class DiskMountManagerImpl : public DiskMountManager,
   // Callback for UnmountDeviceRecursively.
   void OnUnmountDeviceRecursively(UnmountDeviceRecursivelyCallbackData* cb_data,
                                   const std::string& mount_path,
-                                  bool success) {
+                                  MountError error_code) {
+    bool success = error_code == MOUNT_ERROR_NONE;
     if (success) {
       // Do standard processing for Unmount event.
-      OnUnmountPath(UnmountPathCallback(), mount_path, true /* success */);
+      OnUnmountPath(UnmountPathCallback(), mount_path, MOUNT_ERROR_NONE);
       VLOG(1) << mount_path <<  " unmounted.";
     }
     // This is safe as long as all callbacks are called on the same thread as
@@ -464,7 +465,7 @@ class DiskMountManagerImpl : public DiskMountManager,
   // Callback for UnmountPath.
   void OnUnmountPath(const UnmountPathCallback& callback,
                      const std::string& mount_path,
-                     bool success) {
+                     MountError error_code) {
     MountPointMap::iterator mount_points_it = mount_points_.find(mount_path);
     if (mount_points_it == mount_points_.end()) {
       // The path was unmounted, but not as a result of this unmount request,
@@ -475,26 +476,25 @@ class DiskMountManagerImpl : public DiskMountManager,
     }
 
     NotifyMountStatusUpdate(
-        UNMOUNTING,
-        success ? MOUNT_ERROR_NONE : MOUNT_ERROR_INTERNAL,
+        UNMOUNTING, error_code,
         MountPointInfo(mount_points_it->second.source_path,
                        mount_points_it->second.mount_path,
                        mount_points_it->second.mount_type,
                        mount_points_it->second.mount_condition));
 
     std::string path(mount_points_it->second.source_path);
-    if (success)
+    if (error_code == MOUNT_ERROR_NONE)
       mount_points_.erase(mount_points_it);
 
     DiskMap::iterator disk_iter = disks_.find(path);
     if (disk_iter != disks_.end()) {
       DCHECK(disk_iter->second);
-      if (success)
+      if (error_code == MOUNT_ERROR_NONE)
         disk_iter->second->clear_mount_path();
     }
 
     if (!callback.is_null())
-      callback.Run(success ? MOUNT_ERROR_NONE : MOUNT_ERROR_INTERNAL);
+      callback.Run(error_code);
   }
 
   void OnUnmountPathForFormat(const std::string& device_path,

chromeos/disks/disk_mount_manager_unittest.cc

@@ -620,7 +620,8 @@ TEST_F(DiskMountManagerTest, Format_FailToUnmount) {
   // In this test unmount will fail, and there should be no attempt to
   // format the device.
 
-  fake_cros_disks_client_->MakeUnmountFail();
+  fake_cros_disks_client_->MakeUnmountFail(
+      chromeos::MOUNT_ERROR_PATH_NOT_MOUNTED);
   // Start test.
   DiskMountManager::GetInstance()->FormatMountedDevice(kDevice1MountPath);
 
@@ -632,7 +633,7 @@ TEST_F(DiskMountManagerTest, Format_FailToUnmount) {
   ASSERT_EQ(2U, observer_->GetEventCount());
   const MountEvent& mount_event = observer_->GetMountEvent(0);
   EXPECT_EQ(DiskMountManager::UNMOUNTING, mount_event.event);
-  EXPECT_EQ(chromeos::MOUNT_ERROR_INTERNAL, mount_event.error_code);
+  EXPECT_EQ(chromeos::MOUNT_ERROR_PATH_NOT_MOUNTED, mount_event.error_code);
   EXPECT_EQ(kDevice1MountPath, mount_event.mount_point.mount_path);
 
   EXPECT_EQ(FormatEvent(DiskMountManager::FORMAT_COMPLETED,
@@ -698,7 +699,8 @@ TEST_F(DiskMountManagerTest, Format_ConcurrentFormatCalls) {
 
   fake_cros_disks_client_->set_unmount_listener(
       base::Bind(&FakeCrosDisksClient::MakeUnmountFail,
-                 base::Unretained(fake_cros_disks_client_)));
+                 base::Unretained(fake_cros_disks_client_),
+                 chromeos::MOUNT_ERROR_INVALID_UNMOUNT_OPTIONS));
   // Start the test.
   DiskMountManager::GetInstance()->FormatMountedDevice(kDevice1MountPath);
   DiskMountManager::GetInstance()->FormatMountedDevice(kDevice1MountPath);
@@ -1081,7 +1083,7 @@ TEST_F(DiskMountManagerTest, Rename_FailToUnmount) {
   // In this test unmount will fail, and there should be no attempt to
   // rename the device.
 
-  fake_cros_disks_client_->MakeUnmountFail();
+  fake_cros_disks_client_->MakeUnmountFail(chromeos::MOUNT_ERROR_UNKNOWN);
   // Start test.
   DiskMountManager::GetInstance()->RenameMountedDevice(kDevice1MountPath,
                                                        "MYUSB");
@@ -1094,7 +1096,7 @@ TEST_F(DiskMountManagerTest, Rename_FailToUnmount) {
   ASSERT_EQ(2U, observer_->GetEventCount());
   const MountEvent& mount_event = observer_->GetMountEvent(0);
   EXPECT_EQ(DiskMountManager::UNMOUNTING, mount_event.event);
-  EXPECT_EQ(chromeos::MOUNT_ERROR_INTERNAL, mount_event.error_code);
+  EXPECT_EQ(chromeos::MOUNT_ERROR_UNKNOWN, mount_event.error_code);
   EXPECT_EQ(kDevice1MountPath, mount_event.mount_point.mount_path);
 
   EXPECT_EQ(RenameEvent(DiskMountManager::RENAME_COMPLETED,
@@ -1161,7 +1163,8 @@ TEST_F(DiskMountManagerTest, Rename_ConcurrentRenameCalls) {
 
   fake_cros_disks_client_->set_unmount_listener(
       base::Bind(&FakeCrosDisksClient::MakeUnmountFail,
-                 base::Unretained(fake_cros_disks_client_)));
+                 base::Unretained(fake_cros_disks_client_),
+                 chromeos::MOUNT_ERROR_INTERNAL));
   // Start the test.
   DiskMountManager::GetInstance()->RenameMountedDevice(kDevice1MountPath,
                                                        "MYUSB1");