From 6e4379b531a3bdf21b79ada2d5e81f2eb7524bb0 Mon Sep 17 00:00:00 2001 From: Anton Swifton Date: Wed, 27 Oct 2021 06:54:46 +0000 Subject: [PATCH] Add RequestDevices to FirmwareUpdateManager. This function requests the list of currently connected devices from the fwupd DBus client. Bug: 1252981 Test: ash_unittests Change-Id: I16d9b2f47595fce76c82ec35fe686a322b77b913 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3234593 Commit-Queue: Anton Swifton Reviewed-by: Jimmy Gong Cr-Commit-Position: refs/heads/main@{#935320} --- ash/components/fwupd/BUILD.gn | 8 ++++- .../fwupd/firmware_update_manager.cc | 18 ++++++++++++ .../fwupd/firmware_update_manager.h | 22 ++++++++++++-- .../fwupd/firmware_update_manager_unittest.cc | 18 ++++++++++++ chromeos/dbus/fwupd/BUILD.gn | 2 ++ chromeos/dbus/fwupd/fake_fwupd_client.cc | 10 ++++++- chromeos/dbus/fwupd/fwupd_client.cc | 25 ++++++++++++---- chromeos/dbus/fwupd/fwupd_client.h | 19 ++++++++++-- chromeos/dbus/fwupd/fwupd_client_unittest.cc | 20 ++++++++----- chromeos/dbus/fwupd/fwupd_device.cc | 19 ++++++++++++ chromeos/dbus/fwupd/fwupd_device.h | 29 +++++++++++++++++++ 11 files changed, 171 insertions(+), 19 deletions(-) create mode 100644 chromeos/dbus/fwupd/fwupd_device.cc create mode 100644 chromeos/dbus/fwupd/fwupd_device.h diff --git a/ash/components/fwupd/BUILD.gn b/ash/components/fwupd/BUILD.gn index c82d05a5feaa01..b3c5d9c9bdc406 100644 --- a/ash/components/fwupd/BUILD.gn +++ b/ash/components/fwupd/BUILD.gn @@ -9,7 +9,11 @@ assert(is_chromeos_ash, "Non-Chrome-OS builds must not depend on //ash") component("fwupd") { defines = [ "IS_ASH_FIRMWARE_UPDATE_MANAGER_IMPL" ] - deps = [ "//base:base" ] + deps = [ + "//base:base", + "//chromeos/dbus/fwupd", + "//dbus", + ] sources = [ "firmware_update_manager.cc", @@ -22,6 +26,8 @@ source_set("unit_tests") { deps = [ ":fwupd", + "//chromeos/dbus/fwupd", + "//dbus:test_support", "//testing/gtest", ] diff --git a/ash/components/fwupd/firmware_update_manager.cc b/ash/components/fwupd/firmware_update_manager.cc index 4fe3a05a3cdde6..4a2fb9126604e1 100644 --- a/ash/components/fwupd/firmware_update_manager.cc +++ b/ash/components/fwupd/firmware_update_manager.cc @@ -5,6 +5,8 @@ #include "ash/components/fwupd/firmware_update_manager.h" #include "base/check_op.h" +#include "chromeos/dbus/fwupd/fwupd_client.h" +#include "dbus/message.h" namespace ash { @@ -15,12 +17,17 @@ FirmwareUpdateManager* g_instance = nullptr; } // namespace FirmwareUpdateManager::FirmwareUpdateManager() { + DCHECK(chromeos::FwupdClient::Get()); + chromeos::FwupdClient::Get()->AddObserver(this); + DCHECK_EQ(nullptr, g_instance); g_instance = this; + g_instance->RequestDevices(); } FirmwareUpdateManager::~FirmwareUpdateManager() { DCHECK_EQ(this, g_instance); + chromeos::FwupdClient::Get()->RemoveObserver(this); g_instance = nullptr; } @@ -30,4 +37,15 @@ FirmwareUpdateManager* FirmwareUpdateManager::Get() { return g_instance; } +void FirmwareUpdateManager::RequestDevices() { + chromeos::FwupdClient::Get()->GetDevices(); +} + +void FirmwareUpdateManager::OnDeviceListResponse( + chromeos::FwupdDeviceList* devices) { + DCHECK(devices); + // TODO(swifton): This is a stub implementation. + ++on_device_list_response_count_for_testing_; +} + } // namespace ash diff --git a/ash/components/fwupd/firmware_update_manager.h b/ash/components/fwupd/firmware_update_manager.h index b0c5bd5230f103..9166a635053efe 100644 --- a/ash/components/fwupd/firmware_update_manager.h +++ b/ash/components/fwupd/firmware_update_manager.h @@ -6,18 +6,36 @@ #define ASH_COMPONENTS_FWUPD_FIRMWARE_UPDATE_MANAGER_H_ #include "base/component_export.h" +#include "chromeos/dbus/fwupd/fwupd_client.h" +#include "chromeos/dbus/fwupd/fwupd_device.h" +#include "dbus/message.h" namespace ash { // FirmwareUpdateManager contains all logic that runs the firmware update SWA. -class COMPONENT_EXPORT(ASH_FIRMWARE_UPDATE_MANAGER) FirmwareUpdateManager { +class COMPONENT_EXPORT(ASH_FIRMWARE_UPDATE_MANAGER) FirmwareUpdateManager + : public chromeos::FwupdClient::Observer { public: + // Query the fwupd DBus client for currently connected devices. + void RequestDevices(); + + // FwupdClient::Observer: + // When the fwupd DBus client gets a response with devices from fwupd, + // it calls this function and passes the response. + void OnDeviceListResponse(chromeos::FwupdDeviceList* devices) override; + FirmwareUpdateManager(); FirmwareUpdateManager(const FirmwareUpdateManager&) = delete; FirmwareUpdateManager& operator=(const FirmwareUpdateManager&) = delete; - ~FirmwareUpdateManager(); + ~FirmwareUpdateManager() override; // Gets the global instance pointer. static FirmwareUpdateManager* Get(); + + protected: + friend class FirmwareUpdateManagerTest; + // Temporary auxiliary variable for testing. + // TODO(swifton): Replace with mock observers. + int on_device_list_response_count_for_testing_ = 0; }; } // namespace ash diff --git a/ash/components/fwupd/firmware_update_manager_unittest.cc b/ash/components/fwupd/firmware_update_manager_unittest.cc index 67e26ed111f964..901154d11959f4 100644 --- a/ash/components/fwupd/firmware_update_manager_unittest.cc +++ b/ash/components/fwupd/firmware_update_manager_unittest.cc @@ -4,6 +4,9 @@ #include "ash/components/fwupd/firmware_update_manager.h" +#include + +#include "chromeos/dbus/fwupd/fake_fwupd_client.h" #include "testing/gtest/include/gtest/gtest.h" namespace ash { @@ -15,6 +18,21 @@ class FirmwareUpdateManagerTest : public testing::Test { FirmwareUpdateManagerTest& operator=(const FirmwareUpdateManagerTest&) = delete; ~FirmwareUpdateManagerTest() override = default; + + int GetOnDevicesResponseCallbackCallCountForTesting() { + return firmware_update_manager_.on_device_list_response_count_for_testing_; + } + + chromeos::FakeFwupdClient dbus_client_; + FirmwareUpdateManager firmware_update_manager_; }; +// TODO(swifton): Rewrite this test with an observer. +TEST_F(FirmwareUpdateManagerTest, RequestDeviceList) { + // FirmwareUpdateManager requests devices when it is created. + EXPECT_EQ(1, GetOnDevicesResponseCallbackCallCountForTesting()); + firmware_update_manager_.RequestDevices(); + EXPECT_EQ(2, GetOnDevicesResponseCallbackCallCountForTesting()); +} + } // namespace ash diff --git a/chromeos/dbus/fwupd/BUILD.gn b/chromeos/dbus/fwupd/BUILD.gn index 0f7b7331440a5a..59ea4b72b871dd 100644 --- a/chromeos/dbus/fwupd/BUILD.gn +++ b/chromeos/dbus/fwupd/BUILD.gn @@ -19,6 +19,8 @@ component("fwupd") { "fake_fwupd_client.h", "fwupd_client.cc", "fwupd_client.h", + "fwupd_device.cc", + "fwupd_device.h", ] } diff --git a/chromeos/dbus/fwupd/fake_fwupd_client.cc b/chromeos/dbus/fwupd/fake_fwupd_client.cc index 27c7cdaf84ce0b..f38a579f1ec7ed 100644 --- a/chromeos/dbus/fwupd/fake_fwupd_client.cc +++ b/chromeos/dbus/fwupd/fake_fwupd_client.cc @@ -4,6 +4,8 @@ #include "chromeos/dbus/fwupd/fake_fwupd_client.h" +#include "chromeos/dbus/fwupd/fwupd_device.h" + #include namespace chromeos { @@ -12,6 +14,12 @@ FakeFwupdClient::FakeFwupdClient() = default; FakeFwupdClient::~FakeFwupdClient() = default; void FakeFwupdClient::Init(dbus::Bus* bus) {} void FakeFwupdClient::GetUpgrades(std::string device_id) {} -void FakeFwupdClient::GetDevices() {} + +void FakeFwupdClient::GetDevices() { + // TODO(swifton): This is a stub. + auto devices = std::make_unique(); + for (auto& observer : observers_) + observer.OnDeviceListResponse(devices.get()); +} } // namespace chromeos \ No newline at end of file diff --git a/chromeos/dbus/fwupd/fwupd_client.cc b/chromeos/dbus/fwupd/fwupd_client.cc index 48e33b69de4568..3be18a44c140e7 100644 --- a/chromeos/dbus/fwupd/fwupd_client.cc +++ b/chromeos/dbus/fwupd/fwupd_client.cc @@ -91,9 +91,12 @@ class FwupdClientImpl : public FwupdClient { return; } - // TODO(swifton): This is a stub implementation. Replace this with a - // callback call for FirmwareUpdateHandler when it's implemented. - ++get_devices_callback_call_count_for_testing_; + // TODO(swifton): Get the devices from the response. This just sends an + // empty list right now. + auto devices = std::make_unique(); + + for (auto& observer : observers_) + observer.OnDeviceListResponse(devices.get()); } void OnSignalConnected(const std::string& interface_name, @@ -119,16 +122,28 @@ class FwupdClientImpl : public FwupdClient { base::WeakPtrFactory weak_ptr_factory_{this}; }; -FwupdClientImpl::FwupdClientImpl() { +void FwupdClient::AddObserver(FwupdClient::Observer* observer) { + observers_.AddObserver(observer); +} + +void FwupdClient::RemoveObserver(FwupdClient::Observer* observer) { + observers_.RemoveObserver(observer); +} + +FwupdClient::FwupdClient() { DCHECK(!g_instance); g_instance = this; } -FwupdClientImpl::~FwupdClientImpl() { +FwupdClient::~FwupdClient() { DCHECK_EQ(g_instance, this); g_instance = nullptr; } +FwupdClientImpl::FwupdClientImpl() = default; + +FwupdClientImpl::~FwupdClientImpl() = default; + // static std::unique_ptr FwupdClient::Create() { return std::make_unique(); diff --git a/chromeos/dbus/fwupd/fwupd_client.h b/chromeos/dbus/fwupd/fwupd_client.h index e7340d3aa4af4a..7044b44f397bae 100644 --- a/chromeos/dbus/fwupd/fwupd_client.h +++ b/chromeos/dbus/fwupd/fwupd_client.h @@ -8,17 +8,29 @@ #include #include "base/component_export.h" +#include "base/observer_list.h" #include "chromeos/dbus/dbus_client.h" +#include "chromeos/dbus/fwupd/fwupd_device.h" +#include "dbus/message.h" namespace chromeos { // FwupdClient is used for handling signals from the fwupd daemon. class COMPONENT_EXPORT(CHROMEOS_DBUS_FUWPD) FwupdClient : public DBusClient { public: + class Observer : public base::CheckedObserver { + public: + ~Observer() override = default; + virtual void OnDeviceListResponse(FwupdDeviceList* devices) = 0; + }; + + void AddObserver(Observer* observer); + void RemoveObserver(Observer* observer); + // Create() should be used instead. - FwupdClient() = default; + FwupdClient(); FwupdClient(const FwupdClient&) = delete; FwupdClient& operator=(const FwupdClient&) = delete; - ~FwupdClient() override = default; + ~FwupdClient() override; // Factory function. static std::unique_ptr Create(); @@ -40,7 +52,8 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS_FUWPD) FwupdClient : public DBusClient { bool client_is_in_testing_mode_ = false; int device_signal_call_count_for_testing_ = 0; int get_upgrades_callback_call_count_for_testing_ = 0; - int get_devices_callback_call_count_for_testing_ = 0; + + base::ObserverList observers_; }; } // namespace chromeos diff --git a/chromeos/dbus/fwupd/fwupd_client_unittest.cc b/chromeos/dbus/fwupd/fwupd_client_unittest.cc index 25e9eb067c1cff..3262e889ff07f7 100644 --- a/chromeos/dbus/fwupd/fwupd_client_unittest.cc +++ b/chromeos/dbus/fwupd/fwupd_client_unittest.cc @@ -8,6 +8,7 @@ #include "base/memory/scoped_refptr.h" #include "base/test/scoped_feature_list.h" #include "base/test/task_environment.h" +#include "dbus/message.h" #include "dbus/mock_bus.h" #include "dbus/mock_object_proxy.h" #include "testing/gmock/include/gmock/gmock.h" @@ -71,10 +72,6 @@ class FwupdClientTest : public testing::Test { return fwupd_client_->get_upgrades_callback_call_count_for_testing_; } - int GetGetDevicesCallbackCallCount() { - return fwupd_client_->get_devices_callback_call_count_for_testing_; - } - void OnMethodCalled(dbus::MethodCall* method_call, int timeout_ms, dbus::ObjectProxy::ResponseOrErrorCallback* callback) { @@ -141,6 +138,14 @@ class FwupdClientTest : public testing::Test { std::deque dbus_method_call_simulated_results_; }; +class MockObserver : public FwupdClient::Observer { + public: + MOCK_METHOD(void, + OnDeviceListResponse, + (chromeos::FwupdDeviceList * devices), + ()); +}; + // TODO (swifton): Rewrite this test with an observer when it's available. TEST_F(FwupdClientTest, AddOneDevice) { EmitSignal(kFwupdDeviceAddedSignalName); @@ -162,8 +167,11 @@ TEST_F(FwupdClientTest, RequestUpgrades) { EXPECT_EQ(1, GetGetUpgradesCallbackCallCount()); } -// TODO (swifton): Rewrite this test with an observer when it's available. TEST_F(FwupdClientTest, RequestDevices) { + MockObserver observer; + EXPECT_CALL(observer, OnDeviceListResponse(_)).Times(1); + fwupd_client_->AddObserver(&observer); + EXPECT_CALL(*proxy_, DoCallMethodWithErrorResponse(_, _, _)) .WillRepeatedly(Invoke(this, &FwupdClientTest::OnMethodCalled)); @@ -173,8 +181,6 @@ TEST_F(FwupdClientTest, RequestDevices) { fwupd_client_->GetDevices(); base::RunLoop().RunUntilIdle(); - - EXPECT_EQ(1, GetGetDevicesCallbackCallCount()); } } // namespace chromeos diff --git a/chromeos/dbus/fwupd/fwupd_device.cc b/chromeos/dbus/fwupd/fwupd_device.cc new file mode 100644 index 00000000000000..392944db1185e0 --- /dev/null +++ b/chromeos/dbus/fwupd/fwupd_device.cc @@ -0,0 +1,19 @@ +// Copyright 2021 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 "chromeos/dbus/fwupd/fwupd_device.h" + +namespace chromeos { + +FwupdDevice::FwupdDevice() = default; + +FwupdDevice::FwupdDevice(const std::string& id, + const std::u16string& device_name) + : id(id), device_name(device_name) {} + +FwupdDevice::FwupdDevice(const FwupdDevice& other) = default; + +FwupdDevice::~FwupdDevice() = default; + +} // namespace chromeos diff --git a/chromeos/dbus/fwupd/fwupd_device.h b/chromeos/dbus/fwupd/fwupd_device.h new file mode 100644 index 00000000000000..9db3fc72aa4f59 --- /dev/null +++ b/chromeos/dbus/fwupd/fwupd_device.h @@ -0,0 +1,29 @@ +// Copyright 2021 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. + +#ifndef CHROMEOS_DBUS_FWUPD_FWUPD_DEVICE_H_ +#define CHROMEOS_DBUS_FWUPD_FWUPD_DEVICE_H_ + +#include + +#include "base/component_export.h" + +namespace chromeos { + +// Structure to hold FwupdDevice data received from fwupd. +struct COMPONENT_EXPORT(CHROMEOS_DBUS_FWUPD) FwupdDevice { + FwupdDevice(); + FwupdDevice(const std::string& id, const std::u16string& device_name); + FwupdDevice(const FwupdDevice& other); + ~FwupdDevice(); + + std::string id; + std::u16string device_name; +}; + +typedef std::vector FwupdDeviceList; + +} // namespace chromeos + +#endif // CHROMEOS_DBUS_FWUPD_FWUPD_DEVICE_H_