buffet: Multiple listeners of add/remove new command. Replace CommandDispatchInterface with callbacks. Lists of callbacks in CommandQueue. BUG=brillo:697 TEST=FEATURE=test emerge-gizmo buffet Change-Id: I3c164c8c7c2cb098b896aa8d5c9a99d856b05172 Reviewed-on: https://chromium-review.googlesource.com/270350 Commit-Queue: Vitaly Buka <vitalybuka@chromium.org> Tested-by: Vitaly Buka <vitalybuka@chromium.org> Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
diff --git a/buffet/commands/command_dispatch_interface.h b/buffet/commands/command_dispatch_interface.h deleted file mode 100644 index d1a727d..0000000 --- a/buffet/commands/command_dispatch_interface.h +++ /dev/null
@@ -1,29 +0,0 @@ -// Copyright 2014 The Chromium OS 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 BUFFET_COMMANDS_COMMAND_DISPATCH_INTERFACE_H_ -#define BUFFET_COMMANDS_COMMAND_DISPATCH_INTERFACE_H_ - -#include <string> - -namespace buffet { - -class CommandInstance; - -// This is an abstract base interface that a command dispatcher will implement. -// It allows to abstract out the actual transport layer, such as D-Bus, from -// the rest of command registration and delivery subsystems. -class CommandDispachInterface { - public: - virtual ~CommandDispachInterface() = default; - // Callback invoked by CommandQueue when a new command is added to the queue. - virtual void OnCommandAdded(CommandInstance* command_instance) = 0; - // Callback invoked by CommandQueue when a new command is removed from - // the queue. - virtual void OnCommandRemoved(CommandInstance* command_instance) = 0; -}; - -} // namespace buffet - -#endif // BUFFET_COMMANDS_COMMAND_DISPATCH_INTERFACE_H_
diff --git a/buffet/commands/command_manager.cc b/buffet/commands/command_manager.cc index de2125b..cbaaeca 100644 --- a/buffet/commands/command_manager.cc +++ b/buffet/commands/command_manager.cc
@@ -16,18 +16,16 @@ namespace buffet { -CommandManager::CommandManager() { - command_queue_.SetCommandDispachInterface(&command_dispatcher_); +CommandManager::CommandManager() + : CommandManager(base::WeakPtr<ExportedObjectManager>{}) { } CommandManager::CommandManager( const base::WeakPtr<ExportedObjectManager>& object_manager) - : command_dispatcher_(object_manager->GetBus(), object_manager.get()) { - command_queue_.SetCommandDispachInterface(&command_dispatcher_); -} - -CommandManager::CommandManager(CommandDispachInterface* dispatch_interface) { - command_queue_.SetCommandDispachInterface(dispatch_interface); + : command_dispatcher_(object_manager) { + command_queue_.AddOnCommandAddedCallback( + base::Bind(&DBusCommandDispacher::OnCommandAdded, + base::Unretained(&command_dispatcher_))); } const CommandDictionary& CommandManager::GetCommandDictionary() const {
diff --git a/buffet/commands/command_manager.h b/buffet/commands/command_manager.h index b2b9eba..88ebca7 100644 --- a/buffet/commands/command_manager.h +++ b/buffet/commands/command_manager.h
@@ -38,8 +38,6 @@ explicit CommandManager( const base::WeakPtr<chromeos::dbus_utils::ExportedObjectManager>& object_manager); - // Special constructor to help mock out command dispatcher for testing. - explicit CommandManager(CommandDispachInterface* dispatch_interface); // Sets callback which is called when command definitions is changed. void AddOnCommandDefChanged(const base::Closure& callback) { @@ -103,8 +101,8 @@ private: CommandDictionary base_dictionary_; // Base/std command definitions/schemas. CommandDictionary dictionary_; // Command definitions/schemas. - CommandQueue command_queue_; DBusCommandDispacher command_dispatcher_; + CommandQueue command_queue_; std::vector<base::Callback<void()>> on_command_changed_; DISALLOW_COPY_AND_ASSIGN(CommandManager);
diff --git a/buffet/commands/command_queue.cc b/buffet/commands/command_queue.cc index 62d8c20..614261e 100644 --- a/buffet/commands/command_queue.cc +++ b/buffet/commands/command_queue.cc
@@ -4,16 +4,23 @@ #include "buffet/commands/command_queue.h" +#include <base/bind.h> #include <base/time/time.h> -#include "buffet/commands/command_dispatch_interface.h" - namespace buffet { namespace { const int kRemoveCommandDelayMin = 5; } +void CommandQueue::AddOnCommandAddedCallback(const Callback& callback) { + on_command_added_.push_back(callback); +} + +void CommandQueue::AddOnCommandRemovedCallback(const Callback& callback) { + on_command_removed_.push_back(callback); +} + void CommandQueue::Add(std::unique_ptr<CommandInstance> instance) { std::string id = instance->GetID(); LOG_IF(FATAL, id.empty()) << "Command has no ID"; @@ -21,8 +28,8 @@ auto pair = map_.insert(std::make_pair(id, std::move(instance))); LOG_IF(FATAL, !pair.second) << "Command with ID '" << id << "' is already in the queue"; - if (dispatch_interface_) - dispatch_interface_->OnCommandAdded(pair.first->second.get()); + for (const auto& cb : on_command_added_) + cb.Run(pair.first->second.get()); Cleanup(); } @@ -43,8 +50,8 @@ std::unique_ptr<CommandInstance> instance{std::move(p->second)}; instance->SetCommandQueue(nullptr); map_.erase(p); - if (dispatch_interface_) - dispatch_interface_->OnCommandRemoved(instance.get()); + for (const auto& cb : on_command_removed_) + cb.Run(instance.get()); return true; }
diff --git a/buffet/commands/command_queue.h b/buffet/commands/command_queue.h index 09339d9..9178c8a 100644 --- a/buffet/commands/command_queue.h +++ b/buffet/commands/command_queue.h
@@ -10,25 +10,25 @@ #include <queue> #include <string> #include <utility> +#include <vector> +#include <base/callback.h> #include <base/macros.h> #include "buffet/commands/command_instance.h" namespace buffet { -class CommandDispachInterface; - class CommandQueue final { public: + using Callback = base::Callback<void(CommandInstance*)>; CommandQueue() = default; - // Sets a command dispatch notifications for changes in command queue. - // |dispatch_interface| must outlive the CommandQueue object instance - // or be nullptr. - void SetCommandDispachInterface(CommandDispachInterface* dispatch_interface) { - dispatch_interface_ = dispatch_interface; - } + // Adds notifications callback for a new command is added to the queue. + void AddOnCommandAddedCallback(const Callback& callback); + + // Adds notifications callback for a command is removed from the queue. + void AddOnCommandRemovedCallback(const Callback& callback); // Checks if the command queue is empty. bool IsEmpty() const { return map_.empty(); } @@ -75,8 +75,9 @@ // Queue of commands to be removed. std::queue<std::pair<base::Time, std::string>> remove_queue_; - // Callback interface for command dispatch, if provided. - CommandDispachInterface* dispatch_interface_ = nullptr; + using CallbackList = std::vector<Callback>; + CallbackList on_command_added_; + CallbackList on_command_removed_; DISALLOW_COPY_AND_ASSIGN(CommandQueue); };
diff --git a/buffet/commands/command_queue_unittest.cc b/buffet/commands/command_queue_unittest.cc index 8829404..f29b03e 100644 --- a/buffet/commands/command_queue_unittest.cc +++ b/buffet/commands/command_queue_unittest.cc
@@ -8,11 +8,12 @@ #include <string> #include <vector> +#include <base/bind.h> +#include <base/memory/weak_ptr.h> #include <chromeos/strings/string_utils.h> #include <gtest/gtest.h> #include "buffet/commands/command_definition.h" -#include "buffet/commands/command_dispatch_interface.h" #include "buffet/commands/object_schema.h" namespace buffet { @@ -44,22 +45,30 @@ ObjectSchema::Create()}; }; -// Fake implementation of CommandDispachInterface. -// Just keeps track of commands being added to and removed from the queue_. +// Keeps track of commands being added to and removed from the queue_. // Aborts if duplicate commands are added or non-existent commands are removed. -class FakeDispatchInterface : public CommandDispachInterface { +class FakeDispatcher { public: - void OnCommandAdded(CommandInstance* command_instance) override { + explicit FakeDispatcher(CommandQueue* queue) { + queue->AddOnCommandAddedCallback( + base::Bind(&FakeDispatcher::OnCommandAdded, + weak_ptr_factory_.GetWeakPtr())); + queue->AddOnCommandRemovedCallback( + base::Bind(&FakeDispatcher::OnCommandRemoved, + weak_ptr_factory_.GetWeakPtr())); + } + + void OnCommandAdded(CommandInstance* command_instance) { CHECK(ids_.insert(command_instance->GetID()).second) << "Command ID already exists: " << command_instance->GetID(); CHECK(commands_.insert(command_instance).second) << "Command instance already exists"; } - void OnCommandRemoved(CommandInstance* command_instance) override { - CHECK_EQ(1, ids_.erase(command_instance->GetID())) + void OnCommandRemoved(CommandInstance* command_instance) { + CHECK_EQ(1u, ids_.erase(command_instance->GetID())) << "Command ID not found: " << command_instance->GetID(); - CHECK_EQ(1, commands_.erase(command_instance)) + CHECK_EQ(1u, commands_.erase(command_instance)) << "Command instance not found"; } @@ -73,6 +82,7 @@ private: std::set<std::string> ids_; std::set<CommandInstance*> commands_; + base::WeakPtrFactory<FakeDispatcher> weak_ptr_factory_{this}; }; TEST_F(CommandQueueTest, Empty) { @@ -123,8 +133,7 @@ } TEST_F(CommandQueueTest, Dispatch) { - FakeDispatchInterface dispatch; - queue_.SetCommandDispachInterface(&dispatch); + FakeDispatcher dispatch(&queue_); const std::string id1 = "id1"; const std::string id2 = "id2"; queue_.Add(CreateDummyCommandInstance("base.reboot", id1)); @@ -137,7 +146,6 @@ EXPECT_EQ(id2, dispatch.GetIDs()); Remove(id2); EXPECT_EQ("", dispatch.GetIDs()); - queue_.SetCommandDispachInterface(nullptr); } TEST_F(CommandQueueTest, Find) {
diff --git a/buffet/commands/dbus_command_dispatcher.cc b/buffet/commands/dbus_command_dispatcher.cc index 457ea88..2f5333c 100644 --- a/buffet/commands/dbus_command_dispatcher.cc +++ b/buffet/commands/dbus_command_dispatcher.cc
@@ -7,6 +7,7 @@ #include <chromeos/dbus/exported_object_manager.h> #include "buffet/commands/command_instance.h" +#include "buffet/commands/dbus_command_proxy.h" #include "buffet/dbus_constants.h" using chromeos::dbus_utils::AsyncEventSequencer; @@ -15,30 +16,18 @@ namespace buffet { DBusCommandDispacher::DBusCommandDispacher( - const scoped_refptr<dbus::Bus>& bus, - ExportedObjectManager* object_manager) - : bus_(bus), next_id_(0) { - if (object_manager) - object_manager_ = object_manager->AsWeakPtr(); + const base::WeakPtr<ExportedObjectManager>& object_manager) + : object_manager_{object_manager} { } void DBusCommandDispacher::OnCommandAdded(CommandInstance* command_instance) { - auto proxy = CreateDBusCommandProxy(command_instance); + if (!object_manager_) + return; + std::unique_ptr<DBusCommandProxy> proxy{new DBusCommandProxy( + object_manager_.get(), object_manager_->GetBus(), command_instance, + dbus_constants::kCommandServicePathPrefix + std::to_string(++next_id_))}; proxy->RegisterAsync(AsyncEventSequencer::GetDefaultCompletionAction()); command_instance->AddProxy(std::move(proxy)); } -void DBusCommandDispacher::OnCommandRemoved(CommandInstance* command_instance) { -} - -std::unique_ptr<DBusCommandProxy> DBusCommandDispacher::CreateDBusCommandProxy( - CommandInstance* command_instance) { - return std::unique_ptr<DBusCommandProxy>( - new DBusCommandProxy(object_manager_.get(), - bus_, - command_instance, - dbus_constants::kCommandServicePathPrefix + - std::to_string(++next_id_))); -} - } // namespace buffet
diff --git a/buffet/commands/dbus_command_dispatcher.h b/buffet/commands/dbus_command_dispatcher.h index 35d258b..ab63a6c 100644 --- a/buffet/commands/dbus_command_dispatcher.h +++ b/buffet/commands/dbus_command_dispatcher.h
@@ -10,10 +10,6 @@ #include <base/macros.h> #include <base/memory/weak_ptr.h> -#include <dbus/bus.h> - -#include "buffet/commands/command_dispatch_interface.h" -#include "buffet/commands/dbus_command_proxy.h" namespace chromeos { namespace dbus_utils { @@ -23,37 +19,29 @@ namespace buffet { -// Implements D-Bus dispatch of commands. When OnCommandAdded is called over -// CommandDispachInterface, DBusCommandDispacher creates an instance of -// DBusCommandProxy object and advertises it through ExportedObjectManager on -// D-Bus. Command handling processes can watch the new D-Bus object appear -// and communicate with it to update the command handling progress. -// Once command is handled, DBusCommandProxy::Done() is called and the command -// is removed from the command queue and D-Bus ExportedObjectManager. -class DBusCommandDispacher : public CommandDispachInterface { +class CommandInstance; + +// Implements D-Bus dispatch of commands. When OnCommandAdded is called, +// DBusCommandDispacher creates an instance of DBusCommandProxy object and +// advertises it through ExportedObjectManager on D-Bus. Command handling +// processes can watch the new D-Bus object appear and communicate with it to +// update the command handling progress. Once command is handled, +// DBusCommandProxy::Done() is called and the command is removed from the +// command queue and D-Bus ExportedObjectManager. +class DBusCommandDispacher { public: - DBusCommandDispacher( - const scoped_refptr<dbus::Bus>& bus, - chromeos::dbus_utils::ExportedObjectManager* object_manager = nullptr); - virtual ~DBusCommandDispacher() = default; + explicit DBusCommandDispacher(const base::WeakPtr< + chromeos::dbus_utils::ExportedObjectManager>& object_manager); - // CommandDispachInterface overrides. Called by CommandQueue. - void OnCommandAdded(CommandInstance* command_instance) override; - void OnCommandRemoved(CommandInstance* command_instance) override; - - protected: - virtual std::unique_ptr<DBusCommandProxy> CreateDBusCommandProxy( - CommandInstance* command_instance); + void OnCommandAdded(CommandInstance* command_instance); private: - scoped_refptr<dbus::Bus> bus_; base::WeakPtr<chromeos::dbus_utils::ExportedObjectManager> object_manager_; - int next_id_; + int next_id_{0}; // Default constructor is used in special circumstances such as for testing. DBusCommandDispacher() = default; - friend class DBusCommandDispacherTest; friend class CommandManager; DISALLOW_COPY_AND_ASSIGN(DBusCommandDispacher); };
diff --git a/buffet/commands/dbus_command_dispatcher_unittest.cc b/buffet/commands/dbus_command_dispatcher_unittest.cc index 2391bb1..d9d91bc 100644 --- a/buffet/commands/dbus_command_dispatcher_unittest.cc +++ b/buffet/commands/dbus_command_dispatcher_unittest.cc
@@ -16,6 +16,7 @@ #include "buffet/commands/command_dictionary.h" #include "buffet/commands/command_queue.h" +#include "buffet/commands/dbus_command_proxy.h" #include "buffet/commands/unittest_utils.h" #include "buffet/dbus_constants.h" @@ -60,9 +61,10 @@ om_.reset(new chromeos::dbus_utils::ExportedObjectManager( bus_.get(), kExportedObjectManagerPath)); om_->RegisterAsync(AsyncEventSequencer::GetDefaultCompletionAction()); - command_dispatcher_.reset( - new DBusCommandDispacher(om_->GetBus(), om_.get())); - command_queue_.SetCommandDispachInterface(command_dispatcher_.get()); + command_dispatcher_.reset(new DBusCommandDispacher(om_->AsWeakPtr())); + command_queue_.AddOnCommandAddedCallback( + base::Bind(&DBusCommandDispacher::OnCommandAdded, + base::Unretained(command_dispatcher_.get()))); // Use a mock exported object for command proxy. mock_exported_command_proxy_ = new dbus::MockExportedObject( bus_.get(), kCmdObjPath); @@ -133,8 +135,8 @@ scoped_refptr<dbus::MockExportedObject> mock_exported_command_proxy_; std::unique_ptr<chromeos::dbus_utils::ExportedObjectManager> om_; CommandDictionary dictionary_; - CommandQueue command_queue_; std::unique_ptr<DBusCommandDispacher> command_dispatcher_; + CommandQueue command_queue_; }; TEST_F(DBusCommandDispacherTest, Test_Command_Base_Shutdown) {
diff --git a/buffet/device_registration_info_unittest.cc b/buffet/device_registration_info_unittest.cc index 1800da1..ec68b21 100644 --- a/buffet/device_registration_info_unittest.cc +++ b/buffet/device_registration_info_unittest.cc
@@ -188,7 +188,7 @@ storage_ = std::make_shared<MemStorage>(); storage_->Save(&data_); transport_ = std::make_shared<chromeos::http::fake::Transport>(); - command_manager_ = std::make_shared<CommandManager>(nullptr); + command_manager_ = std::make_shared<CommandManager>(); state_manager_ = std::make_shared<StateManager>(&mock_state_change_queue_); chromeos::KeyValueStore config_store; config_store.SetString("client_id", test_data::kClientId);