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) {
