diff options
-rw-r--r-- | chromeos/dbus/cros_disks_client.cc | 59 | ||||
-rw-r--r-- | chromeos/dbus/cros_disks_client.h | 42 | ||||
-rw-r--r-- | chromeos/dbus/fake_cros_disks_client.cc | 23 | ||||
-rw-r--r-- | chromeos/dbus/fake_cros_disks_client.h | 13 | ||||
-rw-r--r-- | chromeos/disks/disk_mount_manager.cc | 76 | ||||
-rw-r--r-- | chromeos/disks/disk_mount_manager_unittest.cc | 28 |
6 files changed, 154 insertions, 87 deletions
diff --git a/chromeos/dbus/cros_disks_client.cc b/chromeos/dbus/cros_disks_client.cc index fae607d..39bdb6f 100644 --- a/chromeos/dbus/cros_disks_client.cc +++ b/chromeos/dbus/cros_disks_client.cc @@ -183,9 +183,8 @@ class CrosDisksClientImpl : public CrosDisksClient { } // CrosDisksClient override. - virtual void SetUpConnections( - const MountEventHandler& mount_event_handler, - const MountCompletedHandler& mount_completed_handler) OVERRIDE { + virtual void SetMountEventHandler( + const MountEventHandler& mount_event_handler) OVERRIDE { static const SignalEventTuple kSignalEventTuples[] = { { cros_disks::kDeviceAdded, CROS_DISKS_DEVICE_ADDED }, { cros_disks::kDeviceScanned, CROS_DISKS_DEVICE_SCANNED }, @@ -193,7 +192,6 @@ class CrosDisksClientImpl : public CrosDisksClient { { cros_disks::kDiskAdded, CROS_DISKS_DISK_ADDED }, { cros_disks::kDiskChanged, CROS_DISKS_DISK_CHANGED }, { cros_disks::kDiskRemoved, CROS_DISKS_DISK_REMOVED }, - { cros_disks::kFormattingFinished, CROS_DISKS_FORMATTING_FINISHED }, }; const size_t kNumSignalEventTuples = arraysize(kSignalEventTuples); @@ -208,6 +206,11 @@ class CrosDisksClientImpl : public CrosDisksClient { base::Bind(&CrosDisksClientImpl::OnSignalConnected, weak_ptr_factory_.GetWeakPtr())); } + } + + // CrosDisksClient override. + virtual void SetMountCompletedHandler( + const MountCompletedHandler& mount_completed_handler) OVERRIDE { proxy_->ConnectToSignal( cros_disks::kCrosDisksInterface, cros_disks::kMountCompleted, @@ -218,6 +221,19 @@ class CrosDisksClientImpl : public CrosDisksClient { weak_ptr_factory_.GetWeakPtr())); } + // CrosDisksClient override. + virtual void SetFormatCompletedHandler( + const FormatCompletedHandler& format_completed_handler) OVERRIDE { + proxy_->ConnectToSignal( + cros_disks::kCrosDisksInterface, + cros_disks::kFormatCompleted, + base::Bind(&CrosDisksClientImpl::OnFormatCompleted, + weak_ptr_factory_.GetWeakPtr(), + format_completed_handler), + base::Bind(&CrosDisksClientImpl::OnSignalConnected, + weak_ptr_factory_.GetWeakPtr())); + } + protected: virtual void Init(dbus::Bus* bus) OVERRIDE { proxy_ = bus->GetObjectProxy( @@ -227,7 +243,7 @@ class CrosDisksClientImpl : public CrosDisksClient { private: // A struct to contain a pair of signal name and mount event type. - // Used by SetUpConnections. + // Used by SetMountEventHandler. struct SignalEventTuple { const char *signal_name; MountEventType event_type; @@ -262,7 +278,7 @@ class CrosDisksClientImpl : public CrosDisksClient { // make this fail if reader is not able to read the error code value from // the response. dbus::MessageReader reader(response); - unsigned int error_code; + uint32 error_code = 0; if (reader.PopUint32(&error_code) && static_cast<MountError>(error_code) != MOUNT_ERROR_NONE) { error_callback.Run(); @@ -341,9 +357,9 @@ class CrosDisksClientImpl : public CrosDisksClient { // Handles MountCompleted signal and calls |handler|. void OnMountCompleted(MountCompletedHandler handler, dbus::Signal* signal) { dbus::MessageReader reader(signal); - unsigned int error_code = 0; + uint32 error_code = 0; std::string source_path; - unsigned int mount_type = 0; + uint32 mount_type = 0; std::string mount_path; if (!reader.PopUint32(&error_code) || !reader.PopString(&source_path) || @@ -356,6 +372,18 @@ class CrosDisksClientImpl : public CrosDisksClient { static_cast<MountType>(mount_type), mount_path); } + // Handles FormatCompleted signal and calls |handler|. + void OnFormatCompleted(FormatCompletedHandler handler, dbus::Signal* signal) { + dbus::MessageReader reader(signal); + uint32 error_code = 0; + std::string device_path; + if (!reader.PopUint32(&error_code) || !reader.PopString(&device_path)) { + LOG(ERROR) << "Invalid signal: " << signal->ToString(); + return; + } + handler.Run(static_cast<FormatError>(error_code), device_path); + } + // Handles the result of signal connection setup. void OnSignalConnected(const std::string& interface, const std::string& signal, @@ -458,13 +486,21 @@ class CrosDisksClientStubImpl : public CrosDisksClient { base::MessageLoopProxy::current()->PostTask(FROM_HERE, error_callback); } - virtual void SetUpConnections( - const MountEventHandler& mount_event_handler, - const MountCompletedHandler& mount_completed_handler) OVERRIDE { + virtual void SetMountEventHandler( + const MountEventHandler& mount_event_handler) OVERRIDE { mount_event_handler_ = mount_event_handler; + } + + virtual void SetMountCompletedHandler( + const MountCompletedHandler& mount_completed_handler) OVERRIDE { mount_completed_handler_ = mount_completed_handler; } + virtual void SetFormatCompletedHandler( + const FormatCompletedHandler& format_completed_handler) OVERRIDE { + format_completed_handler_ = format_completed_handler; + } + private: // Performs file actions for Mount(). static MountError PerformFakeMount(const std::string& source_path, @@ -532,6 +568,7 @@ class CrosDisksClientStubImpl : public CrosDisksClient { MountEventHandler mount_event_handler_; MountCompletedHandler mount_completed_handler_; + FormatCompletedHandler format_completed_handler_; base::WeakPtrFactory<CrosDisksClientStubImpl> weak_ptr_factory_; diff --git a/chromeos/dbus/cros_disks_client.h b/chromeos/dbus/cros_disks_client.h index 92871da82..3cd9ee9 100644 --- a/chromeos/dbus/cros_disks_client.h +++ b/chromeos/dbus/cros_disks_client.h @@ -91,7 +91,6 @@ enum MountEventType { CROS_DISKS_DEVICE_ADDED, CROS_DISKS_DEVICE_REMOVED, CROS_DISKS_DEVICE_SCANNED, - CROS_DISKS_FORMATTING_FINISHED, }; // Additional unmount flags to be added to unmount request. @@ -192,8 +191,8 @@ class CHROMEOS_EXPORT CrosDisksClient : public DBusClient { public: // A callback to handle the result of EnumerateAutoMountableDevices. // The argument is the enumerated device paths. - typedef base::Callback<void(const std::vector<std::string>& device_paths) - > EnumerateAutoMountableDevicesCallback; + typedef base::Callback<void(const std::vector<std::string>& device_paths)> + EnumerateAutoMountableDevicesCallback; // A callback to handle the result of FormatDevice. // The argument is true when formatting succeeded. @@ -201,8 +200,8 @@ class CHROMEOS_EXPORT CrosDisksClient : public DBusClient { // A callback to handle the result of GetDeviceProperties. // The argument is the information about the specified device. - typedef base::Callback<void(const DiskInfo& disk_info) - > GetDevicePropertiesCallback; + typedef base::Callback<void(const DiskInfo& disk_info)> + GetDevicePropertiesCallback; // A callback to handle MountCompleted signal. // The first argument is the error code. @@ -212,15 +211,22 @@ class CHROMEOS_EXPORT CrosDisksClient : public DBusClient { typedef base::Callback<void(MountError error_code, const std::string& source_path, MountType mount_type, - const std::string& mount_path) - > MountCompletedHandler; + const std::string& mount_path)> + MountCompletedHandler; + + // A callback to handle FormatCompleted signal. + // The first argument is the error code. + // The second argument is the device path. + typedef base::Callback<void(FormatError error_code, + const std::string& device_path)> + FormatCompletedHandler; // A callback to handle mount events. // The first argument is the event type. // The second argument is the device path. typedef base::Callback<void(MountEventType event_type, - const std::string& device_path) - > MountEventHandler; + const std::string& device_path)> + MountEventHandler; virtual ~CrosDisksClient(); @@ -265,13 +271,21 @@ class CHROMEOS_EXPORT CrosDisksClient : public DBusClient { const GetDevicePropertiesCallback& callback, const base::Closure& error_callback) = 0; - // Registers given callback for events. - // |mount_event_handler| is called when mount event signal is received. - // |mount_completed_handler| is called when MountCompleted signal is received. - virtual void SetUpConnections( - const MountEventHandler& mount_event_handler, + // Registers |mount_event_handler| as a callback to be invoked when a mount + // event signal is received. + virtual void SetMountEventHandler( + const MountEventHandler& mount_event_handler) = 0; + + // Registers |mount_completed_handler| as a callback to be invoked when a + // MountCompleted signal is received. + virtual void SetMountCompletedHandler( const MountCompletedHandler& mount_completed_handler) = 0; + // Registers |format_completed_handler| as a callback to be invoked when a + // FormatCompleted signal is received. + virtual void SetFormatCompletedHandler( + const FormatCompletedHandler& format_completed_handler) = 0; + // Factory function, creates a new instance and returns ownership. // For normal usage, access the singleton via DBusThreadManager::Get(). static CrosDisksClient* Create(DBusClientImplementationType type); diff --git a/chromeos/dbus/fake_cros_disks_client.cc b/chromeos/dbus/fake_cros_disks_client.cc index 2578077..8e96bf2 100644 --- a/chromeos/dbus/fake_cros_disks_client.cc +++ b/chromeos/dbus/fake_cros_disks_client.cc @@ -75,13 +75,21 @@ void FakeCrosDisksClient::GetDeviceProperties( const base::Closure& error_callback) { } -void FakeCrosDisksClient::SetUpConnections( - const MountEventHandler& mount_event_handler, - const MountCompletedHandler& mount_completed_handler) { +void FakeCrosDisksClient::SetMountEventHandler( + const MountEventHandler& mount_event_handler) { mount_event_handler_ = mount_event_handler; +} + +void FakeCrosDisksClient::SetMountCompletedHandler( + const MountCompletedHandler& mount_completed_handler) { mount_completed_handler_ = mount_completed_handler; } +void FakeCrosDisksClient::SetFormatCompletedHandler( + const FormatCompletedHandler& format_completed_handler) { + format_completed_handler_ = format_completed_handler; +} + bool FakeCrosDisksClient::SendMountEvent(MountEventType event, const std::string& path) { if (mount_event_handler_.is_null()) @@ -101,4 +109,13 @@ bool FakeCrosDisksClient::SendMountCompletedEvent( return true; } +bool FakeCrosDisksClient::SendFormatCompletedEvent( + FormatError error_code, + const std::string& device_path) { + if (format_completed_handler_.is_null()) + return false; + format_completed_handler_.Run(error_code, device_path); + return true; +} + } // namespace chromeos diff --git a/chromeos/dbus/fake_cros_disks_client.h b/chromeos/dbus/fake_cros_disks_client.h index 855a3c1..f51a4be 100644 --- a/chromeos/dbus/fake_cros_disks_client.h +++ b/chromeos/dbus/fake_cros_disks_client.h @@ -41,17 +41,23 @@ class FakeCrosDisksClient : public CrosDisksClient { const std::string& device_path, const GetDevicePropertiesCallback& callback, const base::Closure& error_callback) OVERRIDE; - virtual void SetUpConnections( - const MountEventHandler& mount_event_handler, + virtual void SetMountEventHandler( + const MountEventHandler& mount_event_handler) OVERRIDE; + virtual void SetMountCompletedHandler( const MountCompletedHandler& mount_completed_handler) OVERRIDE; + virtual void SetFormatCompletedHandler( + const FormatCompletedHandler& format_completed_handler) OVERRIDE; // Used in tests to simulate signals sent by cros disks layer. - // Invokes handlers set in |SetUpConnections|. + // Invokes handlers set in |SetMountEventHandler|, |SetMountCompletedHandler|, + // and |SetFormatCompletedHandler|. bool SendMountEvent(MountEventType event, const std::string& path); bool SendMountCompletedEvent(MountError error_code, const std::string& source_path, MountType mount_type, const std::string& mount_path); + bool SendFormatCompletedEvent(FormatError error_code, + const std::string& device_path); // Returns how many times Unmount() was called. int unmount_call_count() const { @@ -105,6 +111,7 @@ class FakeCrosDisksClient : public CrosDisksClient { private: MountEventHandler mount_event_handler_; MountCompletedHandler mount_completed_handler_; + FormatCompletedHandler format_completed_handler_; int unmount_call_count_; std::string last_unmount_device_path_; diff --git a/chromeos/disks/disk_mount_manager.cc b/chromeos/disks/disk_mount_manager.cc index a781107..75ab642 100644 --- a/chromeos/disks/disk_mount_manager.cc +++ b/chromeos/disks/disk_mount_manager.cc @@ -31,11 +31,15 @@ class DiskMountManagerImpl : public DiskMountManager { DCHECK(dbus_thread_manager); cros_disks_client_ = dbus_thread_manager->GetCrosDisksClient(); DCHECK(cros_disks_client_); - cros_disks_client_->SetUpConnections( + cros_disks_client_->SetMountEventHandler( base::Bind(&DiskMountManagerImpl::OnMountEvent, - weak_ptr_factory_.GetWeakPtr()), + weak_ptr_factory_.GetWeakPtr())); + cros_disks_client_->SetMountCompletedHandler( base::Bind(&DiskMountManagerImpl::OnMountCompleted, weak_ptr_factory_.GetWeakPtr())); + cros_disks_client_->SetFormatCompletedHandler( + base::Bind(&DiskMountManagerImpl::OnFormatCompleted, + weak_ptr_factory_.GetWeakPtr())); } virtual ~DiskMountManagerImpl() { @@ -280,7 +284,7 @@ class DiskMountManagerImpl : public DiskMountManager { if (success) { // Do standard processing for Unmount event. OnUnmountPath(UnmountPathCallback(), true, mount_path); - LOG(INFO) << mount_path << " unmounted."; + VLOG(1) << mount_path << " unmounted."; } // This is safe as long as all callbacks are called on the same thread as // UnmountDeviceRecursively. @@ -407,6 +411,33 @@ class DiskMountManagerImpl : public DiskMountManager { NotifyFormatStatusUpdate(FORMAT_STARTED, error_code, device_path); } + // Callback to handle FormatCompleted signal and Format method call failure. + void OnFormatCompleted(FormatError error_code, + const std::string& device_path) { + std::string actual_device_path; + // Depending on cros-disks implementation the event may return either file + // path or device path. We want to use device path. + // TODO(benchan): This shouldn't be necessary. Find out the issue and ensure + // that cros-disks simply returns the path format that Chrome expects, then + // we should remove the following code. + for (DiskMountManager::DiskMap::const_iterator it = disks_.begin(); + it != disks_.end(); ++it) { + if (it->second->file_path() == device_path || + it->second->device_path() == device_path) { + actual_device_path = it->second->device_path(); + break; + } + } + + if (actual_device_path.empty()) { + LOG(ERROR) << "Error while handling disks metadata. Cannot find " + << "device that is being formatted."; + return; + } + + NotifyFormatStatusUpdate(FORMAT_COMPLETED, error_code, actual_device_path); + } + // Callbcak for GetDeviceProperties. void OnGetDeviceProperties(const DiskInfo& disk_info) { // TODO(zelidrag): Find a better way to filter these out before we @@ -513,20 +544,6 @@ class DiskMountManagerImpl : public DiskMountManager { NotifyDeviceStatusUpdate(DEVICE_SCANNED, device_path); break; } - 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; - } - - LOG(ERROR) << "Error while handling disks metadata. Cannot find " - << "device that is being formatted."; - break; - } default: { LOG(ERROR) << "Unknown event: " << event; } @@ -560,31 +577,6 @@ class DiskMountManagerImpl : public DiskMountManager { OnFormatEvent(event, error_code, device_path)); } - // Converts file path to device path. - 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. - 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() == path || - it->second->device_path() == path) { - *device_path = it->second->device_path(); - return; - } - } - } - // Finds system path prefix from |system_path|. const std::string& FindSystemPathPrefix(const std::string& system_path) { if (system_path.empty()) diff --git a/chromeos/disks/disk_mount_manager_unittest.cc b/chromeos/disks/disk_mount_manager_unittest.cc index 36e67ad..3786dcd3 100644 --- a/chromeos/disks/disk_mount_manager_unittest.cc +++ b/chromeos/disks/disk_mount_manager_unittest.cc @@ -386,7 +386,7 @@ TEST_F(DiskMountManagerTest, Format_FormatFails) { // 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 + // FORMAT_COMPLETED signal, so the observer should also be notified the // formatting has failed (FORMAT_COMPLETED event). { InSequence s; @@ -429,14 +429,14 @@ TEST_F(DiskMountManagerTest, Format_FormatFails) { // The device should be unmounted by now. EXPECT_FALSE(HasMountPoint("/device/mount_path")); - // Send failing FORMATTING_FINISHED signal. + // Send failing FORMAT_COMPLETED signal. // The failure is marked by ! in fromt of the path (but this should change // soon). - fake_cros_disks_client_->SendMountEvent( - chromeos::CROS_DISKS_FORMATTING_FINISHED, "!/device/source_path"); + fake_cros_disks_client_->SendFormatCompletedEvent( + chromeos::FORMAT_ERROR_UNKNOWN, "/device/source_path"); } -// Tests the same case as Format_FormatFails, but the FORMATTING_FINISHED event +// Tests the same case as Format_FormatFails, but the FORMAT_COMPLETED event // is sent with file_path of the formatted device (instead of its device path). TEST_F(DiskMountManagerTest, Format_FormatFailsAndReturnFilePath) { // Set up expectations for observer mock. @@ -481,9 +481,9 @@ TEST_F(DiskMountManagerTest, Format_FormatFailsAndReturnFilePath) { // The device should be unmounted by now. EXPECT_FALSE(HasMountPoint("/device/mount_path")); - // Send failing FORMATTING_FINISHED signal with the device's file path. - fake_cros_disks_client_->SendMountEvent( - chromeos::CROS_DISKS_FORMATTING_FINISHED, "!/device/file_path"); + // Send failing FORMAT_COMPLETED signal with the device's file path. + fake_cros_disks_client_->SendFormatCompletedEvent( + chromeos::FORMAT_ERROR_UNKNOWN, "/device/file_path"); } // Tests the case when formatting completes successfully. @@ -536,8 +536,8 @@ TEST_F(DiskMountManagerTest, Format_FormatSuccess) { EXPECT_FALSE(HasMountPoint("/device/mount_path")); // Simulate cros_disks reporting success. - fake_cros_disks_client_->SendMountEvent( - chromeos::CROS_DISKS_FORMATTING_FINISHED, "/device/source_path"); + fake_cros_disks_client_->SendFormatCompletedEvent( + chromeos::FORMAT_ERROR_NONE, "/device/source_path"); } // Tests that it's possible to format the device twice in a row (this may not be @@ -597,8 +597,8 @@ TEST_F(DiskMountManagerTest, Format_ConsecutiveFormatCalls) { EXPECT_FALSE(HasMountPoint("/device/mount_path")); // Simulate cros_disks reporting success. - fake_cros_disks_client_->SendMountEvent( - chromeos::CROS_DISKS_FORMATTING_FINISHED, "/device/source_path"); + fake_cros_disks_client_->SendFormatCompletedEvent( + chromeos::FORMAT_ERROR_NONE, "/device/source_path"); // Simulate the device remounting. fake_cros_disks_client_->SendMountCompletedEvent( @@ -627,8 +627,8 @@ TEST_F(DiskMountManagerTest, Format_ConsecutiveFormatCalls) { fake_cros_disks_client_->last_format_device_filesystem()); // Simulate cros_disks reporting success. - fake_cros_disks_client_->SendMountEvent( - chromeos::CROS_DISKS_FORMATTING_FINISHED, "/device/source_path"); + fake_cros_disks_client_->SendFormatCompletedEvent( + chromeos::FORMAT_ERROR_NONE, "/device/source_path"); } } // namespace |