From e3c1fc95c46924e65957db4cb011e1994569f3ad Mon Sep 17 00:00:00 2001 From: "tbarzic@chromium.org" Date: Thu, 15 Nov 2012 00:56:46 +0000 Subject: Small refactoring in DiskMountManager (event reporting; format method). * Rename DiskMountManager::Observer methods to On____Event format. * Break up DiskMountManagerEventType enum. * Add separate observer method for formatting events. * Pass formatting event success using bool instead of adding '!' in front of the path (but only in events from DiskMountManager; will remove it from CrosDisksClient events in another patch). * Add unit tests for formatting handling in DiskMountManager. (TODO: add more tests) TBR:vandebo@chromium.org (for mechanical changes in chrome/browser/system_monitor) BUG=157587, 126815 TEST=manual: formatting works (and Formatting pending notification goes away) chromeos_unittests::DiskMountManagerTest.* Review URL: https://chromiumcodereview.appspot.com/11365142 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@167799 0039d316-1c4b-4281-b951-d872f2087c98 --- chromeos/disks/disk_mount_manager.cc | 258 ++++++----- chromeos/disks/disk_mount_manager.h | 63 +-- chromeos/disks/disk_mount_manager_unittest.cc | 625 ++++++++++++++++++++++++++ chromeos/disks/mock_disk_mount_manager.cc | 22 +- chromeos/disks/mock_disk_mount_manager.h | 5 +- 5 files changed, 816 insertions(+), 157 deletions(-) create mode 100644 chromeos/disks/disk_mount_manager_unittest.cc (limited to 'chromeos/disks') diff --git a/chromeos/disks/disk_mount_manager.cc b/chromeos/disks/disk_mount_manager.cc index 6f450d2..66c2a0b 100644 --- a/chromeos/disks/disk_mount_manager.cc +++ b/chromeos/disks/disk_mount_manager.cc @@ -30,7 +30,6 @@ class DiskMountManagerImpl : public DiskMountManager { DCHECK(dbus_thread_manager); cros_disks_client_ = dbus_thread_manager->GetCrosDisksClient(); DCHECK(cros_disks_client_); - cros_disks_client_->SetUpConnections( base::Bind(&DiskMountManagerImpl::OnMountEvent, weak_ptr_factory_.GetWeakPtr()), @@ -94,52 +93,31 @@ class DiskMountManagerImpl : public DiskMountManager { } // DiskMountManager override. - virtual void FormatUnmountedDevice(const std::string& file_path) OVERRIDE { - for (DiskMountManager::DiskMap::iterator it = disks_.begin(); - it != disks_.end(); ++it) { - if (it->second->file_path() == file_path && - !it->second->mount_path().empty()) { - LOG(ERROR) << "Device is still mounted: " << file_path; - OnFormatDevice(file_path, false); - return; - } - } - const char kFormatVFAT[] = "vfat"; - cros_disks_client_->FormatDevice( - file_path, - kFormatVFAT, - base::Bind(&DiskMountManagerImpl::OnFormatDevice, - weak_ptr_factory_.GetWeakPtr()), - base::Bind(&DiskMountManagerImpl::OnFormatDevice, - weak_ptr_factory_.GetWeakPtr(), - file_path, - false)); - } - - // DiskMountManager override. virtual void FormatMountedDevice(const std::string& mount_path) OVERRIDE { - Disk* disk = NULL; - for (DiskMountManager::DiskMap::iterator it = disks_.begin(); - it != disks_.end(); ++it) { - if (it->second->mount_path() == mount_path) { - disk = it->second; - break; - } - } - if (!disk) { - LOG(ERROR) << "Device with this mount path not found: " << mount_path; + 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."; OnFormatDevice(mount_path, false); return; } - if (formatting_pending_.find(disk->device_path()) != - formatting_pending_.end()) { + + std::string device_path = mount_point->second.source_path; + DiskMap::const_iterator disk = disks_.find(device_path); + if (disk == disks_.end()) { + LOG(ERROR) << "Device with path \"" << device_path << "\" not found."; + OnFormatDevice(device_path, false); + return; + } + + if (formatting_pending_.find(device_path) != formatting_pending_.end()) { LOG(ERROR) << "Formatting is already pending: " << mount_path; - OnFormatDevice(mount_path, false); + OnFormatDevice(device_path, false); return; } + // Formatting process continues, after unmounting. - formatting_pending_[disk->device_path()] = disk->file_path(); - UnmountPath(disk->mount_path(), UNMOUNT_OPTIONS_NONE); + formatting_pending_.insert(device_path); + UnmountPath(disk->second->mount_path(), UNMOUNT_OPTIONS_NONE); } // DiskMountManager override. @@ -216,6 +194,35 @@ class DiskMountManagerImpl : public DiskMountManager { return mount_points_; } + // DiskMountManager override. + virtual bool AddDiskForTest(Disk* disk) OVERRIDE { + if (disks_.find(disk->device_path()) != disks_.end()) { + LOG(ERROR) << "Attempt to add a duplicate disk"; + return false; + } + + disks_.insert(std::make_pair(disk->device_path(), disk)); + return true; + } + + // DiskMountManager override. + // Corresponding disk should be added to the manager before this is called. + virtual bool AddMountPointForTest( + const MountPointInfo& mount_point) OVERRIDE { + if (mount_points_.find(mount_point.mount_path) != mount_points_.end()) { + LOG(ERROR) << "Attempt to add a duplicate mount point"; + return false; + } + if (mount_point.mount_type == chromeos::MOUNT_TYPE_DEVICE && + disks_.find(mount_point.source_path) == disks_.end()) { + LOG(ERROR) << "Device mount points must have a disk entry."; + return false; + } + + mount_points_.insert(std::make_pair(mount_point.mount_path, mount_point)); + return true; + } + private: struct UnmountDeviceRecursiveCallbackData { void* user_data; @@ -285,7 +292,7 @@ class DiskMountManagerImpl : public DiskMountManager { const MountPointInfo mount_info(source_path, mount_path, mount_type, mount_condition); - NotifyMountCompleted(MOUNTING, error_code, mount_info); + NotifyMountStatusUpdate(MOUNTING, error_code, mount_info); // If the device is corrupted but it's still possible to format it, it will // be fake mounted. @@ -306,7 +313,6 @@ class DiskMountManagerImpl : public DiskMountManager { Disk* disk = iter->second; DCHECK(disk); disk->set_mount_path(mount_info.mount_path); - NotifyDiskStatusUpdate(MOUNT_DISK_MOUNTED, disk); } } @@ -315,7 +321,8 @@ class DiskMountManagerImpl : public DiskMountManager { MountPointMap::iterator mount_points_it = mount_points_.find(mount_path); if (mount_points_it == mount_points_.end()) return; - NotifyMountCompleted( + + NotifyMountStatusUpdate( UNMOUNTING, success ? MOUNT_ERROR_NONE : MOUNT_ERROR_INTERNAL, MountPointInfo(mount_points_it->second.source_path, @@ -327,40 +334,48 @@ class DiskMountManagerImpl : public DiskMountManager { if (success) mount_points_.erase(mount_points_it); - DiskMap::iterator iter = disks_.find(path); - if (iter == disks_.end()) { - // disk might have been removed by now. - return; + DiskMap::iterator disk_iter = disks_.find(path); + if (disk_iter != disks_.end()) { + DCHECK(disk_iter->second); + if (success) + disk_iter->second->clear_mount_path(); } - Disk* disk = iter->second; - DCHECK(disk); - if (success) - disk->clear_mount_path(); - + FormatTaskSet::iterator format_iter = formatting_pending_.find(path); // Check if there is a formatting scheduled. - PathMap::iterator it = formatting_pending_.find(disk->device_path()); - if (it != formatting_pending_.end()) { - // Copy the string before it gets erased. - const std::string file_path = it->second; - formatting_pending_.erase(it); - if (success) { - FormatUnmountedDevice(file_path); + if (format_iter != formatting_pending_.end()) { + formatting_pending_.erase(format_iter); + if (success && disk_iter != disks_.end()) { + FormatUnmountedDevice(path); } else { - OnFormatDevice(file_path, false); + OnFormatDevice(path, false); } } } + // Starts device formatting. + void FormatUnmountedDevice(const std::string& device_path) { + DiskMap::const_iterator disk = disks_.find(device_path); + DCHECK(disk != disks_.end() && disk->second->mount_path().empty()); + + const char kFormatVFAT[] = "vfat"; + cros_disks_client_->FormatDevice( + device_path, + kFormatVFAT, + base::Bind(&DiskMountManagerImpl::OnFormatDevice, + weak_ptr_factory_.GetWeakPtr()), + base::Bind(&DiskMountManagerImpl::OnFormatDevice, + weak_ptr_factory_.GetWeakPtr(), + device_path, + false)); + } + // Callback for FormatDevice. + // TODO(tbarzic): Pass FormatError instead of bool. void OnFormatDevice(const std::string& device_path, bool success) { - if (success) { - NotifyDeviceStatusUpdate(MOUNT_FORMATTING_STARTED, device_path); - } else { - NotifyDeviceStatusUpdate(MOUNT_FORMATTING_STARTED, - std::string("!") + device_path); - LOG(WARNING) << "Format request failed for device " << device_path; - } + FormatError error_code = + success ? FORMAT_ERROR_NONE : FORMAT_ERROR_UNKNOWN; + NotifyFormatStatusUpdate(FORMAT_STARTED, error_code, device_path); } // Callbcak for GetDeviceProperties. @@ -400,8 +415,7 @@ class DiskMountManagerImpl : public DiskMountManager { disk_info.on_boot_device(), disk_info.is_hidden()); disks_.insert(std::make_pair(disk_info.device_path(), disk)); - NotifyDiskStatusUpdate(is_new ? MOUNT_DISK_ADDED : MOUNT_DISK_CHANGED, - disk); + NotifyDiskStatusUpdate(is_new ? DISK_ADDED : DISK_CHANGED, disk); } // Callbcak for RequestMountInfo. @@ -423,7 +437,7 @@ class DiskMountManagerImpl : public DiskMountManager { for (DiskMap::iterator iter = disks_.begin(); iter != disks_.end(); ) { if (current_device_set.find(iter->first) == current_device_set.end()) { Disk* disk = iter->second; - NotifyDiskStatusUpdate(MOUNT_DISK_REMOVED, disk); + NotifyDiskStatusUpdate(DISK_REMOVED, disk); delete iter->second; disks_.erase(iter++); } else { @@ -436,98 +450,110 @@ class DiskMountManagerImpl : public DiskMountManager { void OnMountEvent(MountEventType event, const std::string& device_path_arg) { // Take a copy of the argument so we can modify it below. std::string device_path = device_path_arg; - DiskMountManagerEventType type = MOUNT_DEVICE_ADDED; switch (event) { - case DISK_ADDED: { + case CROS_DISKS_DISK_ADDED: { cros_disks_client_->GetDeviceProperties( device_path, base::Bind(&DiskMountManagerImpl::OnGetDeviceProperties, weak_ptr_factory_.GetWeakPtr()), base::Bind(&base::DoNothing)); - return; + break; } - case DISK_REMOVED: { + case CROS_DISKS_DISK_REMOVED: { // Search and remove disks that are no longer present. DiskMountManager::DiskMap::iterator iter = disks_.find(device_path); if (iter != disks_.end()) { Disk* disk = iter->second; - NotifyDiskStatusUpdate(MOUNT_DISK_REMOVED, disk); + NotifyDiskStatusUpdate(DISK_REMOVED, disk); delete iter->second; disks_.erase(iter); } - return; + break; } - case DEVICE_ADDED: { - type = MOUNT_DEVICE_ADDED; + case CROS_DISKS_DEVICE_ADDED: { system_path_prefixes_.insert(device_path); + NotifyDeviceStatusUpdate(DEVICE_ADDED, device_path); break; } - case DEVICE_REMOVED: { - type = MOUNT_DEVICE_REMOVED; + case CROS_DISKS_DEVICE_REMOVED: { system_path_prefixes_.erase(device_path); + NotifyDeviceStatusUpdate(DEVICE_REMOVED, device_path); break; } - case DEVICE_SCANNED: { - type = MOUNT_DEVICE_SCANNED; + case CROS_DISKS_DEVICE_SCANNED: { + NotifyDeviceStatusUpdate(DEVICE_SCANNED, device_path); break; } - case FORMATTING_FINISHED: { - // FORMATTING_FINISHED actually returns file path instead of device - // path. - device_path = FilePathToDevicePath(device_path); - if (device_path.empty()) { - LOG(ERROR) << "Error while handling disks metadata. Cannot find " - << "device that is being formatted."; - return; + case CROS_DISKS_FORMATTING_FINISHED: { + std::string path; + FormatError error_code; + ParseFormatFinishedPath(device_path, &path, &error_code); + + if (!path.empty()) { + NotifyFormatStatusUpdate(FORMAT_COMPLETED, error_code, path); + break; } - type = MOUNT_FORMATTING_FINISHED; + + LOG(ERROR) << "Error while handling disks metadata. Cannot find " + << "device that is being formatted."; break; } default: { LOG(ERROR) << "Unknown event: " << event; - return; } } - NotifyDeviceStatusUpdate(type, device_path); } // Notifies all observers about disk status update. - void NotifyDiskStatusUpdate(DiskMountManagerEventType event, + void NotifyDiskStatusUpdate(DiskEvent event, const Disk* disk) { - FOR_EACH_OBSERVER(Observer, observers_, DiskChanged(event, disk)); + FOR_EACH_OBSERVER(Observer, observers_, OnDiskEvent(event, disk)); } // Notifies all observers about device status update. - void NotifyDeviceStatusUpdate(DiskMountManagerEventType event, + void NotifyDeviceStatusUpdate(DeviceEvent event, const std::string& device_path) { - FOR_EACH_OBSERVER(Observer, observers_, DeviceChanged(event, device_path)); + FOR_EACH_OBSERVER(Observer, observers_, OnDeviceEvent(event, device_path)); } // Notifies all observers about mount completion. - void NotifyMountCompleted(MountEvent event_type, - MountError error_code, - const MountPointInfo& mount_info) { + void NotifyMountStatusUpdate(MountEvent event, + MountError error_code, + const MountPointInfo& mount_info) { + FOR_EACH_OBSERVER(Observer, observers_, + OnMountEvent(event, error_code, mount_info)); + } + + void NotifyFormatStatusUpdate(FormatEvent event, + FormatError error_code, + const std::string& device_path) { FOR_EACH_OBSERVER(Observer, observers_, - MountCompleted(event_type, error_code, mount_info)); + OnFormatEvent(event, error_code, device_path)); } // Converts file path to device path. - std::string FilePathToDevicePath(const std::string& file_path) { - // TODO(hashimoto): Refactor error handling code like here. + void ParseFormatFinishedPath(const std::string& received_path, + std::string* device_path, + FormatError* error_code) { + // TODO(tbarzic): Refactor error handling code like here. // Appending "!" is not the best way to indicate error. This kind of trick - // also makes it difficult to simplify the code paths. crosbug.com/22972 - const int failed = StartsWithASCII(file_path, "!", true); + // also makes it difficult to simplify the code paths. + bool success = !StartsWithASCII(received_path, "!", true); + *error_code = success ? FORMAT_ERROR_NONE : FORMAT_ERROR_UNKNOWN; + + std::string path = received_path.substr(success ? 0 : 1); + + // Depending on cros disks implementation the event may return either file + // path or device path. We want to use device path. for (DiskMountManager::DiskMap::iterator it = disks_.begin(); it != disks_.end(); ++it) { // Skip the leading '!' on the failure case. - if (it->second->file_path() == file_path.substr(failed)) { - if (failed) - return std::string("!") + it->second->device_path(); - else - return it->second->device_path(); + if (it->second->file_path() == path || + it->second->device_path() == path) { + *device_path = it->second->device_path(); + return; } } - return ""; } // Finds system path prefix from |system_path|. @@ -562,8 +588,8 @@ class DiskMountManagerImpl : public DiskMountManager { // 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::map PathMap; - PathMap formatting_pending_; + typedef std::set FormatTaskSet; + FormatTaskSet formatting_pending_; base::WeakPtrFactory weak_ptr_factory_; @@ -614,6 +640,14 @@ DiskMountManager::Disk::Disk(const std::string& device_path, DiskMountManager::Disk::~Disk() {} +bool DiskMountManager::AddDiskForTest(Disk* disk) { + return false; +} + +bool DiskMountManager::AddMountPointForTest(const MountPointInfo& mount_point) { + return false; +} + // static std::string DiskMountManager::MountTypeToString(MountType type) { switch (type) { diff --git a/chromeos/disks/disk_mount_manager.h b/chromeos/disks/disk_mount_manager.h index 0fe1a1a..895c607 100644 --- a/chromeos/disks/disk_mount_manager.h +++ b/chromeos/disks/disk_mount_manager.h @@ -13,20 +13,6 @@ namespace chromeos { namespace disks { -// Types of events DiskMountManager sends to its observers. -enum DiskMountManagerEventType { - MOUNT_DISK_ADDED, - MOUNT_DISK_REMOVED, - MOUNT_DISK_CHANGED, - MOUNT_DISK_MOUNTED, - MOUNT_DISK_UNMOUNTED, - MOUNT_DEVICE_ADDED, - MOUNT_DEVICE_REMOVED, - MOUNT_DEVICE_SCANNED, - MOUNT_FORMATTING_STARTED, - MOUNT_FORMATTING_FINISHED, -}; - // Condition of mounted filesystem. enum MountCondition { MOUNT_CONDITION_NONE, @@ -38,12 +24,29 @@ enum MountCondition { // Other classes can add themselves as observers. class CHROMEOS_EXPORT DiskMountManager { public: - // Event type given to observers' MountCompleted method. + // Event types passed to the observers. + enum DiskEvent { + DISK_ADDED, + DISK_REMOVED, + DISK_CHANGED, + }; + + enum DeviceEvent { + DEVICE_ADDED, + DEVICE_REMOVED, + DEVICE_SCANNED, + }; + enum MountEvent { MOUNTING, UNMOUNTING, }; + enum FormatEvent { + FORMAT_STARTED, + FORMAT_COMPLETED + }; + // Used to house an instance of each found mount device. class Disk { public: @@ -196,16 +199,19 @@ class CHROMEOS_EXPORT DiskMountManager { public: virtual ~Observer() {} - // A function called when disk mount status is changed. - virtual void DiskChanged(DiskMountManagerEventType event, - const Disk* disk) = 0; - // A function called when device status is changed. - virtual void DeviceChanged(DiskMountManagerEventType event, + // Called when disk mount status is changed. + virtual void OnDiskEvent(DiskEvent event, const Disk* disk) = 0; + // Called when device status is changed. + virtual void OnDeviceEvent(DeviceEvent event, + const std::string& device_path) = 0; + // Called after a mount point has been mounted or unmounted. + virtual void OnMountEvent(MountEvent event, + MountError error_code, + const MountPointInfo& mount_info) = 0; + // Called on format process events. + virtual void OnFormatEvent(FormatEvent event, + FormatError error_code, const std::string& device_path) = 0; - // A function called after mount is completed. - virtual void MountCompleted(MountEvent event_type, - MountError error_code, - const MountPointInfo& mount_info) = 0; }; virtual ~DiskMountManager() {} @@ -240,10 +246,6 @@ class CHROMEOS_EXPORT DiskMountManager { virtual void UnmountPath(const std::string& mount_path, UnmountOptions options) = 0; - // Formats device given its file path. - // Example: file_path: /dev/sdb1 - virtual void FormatUnmountedDevice(const std::string& file_path) = 0; - // Formats Device given its mount path. Unmounts the device. // Example: mount_path: /media/VOLUME_LABEL virtual void FormatMountedDevice(const std::string& mount_path) = 0; @@ -254,6 +256,11 @@ class CHROMEOS_EXPORT DiskMountManager { UnmountDeviceRecursiveCallbackType callback, void* user_data) = 0; + // Used in tests to initialize the manager's disk and mount point sets. + // Default implementation does noting. It just fails. + virtual bool AddDiskForTest(Disk* disk); + virtual bool AddMountPointForTest(const MountPointInfo& mount_point); + // Returns corresponding string to |type| like "device" or "file". static std::string MountTypeToString(MountType type); diff --git a/chromeos/disks/disk_mount_manager_unittest.cc b/chromeos/disks/disk_mount_manager_unittest.cc new file mode 100644 index 0000000..a66aab9 --- /dev/null +++ b/chromeos/disks/disk_mount_manager_unittest.cc @@ -0,0 +1,625 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/bind.h" +#include "base/message_loop.h" +#include "chromeos/dbus/mock_cros_disks_client.h" +#include "chromeos/dbus/mock_dbus_thread_manager.h" +#include "chromeos/disks/disk_mount_manager.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +using chromeos::disks::DiskMountManager; +using chromeos::CrosDisksClient; +using chromeos::DBusThreadManager; +using chromeos::MockCrosDisksClient; +using chromeos::MockDBusThreadManager; +using testing::_; +using testing::Field; +using testing::InSequence; + +namespace { + +// Holds information needed to create a DiskMountManager::Disk instance. +struct TestDiskInfo { + const char* source_path; + const char* mount_path; + const char* system_path; + const char* file_path; + const char* device_label; + const char* drive_label; + const char* vendor_id; + const char* vendor_name; + const char* product_id; + const char* product_name; + const char* fs_uuid; + const char* system_path_prefix; + chromeos::DeviceType device_type; + uint64 size_in_bytes; + bool is_parent; + bool is_read_only; + bool has_media; + bool on_boot_device; + bool is_hidden; +}; + +// Holds information to create a DiskMOuntManager::MountPointInfo instance. +struct TestMountPointInfo { + const char* source_path; + const char* mount_path; + chromeos::MountType mount_type; + chromeos::disks::MountCondition mount_condition; +}; + +// List of disks held in DiskMountManager at the begining of the test. +const TestDiskInfo kTestDisks[] = { + { + "/device/source_path", + "/device/mount_path", + "/device/prefix/system_path", + "/device/file_path", + "/device/device_label", + "/device/drive_label", + "/device/vendor_id", + "/device/vendor_name", + "/device/product_id", + "/device/product_name", + "/device/fs_uuid", + "/device/prefix", + chromeos::DEVICE_TYPE_USB, + 1073741824, // size in bytes + false, // is parent + false, // is read only + true, // has media + false, // is on boot device + false // is hidden + }, +}; + +// List ofmount points held in DiskMountManager at the begining of the test. +const TestMountPointInfo kTestMountPoints[] = { + { + "/archive/source_path", + "/archive/mount_path", + chromeos::MOUNT_TYPE_ARCHIVE, + chromeos::disks::MOUNT_CONDITION_NONE + }, + { + "/device/source_path", + "/device/mount_path", + chromeos::MOUNT_TYPE_DEVICE, + chromeos::disks::MOUNT_CONDITION_NONE + }, +}; + +// Mocks response from CrosDisksClient::Unmount. +ACTION_P(MockUnmountPath, success) { + CrosDisksClient::UnmountCallback callback = success ? arg2 : arg3; + base::MessageLoopProxy::current()->PostTask(FROM_HERE, + base::Bind(callback, arg0)); +} + +// Mocks response from CrosDisksClient::FormatDevice. +ACTION_P(MockFormatDevice, success) { + if (success) { + base::MessageLoopProxy::current()->PostTask(FROM_HERE, + base::Bind(arg2, arg0, true)); + } else { + base::MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(arg3)); + } +} + +// Mocks DiskMountManager observer. +class MockDiskMountManagerObserver : public DiskMountManager::Observer { + public: + virtual ~MockDiskMountManagerObserver() {} + + MOCK_METHOD2(OnDiskEvent, void(DiskMountManager::DiskEvent event, + const DiskMountManager::Disk* disk)); + MOCK_METHOD2(OnDeviceEvent, void(DiskMountManager::DeviceEvent event, + const std::string& device_path)); + MOCK_METHOD3(OnMountEvent, + void(DiskMountManager::MountEvent event, + chromeos::MountError error_code, + const DiskMountManager::MountPointInfo& mount_point)); + MOCK_METHOD3(OnFormatEvent, + void(DiskMountManager::FormatEvent event, + chromeos::FormatError error_code, + const std::string& device_path)); +}; + +class DiskMountManagerTest : public testing::Test { + public: + DiskMountManagerTest() {} + virtual ~DiskMountManagerTest() {} + + // Sets up test dbus tread manager and disks mount manager. + // Initializes disk mount manager disks and mount points. + // Adds a test observer to the disk mount manager. + virtual void SetUp() { + MockDBusThreadManager* mock_thread_manager = new MockDBusThreadManager(); + DBusThreadManager::InitializeForTesting(mock_thread_manager); + + mock_cros_disks_client_ = mock_thread_manager->mock_cros_disks_client(); + + DiskMountManager::Initialize(); + + InitDisksAndMountPoints(); + + DiskMountManager::GetInstance()->AddObserver(&observer_); + } + + // Shuts down dbus thread manager and disk moutn manager used in the test. + virtual void TearDown() { + DiskMountManager::GetInstance()->RemoveObserver(&observer_); + DiskMountManager::Shutdown(); + DBusThreadManager::Shutdown(); + } + + protected: + // Checks if disk mount manager contains a mount point with specified moutn + // path. + bool HasMountPoint(const std::string& mount_path) { + const DiskMountManager::MountPointMap& mount_points = + DiskMountManager::GetInstance()->mount_points(); + return mount_points.find(mount_path) != mount_points.end(); + } + + private: + // Adds a new disk to the disk mount manager. + void AddTestDisk(const TestDiskInfo& disk) { + EXPECT_TRUE(DiskMountManager::GetInstance()->AddDiskForTest( + new DiskMountManager::Disk(disk.source_path, + disk.mount_path, + disk.system_path, + disk.file_path, + disk.device_label, + disk.drive_label, + disk.vendor_id, + disk.vendor_name, + disk.product_id, + disk.product_name, + disk.fs_uuid, + disk.system_path_prefix, + disk.device_type, + disk.size_in_bytes, + disk.is_parent, + disk.is_read_only, + disk.has_media, + disk.on_boot_device, + disk.is_hidden))); + } + + // Adds a new mount point to the disk mount manager. + // If the moutn point is a device mount point, disk with its source path + // should already be added to the disk mount manager. + void AddTestMountPoint(const TestMountPointInfo& mount_point) { + EXPECT_TRUE(DiskMountManager::GetInstance()->AddMountPointForTest( + DiskMountManager::MountPointInfo(mount_point.source_path, + mount_point.mount_path, + mount_point.mount_type, + mount_point.mount_condition))); + } + + // Adds disks and mount points to disk mount manager. + void InitDisksAndMountPoints() { + // Disks should be added first (when adding device mount points it is + // expected that the corresponding disk is already added). + for (size_t i = 0; i < arraysize(kTestDisks); i++) + AddTestDisk(kTestDisks[i]); + + for (size_t i = 0; i < arraysize(kTestMountPoints); i++) + AddTestMountPoint(kTestMountPoints[i]); + } + + protected: + MockCrosDisksClient* mock_cros_disks_client_; + MockDiskMountManagerObserver observer_; + MessageLoopForUI message_loop_; +}; + +// Tests that the observer gets notified on attempt to format non existent mount +// point. +TEST_F(DiskMountManagerTest, Format_NotMounted) { + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED, + chromeos::FORMAT_ERROR_UNKNOWN, + "/mount/non_existent")) + .Times(1); + DiskMountManager::GetInstance()->FormatMountedDevice("/mount/non_existent"); +} + +// Tests that it is not possible to format archive mount point. +TEST_F(DiskMountManagerTest, Format_Archive) { + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED, + chromeos::FORMAT_ERROR_UNKNOWN, + "/archive/source_path")) + .Times(1); + + DiskMountManager::GetInstance()->FormatMountedDevice("/archive/mount_path"); +} + +// Tests that format fails if the device cannot be unmounted. +TEST_F(DiskMountManagerTest, Format_FailToUnmount) { + // Set up cros disks client mocks. + // Before formatting mounted device, the device should be unmounted. + // In this test unmount will fail, and there should be no attempt to + // format the device. + EXPECT_CALL(*mock_cros_disks_client_, + Unmount("/device/mount_path", chromeos::UNMOUNT_OPTIONS_NONE, + _, _)) + .WillOnce(MockUnmountPath(false)); + + EXPECT_CALL(*mock_cros_disks_client_, + FormatDevice("/device/mount_path", _, _, _)) + .Times(0); + + // Set up expectations for observer mock. + // Observer should be notified that unmount attempt fails and format task + // failed to start. + { + InSequence s; + + EXPECT_CALL(observer_, + OnMountEvent(DiskMountManager::UNMOUNTING, + chromeos::MOUNT_ERROR_INTERNAL, + Field(&DiskMountManager::MountPointInfo::mount_path, + "/device/mount_path"))) + .Times(1); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED, + chromeos::FORMAT_ERROR_UNKNOWN, + "/device/source_path")) + .Times(1); + } + + // Start test. + DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path"); + + // Cros disks will respond asynchronoulsy, so let's drain the message loop. + message_loop_.RunUntilIdle(); + + // The device mount should still be here. + EXPECT_TRUE(HasMountPoint("/device/mount_path")); +} + +// Tests that observer is notified when cros disks fails to start format +// process. +TEST_F(DiskMountManagerTest, Format_FormatFailsToStart) { + // Set up cros disks client mocks. + // Before formatting mounted device, the device should be unmounted. + // In this test, unmount will succeed, but call to FormatDevice method will + // fail. + EXPECT_CALL(*mock_cros_disks_client_, + Unmount("/device/mount_path", chromeos::UNMOUNT_OPTIONS_NONE, + _, _)) + .WillOnce(MockUnmountPath(true)); + + EXPECT_CALL(*mock_cros_disks_client_, + FormatDevice("/device/source_path", "vfat", _, _)) + .WillOnce(MockFormatDevice(false)); + + // Set up expectations for observer mock. + // Observer should be notified that the device was unmounted and format task + // failed to start. + { + InSequence s; + + EXPECT_CALL(observer_, + OnMountEvent(DiskMountManager::UNMOUNTING, + chromeos::MOUNT_ERROR_NONE, + Field(&DiskMountManager::MountPointInfo::mount_path, + "/device/mount_path"))) + .Times(1); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED, + chromeos::FORMAT_ERROR_UNKNOWN, + "/device/source_path")) + .Times(1); + } + + // Start the test. + DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path"); + + // Cros disks will respond asynchronoulsy, so let's drain the message loop. + message_loop_.RunUntilIdle(); + + // The device mount should be gone. + EXPECT_FALSE(HasMountPoint("/device/mount_path")); +} + +// 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 + // should be an error before device unmount is attempted). + // 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)); + + EXPECT_CALL(*mock_cros_disks_client_, + FormatDevice("/device/source_path", "vfat", _, _)) + .WillOnce(MockFormatDevice(true)); + + // Set up expectations for observer mock. + // 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 + // unmounted for 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, + Field(&DiskMountManager::MountPointInfo::mount_path, + "/device/mount_path"))) + .Times(1); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED, + chromeos::FORMAT_ERROR_NONE, + "/device/source_path")) + .Times(1); + } + + // Start the test. + DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path"); + DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path"); + + // Cros disks will respond asynchronoulsy, so let's drain the message loop. + message_loop_.RunUntilIdle(); + + // The device mount should be gone. + EXPECT_FALSE(HasMountPoint("/device/mount_path")); +} + +// Tests the case when the format process actually starts and fails. +TEST_F(DiskMountManagerTest, Format_FormatFails) { + // Set up cros disks client mocks. + // Both unmount and format device cals are successfull in this test. + EXPECT_CALL(*mock_cros_disks_client_, + Unmount("/device/mount_path", chromeos::UNMOUNT_OPTIONS_NONE, + _, _)) + .WillOnce(MockUnmountPath(true)); + + EXPECT_CALL(*mock_cros_disks_client_, + FormatDevice("/device/source_path", "vfat", _, _)) + .WillOnce(MockFormatDevice(true)); + + // Set up expectations for observer mock. + // The observer should get notified that the device was unmounted and that + // formatting has started. + // After the formatting starts, the test will simulate failing + // FORMATTING_FINISHED signal, so the observer should also be notified the + // formatting has failed (FORMAT_COMPLETED event). + { + InSequence s; + + EXPECT_CALL(observer_, + OnMountEvent(DiskMountManager::UNMOUNTING, + chromeos::MOUNT_ERROR_NONE, + Field(&DiskMountManager::MountPointInfo::mount_path, + "/device/mount_path"))) + .Times(1); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED, + chromeos::FORMAT_ERROR_NONE, + "/device/source_path")) + .Times(1); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED, + chromeos::FORMAT_ERROR_UNKNOWN, + "/device/source_path")) + .Times(1); + } + + // Start the test. + DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path"); + + // Wait for Unmount and FormatDevice calls to end. + message_loop_.RunUntilIdle(); + + // The device should be unmounted by now. + EXPECT_FALSE(HasMountPoint("/device/mount_path")); + + // Send failing FORMATTING_FINISHED signal. + // The failure is marked by ! in fromt of the path (but this should change + // soon). + mock_cros_disks_client_->SendMountEvent( + chromeos::CROS_DISKS_FORMATTING_FINISHED, "!/device/source_path"); +} + +// Tests the same case as Format_FormatFails, but the FORMATTING_FINISHED event +// is sent with file_path of the formatted device (instead of its device path). +TEST_F(DiskMountManagerTest, Format_FormatFailsAndReturnFilePath) { + // Set up cros disks client mocks. + EXPECT_CALL(*mock_cros_disks_client_, + Unmount("/device/mount_path", chromeos::UNMOUNT_OPTIONS_NONE, + _, _)) + .WillOnce(MockUnmountPath(true)); + + EXPECT_CALL(*mock_cros_disks_client_, + FormatDevice("/device/source_path", "vfat", _, _)) + .WillOnce(MockFormatDevice(true)); + + // Set up expectations for observer mock. + { + InSequence s; + + EXPECT_CALL(observer_, + OnMountEvent(DiskMountManager::UNMOUNTING, + chromeos::MOUNT_ERROR_NONE, + Field(&DiskMountManager::MountPointInfo::mount_path, + "/device/mount_path"))) + .Times(1); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED, + chromeos::FORMAT_ERROR_NONE, + "/device/source_path")) + .Times(1); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED, + chromeos::FORMAT_ERROR_UNKNOWN, + "/device/source_path")) + .Times(1); + } + + // Start test. + DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path"); + + // Wait for Unmount and FormatDevice calls to end. + message_loop_.RunUntilIdle(); + + // The device should be unmounted by now. + EXPECT_FALSE(HasMountPoint("/device/mount_path")); + + // Send failing FORMATTING_FINISHED signal with the device's file path. + mock_cros_disks_client_->SendMountEvent( + chromeos::CROS_DISKS_FORMATTING_FINISHED, "!/device/file_path"); +} + +// Tests the case when formatting completes successfully. +TEST_F(DiskMountManagerTest, Format_FormatSuccess) { + // Set up cros disks client mocks. + // Both unmount and format device cals are successfull in this test. + EXPECT_CALL(*mock_cros_disks_client_, + Unmount("/device/mount_path", chromeos::UNMOUNT_OPTIONS_NONE, + _, _)) + .WillOnce(MockUnmountPath(true)); + + EXPECT_CALL(*mock_cros_disks_client_, + FormatDevice("/device/source_path", "vfat", _, _)) + .WillOnce(MockFormatDevice(true)); + + // Set up expectations for observer mock. + // The observer should receive UNMOUNTING, FORMAT_STARTED and FORMAT_COMPLETED + // events (all of them without an error set). + { + InSequence s; + + EXPECT_CALL(observer_, + OnMountEvent(DiskMountManager::UNMOUNTING, + chromeos::MOUNT_ERROR_NONE, + Field(&DiskMountManager::MountPointInfo::mount_path, + "/device/mount_path"))) + .Times(1); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED, + chromeos::FORMAT_ERROR_NONE, + "/device/source_path")) + .Times(1); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED, + chromeos::FORMAT_ERROR_NONE, + "/device/source_path")) + .Times(1); + } + + // Start the test. + DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path"); + + // Wait for Unmount and FormatDevice calls to end. + message_loop_.RunUntilIdle(); + + // The device should be unmounted by now. + EXPECT_FALSE(HasMountPoint("/device/mount_path")); + + // Simulate cros_disks reporting success. + mock_cros_disks_client_->SendMountEvent( + chromeos::CROS_DISKS_FORMATTING_FINISHED, "/device/source_path"); +} + +// Tests that it's possible to format the device twice in a row (this may not be +// true if the list of pending formats is not properly cleared). +TEST_F(DiskMountManagerTest, Format_ConsecutiveFormatCalls) { + // Set up cros disks client mocks. + // All unmount and format device cals are successfull in this test. + // Each of the should be made twice (once for each formatting task). + EXPECT_CALL(*mock_cros_disks_client_, + Unmount("/device/mount_path", chromeos::UNMOUNT_OPTIONS_NONE, + _, _)) + .WillOnce(MockUnmountPath(true)) + .WillOnce(MockUnmountPath(true)); + + EXPECT_CALL(*mock_cros_disks_client_, + FormatDevice("/device/source_path", "vfat", _, _)) + .WillOnce(MockFormatDevice(true)) + .WillOnce(MockFormatDevice(true)); + + // Set up expectations for observer mock. + // The observer should receive UNMOUNTING, FORMAT_STARTED and FORMAT_COMPLETED + // events (all of them without an error set) twice (once for each formatting + // task). + // Also, there should be a MOUNTING event when the device remounting is + // simulated. + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_COMPLETED, + chromeos::FORMAT_ERROR_NONE, + "/device/source_path")) + .Times(2); + + EXPECT_CALL(observer_, OnFormatEvent(DiskMountManager::FORMAT_STARTED, + chromeos::FORMAT_ERROR_NONE, + "/device/source_path")) + .Times(2); + + EXPECT_CALL(observer_, + OnMountEvent(DiskMountManager::UNMOUNTING, + chromeos::MOUNT_ERROR_NONE, + Field(&DiskMountManager::MountPointInfo::mount_path, + "/device/mount_path"))) + .Times(2); + + EXPECT_CALL(observer_, + OnMountEvent(DiskMountManager::MOUNTING, + chromeos::MOUNT_ERROR_NONE, + Field(&DiskMountManager::MountPointInfo::mount_path, + "/device/mount_path"))) + .Times(1); + + // Start the test. + DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path"); + + // Wait for Unmount and FormatDevice calls to end. + message_loop_.RunUntilIdle(); + + // The device should be unmounted by now. + EXPECT_FALSE(HasMountPoint("/device/mount_path")); + + // Simulate cros_disks reporting success. + mock_cros_disks_client_->SendMountEvent( + chromeos::CROS_DISKS_FORMATTING_FINISHED, "/device/source_path"); + + // Simulate the device remounting. + mock_cros_disks_client_->SendMountCompletedEvent( + chromeos::MOUNT_ERROR_NONE, + "/device/source_path", + chromeos::MOUNT_TYPE_DEVICE, + "/device/mount_path"); + + EXPECT_TRUE(HasMountPoint("/device/mount_path")); + + // Try formatting again. + DiskMountManager::GetInstance()->FormatMountedDevice("/device/mount_path"); + + // Wait for Unmount and FormatDevice calls to end. + message_loop_.RunUntilIdle(); + + // Simulate cros_disks reporting success. + mock_cros_disks_client_->SendMountEvent( + chromeos::CROS_DISKS_FORMATTING_FINISHED, "/device/source_path"); +} + +} // namespace + diff --git a/chromeos/disks/mock_disk_mount_manager.cc b/chromeos/disks/mock_disk_mount_manager.cc index 9b6171e..2372720 100644 --- a/chromeos/disks/mock_disk_mount_manager.cc +++ b/chromeos/disks/mock_disk_mount_manager.cc @@ -92,13 +92,10 @@ void MockDiskMountManager::NotifyDeviceInsertEvents() { std::string(kTestDevicePath), disk1.get())); // Device Added - DiskMountManagerEventType event; - event = MOUNT_DEVICE_ADDED; - NotifyDeviceChanged(event, kTestSystemPath); + NotifyDeviceChanged(DEVICE_ADDED, kTestSystemPath); // Disk Added - event = MOUNT_DISK_ADDED; - NotifyDiskChanged(event, disk1.get()); + NotifyDiskChanged(DISK_ADDED, disk1.get()); // Disk Changed scoped_ptr disk2(new DiskMountManager::Disk( @@ -124,8 +121,7 @@ void MockDiskMountManager::NotifyDeviceInsertEvents() { disks_.clear(); disks_.insert(std::pair( std::string(kTestDevicePath), disk2.get())); - event = MOUNT_DISK_CHANGED; - NotifyDiskChanged(event, disk2.get()); + NotifyDiskChanged(DISK_CHANGED, disk2.get()); } void MockDiskMountManager::NotifyDeviceRemoveEvents() { @@ -152,7 +148,7 @@ void MockDiskMountManager::NotifyDeviceRemoveEvents() { disks_.clear(); disks_.insert(std::pair( std::string(kTestDevicePath), disk.get())); - NotifyDiskChanged(MOUNT_DISK_REMOVED, disk.get()); + NotifyDiskChanged(DISK_REMOVED, disk.get()); } void MockDiskMountManager::SetupDefaultReplies() { @@ -172,8 +168,6 @@ void MockDiskMountManager::SetupDefaultReplies() { .Times(AnyNumber()); EXPECT_CALL(*this, UnmountPath(_, _)) .Times(AnyNumber()); - EXPECT_CALL(*this, FormatUnmountedDevice(_)) - .Times(AnyNumber()); EXPECT_CALL(*this, FormatMountedDevice(_)) .Times(AnyNumber()); EXPECT_CALL(*this, UnmountDeviceRecursive(_, _, _)) @@ -234,14 +228,14 @@ MockDiskMountManager::FindDiskBySourcePathInternal( } void MockDiskMountManager::NotifyDiskChanged( - DiskMountManagerEventType event, + DiskEvent event, const DiskMountManager::Disk* disk) { - FOR_EACH_OBSERVER(Observer, observers_, DiskChanged(event, disk)); + FOR_EACH_OBSERVER(Observer, observers_, OnDiskEvent(event, disk)); } -void MockDiskMountManager::NotifyDeviceChanged(DiskMountManagerEventType event, +void MockDiskMountManager::NotifyDeviceChanged(DeviceEvent event, const std::string& path) { - FOR_EACH_OBSERVER(Observer, observers_, DeviceChanged(event, path)); + FOR_EACH_OBSERVER(Observer, observers_, OnDeviceEvent(event, path)); } } // namespace disks diff --git a/chromeos/disks/mock_disk_mount_manager.h b/chromeos/disks/mock_disk_mount_manager.h index e6b8e89..badf526 100644 --- a/chromeos/disks/mock_disk_mount_manager.h +++ b/chromeos/disks/mock_disk_mount_manager.h @@ -33,7 +33,6 @@ class MockDiskMountManager : public DiskMountManager { MOCK_METHOD4(MountPath, void(const std::string&, const std::string&, const std::string&, MountType)); MOCK_METHOD2(UnmountPath, void(const std::string&, UnmountOptions)); - MOCK_METHOD1(FormatUnmountedDevice, void(const std::string&)); MOCK_METHOD1(FormatMountedDevice, void(const std::string&)); MOCK_METHOD3(UnmountDeviceRecursive, void(const std::string&, DiskMountManager::UnmountDeviceRecursiveCallbackType, void*)); @@ -76,11 +75,11 @@ class MockDiskMountManager : public DiskMountManager { const std::string& source_path) const; // Notifies observers about device status update. - void NotifyDeviceChanged(DiskMountManagerEventType event, + void NotifyDeviceChanged(DeviceEvent event, const std::string& path); // Notifies observers about disk status update. - void NotifyDiskChanged(DiskMountManagerEventType event, + void NotifyDiskChanged(DiskEvent event, const DiskMountManager::Disk* disk); // The list of observers. -- cgit v1.1