buffet: Separate command IDs from DBus paths.
As we're starting to support cloud and local cases, we need proper
support for cloud command IDs. Therefore now we require each
command instance to have some command id, but generate dbus path
independently.
BUG=None
TEST=cros_workon_make buffet --test&&manual
Change-Id: I83ae92872d920c8e0d6e15324ad11feebfd1f540
Reviewed-on: https://chromium-review.googlesource.com/226532
Tested-by: Anton Muhin <antonm@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Anton Muhin <antonm@chromium.org>
diff --git a/buffet/commands/command_instance.cc b/buffet/commands/command_instance.cc
index f403630..0caee20 100644
--- a/buffet/commands/command_instance.cc
+++ b/buffet/commands/command_instance.cc
@@ -128,6 +128,13 @@
instance.reset(new CommandInstance(command_name,
command_def->GetCategory(),
parameters));
+ // TODO(antonm): Move command_id to ctor and remove setter.
+ std::string command_id;
+ if (json->GetStringWithoutPathExpansion(commands::attributes::kCommand_Id,
+ &command_id)) {
+ instance->SetID(command_id);
+ }
+
return instance;
}
diff --git a/buffet/commands/command_manager.cc b/buffet/commands/command_manager.cc
index 7a1701a..1676cdc 100644
--- a/buffet/commands/command_manager.cc
+++ b/buffet/commands/command_manager.cc
@@ -82,9 +82,13 @@
}
}
-std::string CommandManager::AddCommand(
+void CommandManager::AddCommand(
std::unique_ptr<CommandInstance> command_instance) {
- return command_queue_.Add(std::move(command_instance));
+ command_queue_.Add(std::move(command_instance));
+}
+
+CommandInstance* CommandManager::FindCommand(const std::string& id) const {
+ return command_queue_.Find(id);
}
} // namespace buffet
diff --git a/buffet/commands/command_manager.h b/buffet/commands/command_manager.h
index 05326b3..88b551a 100644
--- a/buffet/commands/command_manager.h
+++ b/buffet/commands/command_manager.h
@@ -75,8 +75,13 @@
// the current device.
void Startup();
- // Adds a new command to the command queue. Returns command ID.
- std::string AddCommand(std::unique_ptr<CommandInstance> command_instance);
+ // Adds a new command to the command queue.
+ void AddCommand(std::unique_ptr<CommandInstance> command_instance);
+
+ // Finds a command by the command |id|. Returns nullptr if the command with
+ // the given |id| is not found. The returned pointer should not be persisted
+ // for a long period of time.
+ CommandInstance* FindCommand(const std::string& id) const;
private:
CommandDictionary base_dictionary_; // Base/std command definitions/schemas.
diff --git a/buffet/commands/command_queue.cc b/buffet/commands/command_queue.cc
index 8583921..d16d15f 100644
--- a/buffet/commands/command_queue.cc
+++ b/buffet/commands/command_queue.cc
@@ -7,17 +7,15 @@
namespace buffet {
-std::string CommandQueue::Add(std::unique_ptr<CommandInstance> instance) {
- std::string id = std::to_string(++next_id_);
- instance->SetID(id);
+void CommandQueue::Add(std::unique_ptr<CommandInstance> instance) {
+ std::string id = instance->GetID();
+ LOG_IF(FATAL, id.empty()) << "Command has no ID";
instance->SetCommandQueue(this);
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());
-
- return id;
}
std::unique_ptr<CommandInstance> CommandQueue::Remove(
diff --git a/buffet/commands/command_queue.h b/buffet/commands/command_queue.h
index 6d81b34..6d6547d 100644
--- a/buffet/commands/command_queue.h
+++ b/buffet/commands/command_queue.h
@@ -35,11 +35,9 @@
size_t GetCount() const { return map_.size(); }
// Adds a new command to the queue. Each command in the queue has a unique
- // ID that identifies that command instance in this queue. This identifier
- // has no relation to any GCD command identifiers or anything else. Just a
- // unique key in this queue class.
- // The ID string of the added command is returned by this method.
- std::string Add(std::unique_ptr<CommandInstance> instance);
+ // ID that identifies that command instance in this queue.
+ // One shouldn't attempt to add a command with the same ID.
+ void Add(std::unique_ptr<CommandInstance> instance);
// Removes a command identified by |id| from the queue. Returns a unique
// pointer to the command instance if removed successfully, or an empty
@@ -54,8 +52,6 @@
private:
// ID-to-CommandInstance map.
std::map<std::string, std::unique_ptr<CommandInstance>> map_;
- // Counter for generating unique command IDs.
- int next_id_ = 0;
// Callback interface for command dispatch, if provided.
CommandDispachInterface* dispatch_interface_ = nullptr;
diff --git a/buffet/commands/command_queue_unittest.cc b/buffet/commands/command_queue_unittest.cc
index 36c760e..8fe0c9d 100644
--- a/buffet/commands/command_queue_unittest.cc
+++ b/buffet/commands/command_queue_unittest.cc
@@ -15,9 +15,11 @@
namespace {
std::unique_ptr<buffet::CommandInstance> CreateDummyCommandInstance(
- const std::string& name = "base.reboot") {
- return std::unique_ptr<buffet::CommandInstance>(
+ const std::string& name, const std::string& id) {
+ auto cmd = std::unique_ptr<buffet::CommandInstance>(
new buffet::CommandInstance(name, "powerd", {}));
+ cmd->SetID(id);
+ return cmd;
}
// Fake implementation of CommandDispachInterface.
@@ -61,19 +63,19 @@
TEST(CommandQueue, Add) {
buffet::CommandQueue queue;
- std::set<std::string> ids; // Using set<> to check that IDs are unique.
- ids.insert(queue.Add(CreateDummyCommandInstance()));
- ids.insert(queue.Add(CreateDummyCommandInstance()));
- ids.insert(queue.Add(CreateDummyCommandInstance()));
+ queue.Add(CreateDummyCommandInstance("base.reboot", "id1"));
+ queue.Add(CreateDummyCommandInstance("base.reboot", "id2"));
+ queue.Add(CreateDummyCommandInstance("base.reboot", "id3"));
EXPECT_EQ(3, queue.GetCount());
- EXPECT_EQ(3, ids.size());
EXPECT_FALSE(queue.IsEmpty());
}
TEST(CommandQueue, Remove) {
buffet::CommandQueue queue;
- std::string id1 = queue.Add(CreateDummyCommandInstance());
- std::string id2 = queue.Add(CreateDummyCommandInstance());
+ const std::string id1 = "id1";
+ const std::string id2 = "id2";
+ queue.Add(CreateDummyCommandInstance("base.reboot", id1));
+ queue.Add(CreateDummyCommandInstance("base.reboot", id2));
EXPECT_FALSE(queue.IsEmpty());
EXPECT_EQ(nullptr, queue.Remove("dummy").get());
EXPECT_EQ(2, queue.GetCount());
@@ -92,8 +94,10 @@
FakeDispatchInterface dispatch;
buffet::CommandQueue queue;
queue.SetCommandDispachInterface(&dispatch);
- std::string id1 = queue.Add(CreateDummyCommandInstance());
- std::string id2 = queue.Add(CreateDummyCommandInstance());
+ const std::string id1 = "id1";
+ const std::string id2 = "id2";
+ queue.Add(CreateDummyCommandInstance("base.reboot", id1));
+ queue.Add(CreateDummyCommandInstance("base.reboot", id2));
std::set<std::string> ids{id1, id2}; // Make sure they are sorted properly.
std::string expected_set = chromeos::string_utils::Join(
',', std::vector<std::string>(ids.begin(), ids.end()));
@@ -106,8 +110,10 @@
TEST(CommandQueue, Find) {
buffet::CommandQueue queue;
- std::string id1 = queue.Add(CreateDummyCommandInstance("base.reboot"));
- std::string id2 = queue.Add(CreateDummyCommandInstance("base.shutdown"));
+ const std::string id1 = "id1";
+ const std::string id2 = "id2";
+ queue.Add(CreateDummyCommandInstance("base.reboot", id1));
+ queue.Add(CreateDummyCommandInstance("base.shutdown", id2));
EXPECT_EQ(nullptr, queue.Find("dummy"));
auto cmd1 = queue.Find(id1);
EXPECT_NE(nullptr, cmd1);
diff --git a/buffet/commands/dbus_command_dispatcher.cc b/buffet/commands/dbus_command_dispatcher.cc
index 591c1cc..005354a 100644
--- a/buffet/commands/dbus_command_dispatcher.cc
+++ b/buffet/commands/dbus_command_dispatcher.cc
@@ -16,7 +16,7 @@
DBusCommandDispacher::DBusCommandDispacher(
const scoped_refptr<dbus::Bus>& bus,
ExportedObjectManager* object_manager)
- : bus_(bus) {
+ : bus_(bus), next_id_(0) {
if (object_manager)
object_manager_ = object_manager->AsWeakPtr();
}
@@ -44,9 +44,13 @@
}
std::unique_ptr<DBusCommandProxy> DBusCommandDispacher::CreateDBusCommandProxy(
- CommandInstance* command_instance) const {
+ CommandInstance* command_instance) {
return std::unique_ptr<DBusCommandProxy>(
- new DBusCommandProxy(object_manager_.get(), bus_, command_instance));
+ 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 94f938b..0bfc21f 100644
--- a/buffet/commands/dbus_command_dispatcher.h
+++ b/buffet/commands/dbus_command_dispatcher.h
@@ -47,11 +47,12 @@
protected:
virtual std::unique_ptr<DBusCommandProxy> CreateDBusCommandProxy(
- CommandInstance* command_instance) const;
+ CommandInstance* command_instance);
private:
scoped_refptr<dbus::Bus> bus_;
base::WeakPtr<chromeos::dbus_utils::ExportedObjectManager> object_manager_;
+ int next_id_;
// This is the map that tracks relationship between CommandInstance and
// corresponding DBusCommandProxy objects.
std::map<CommandInstance*, std::unique_ptr<DBusCommandProxy>> command_map_;
diff --git a/buffet/commands/dbus_command_dispatcher_unittest.cc b/buffet/commands/dbus_command_dispatcher_unittest.cc
index 9de6889..0cc1142 100644
--- a/buffet/commands/dbus_command_dispatcher_unittest.cc
+++ b/buffet/commands/dbus_command_dispatcher_unittest.cc
@@ -98,13 +98,13 @@
}
- std::string AddNewCommand(const std::string& json) {
+ void AddNewCommand(const std::string& json, const std::string& id) {
auto command_instance = CommandInstance::FromJson(
CreateDictionaryValue(json.c_str()).get(), dictionary_, nullptr);
+ command_instance->SetID(id);
// Two interfaces are added - Command and Properties.
EXPECT_CALL(*mock_exported_object_manager_, SendSignal(_)).Times(2);
- return command_instance ?
- command_queue_.Add(std::move(command_instance)) : std::string();
+ command_queue_.Add(std::move(command_instance));
}
void FinishCommand(DBusCommandProxy* proxy) {
@@ -126,8 +126,8 @@
};
TEST_F(DBusCommandDispacherTest, Test_Command_Base_Shutdown) {
- std::string id = AddNewCommand("{'name':'base.shutdown'}");
- EXPECT_EQ("1", id);
+ const std::string id = "id0000";
+ AddNewCommand("{'name':'base.shutdown'}", id);
CommandInstance* command_instance = command_queue_.Find(id);
ASSERT_NE(nullptr, command_instance);
DBusCommandProxy* command_proxy =
@@ -157,13 +157,13 @@
}
TEST_F(DBusCommandDispacherTest, Test_Command_Base_Reboot) {
- std::string id = AddNewCommand(R"({
+ const std::string id = "id0001";
+ AddNewCommand(R"({
'name': 'base.reboot',
'parameters': {
'delay': 20
}
- })");
- EXPECT_EQ("1", id);
+ })", id);
CommandInstance* command_instance = command_queue_.Find(id);
ASSERT_NE(nullptr, command_instance);
DBusCommandProxy* command_proxy =
diff --git a/buffet/commands/dbus_command_proxy.cc b/buffet/commands/dbus_command_proxy.cc
index 07ead32..4ef94a8 100644
--- a/buffet/commands/dbus_command_proxy.cc
+++ b/buffet/commands/dbus_command_proxy.cc
@@ -10,7 +10,6 @@
#include "buffet/commands/command_instance.h"
#include "buffet/commands/prop_constraints.h"
#include "buffet/commands/prop_types.h"
-#include "buffet/libbuffet/dbus_constants.h"
using chromeos::dbus_utils::AsyncEventSequencer;
using chromeos::dbus_utils::ExportedObjectManager;
@@ -19,9 +18,9 @@
DBusCommandProxy::DBusCommandProxy(ExportedObjectManager* object_manager,
const scoped_refptr<dbus::Bus>& bus,
- CommandInstance* command_instance)
- : object_path_(dbus_constants::kCommandServicePathPrefix +
- command_instance->GetID()),
+ CommandInstance* command_instance,
+ std::string object_path)
+ : object_path_(std::move(object_path)),
command_instance_(command_instance),
dbus_object_(object_manager, bus, object_path_) {
}
diff --git a/buffet/commands/dbus_command_proxy.h b/buffet/commands/dbus_command_proxy.h
index 50878fb..7286a4d 100644
--- a/buffet/commands/dbus_command_proxy.h
+++ b/buffet/commands/dbus_command_proxy.h
@@ -13,6 +13,7 @@
#include <chromeos/dbus/dbus_object.h>
#include "buffet/commands/command_proxy_interface.h"
+#include "buffet/libbuffet/dbus_constants.h"
namespace chromeos {
namespace dbus_utils {
@@ -28,7 +29,8 @@
public:
DBusCommandProxy(chromeos::dbus_utils::ExportedObjectManager* object_manager,
const scoped_refptr<dbus::Bus>& bus,
- CommandInstance* command_instance);
+ CommandInstance* command_instance,
+ std::string object_path);
~DBusCommandProxy() override = default;
void RegisterAsync(
diff --git a/buffet/commands/dbus_command_proxy_unittest.cc b/buffet/commands/dbus_command_proxy_unittest.cc
index ae91b13..ab4e36c 100644
--- a/buffet/commands/dbus_command_proxy_unittest.cc
+++ b/buffet/commands/dbus_command_proxy_unittest.cc
@@ -92,7 +92,8 @@
ExportMethod(_, _, _, _)).Times(AnyNumber());
command_proxy_.reset(new DBusCommandProxy(nullptr, bus_,
- command_instance_.get()));
+ command_instance_.get(),
+ cmd_path));
command_instance_->AddProxy(command_proxy_.get());
command_proxy_->RegisterAsync(
AsyncEventSequencer::GetDefaultCompletionAction());
diff --git a/buffet/commands/schema_constants.cc b/buffet/commands/schema_constants.cc
index 74f96af..faf58fb 100644
--- a/buffet/commands/schema_constants.cc
+++ b/buffet/commands/schema_constants.cc
@@ -43,6 +43,7 @@
const char kObject_Properties[] = "properties";
+const char kCommand_Id[] = "id";
const char kCommand_Name[] = "name";
const char kCommand_Parameters[] = "parameters";
} // namespace attributes
diff --git a/buffet/commands/schema_constants.h b/buffet/commands/schema_constants.h
index 2375ada..d5a8c4e 100644
--- a/buffet/commands/schema_constants.h
+++ b/buffet/commands/schema_constants.h
@@ -47,6 +47,7 @@
extern const char kObject_Properties[];
+extern const char kCommand_Id[];
extern const char kCommand_Name[];
extern const char kCommand_Parameters[];
} // namespace attributes
diff --git a/buffet/device_registration_info.cc b/buffet/device_registration_info.cc
index 30c62a9..b925033 100644
--- a/buffet/device_registration_info.cc
+++ b/buffet/device_registration_info.cc
@@ -140,8 +140,11 @@
return chromeos::url::AppendQueryParams(result, params);
}
-auto ignore_cloud_error = base::Bind([](const chromeos::Error&){});
-auto ignore_cloud_result = base::Bind([](const base::DictionaryValue&){});
+void IgnoreCloudError(const chromeos::Error&) {
+}
+
+void IgnoreCloudResult(const base::DictionaryValue&) {
+}
} // anonymous namespace
@@ -662,7 +665,7 @@
}),
// TODO(antonm): Failure to update device resource probably deserves
// some additional actions.
- ignore_cloud_error);
+ base::Bind(&IgnoreCloudError));
}
void DeviceRegistrationInfo::FetchCommands(
@@ -679,7 +682,7 @@
const base::ListValue empty;
callback.Run(commands ? *commands : empty);
}),
- ignore_cloud_error);
+ base::Bind(&IgnoreCloudError));
}
void DeviceRegistrationInfo::AbortLimboCommands(
@@ -714,7 +717,7 @@
chromeos::http::request_type::kPut,
GetServiceURL("commands/" + command_id),
command_copy.get(),
- ignore_cloud_result, ignore_cloud_error);
+ base::Bind(&IgnoreCloudResult), base::Bind(&IgnoreCloudError));
}
base::MessageLoop::current()->PostTask(FROM_HERE, callback);
@@ -751,9 +754,9 @@
continue;
}
- // TODO(antonm): Double check if there is a chance to publish
- // the same command twice if it doesn't change its status.
- command_manager_->AddCommand(std::move(command_instance));
+ // TODO(antonm): Properly process cancellation of commands.
+ if (!command_manager_->FindCommand(command_instance->GetID()))
+ command_manager_->AddCommand(std::move(command_instance));
}
}
diff --git a/buffet/manager.cc b/buffet/manager.cc
index 7deb36f..9c1cd94 100644
--- a/buffet/manager.cc
+++ b/buffet/manager.cc
@@ -167,6 +167,7 @@
void Manager::HandleAddCommand(scoped_ptr<DBusMethodResponse> response,
const std::string& json_command) {
+ static int next_id = 0;
std::string error_message;
std::unique_ptr<base::Value> value(base::JSONReader::ReadAndReturnError(
json_command, base::JSON_PARSE_RFC, nullptr, &error_message));
@@ -183,6 +184,7 @@
response->ReplyWithError(error.get());
return;
}
+ command_instance->SetID(std::to_string(++next_id));
command_manager_->AddCommand(std::move(command_instance));
response->Return();
}
