buffet: Move command state into CommandInstance
The command state is now stored in CommandInstance and not in
DBusCommandProxy. CommandInstance can now notify the proxy of
command state changes via CommandProxyInterface.
Moved command status strings from dbus_constants.h into the
CommandInstance class, as members.
Added a property on DBusCommandProxy to expose the command
parameters to command handlers, so they can get the parameter
values over D-Bus.
BUG=chromium:374864
TEST=FEATURES=test emerge-link buffet
Change-Id: Ief3397ef09644772ffc3b1b01ed972a8b6779df4
Reviewed-on: https://chromium-review.googlesource.com/216296
Tested-by: Alex Vakulenko <avakulenko@chromium.org>
Reviewed-by: Christopher Wiley <wiley@chromium.org>
Commit-Queue: Alex Vakulenko <avakulenko@chromium.org>
diff --git a/buffet/commands/command_instance.cc b/buffet/commands/command_instance.cc
index d217574..9e1211a 100644
--- a/buffet/commands/command_instance.cc
+++ b/buffet/commands/command_instance.cc
@@ -10,11 +10,22 @@
#include "buffet/commands/command_definition.h"
#include "buffet/commands/command_dictionary.h"
+#include "buffet/commands/command_proxy_interface.h"
+#include "buffet/commands/command_queue.h"
#include "buffet/commands/schema_constants.h"
#include "buffet/commands/schema_utils.h"
namespace buffet {
+const char CommandInstance::kStatusQueued[] = "queued";
+const char CommandInstance::kStatusInProgress[] = "inProgress";
+const char CommandInstance::kStatusPaused[] = "paused";
+const char CommandInstance::kStatusError[] = "error";
+const char CommandInstance::kStatusDone[] = "done";
+const char CommandInstance::kStatusCanceled[] = "canceled";
+const char CommandInstance::kStatusAborted[] = "aborted";
+const char CommandInstance::kStatusExpired[] = "expired";
+
CommandInstance::CommandInstance(const std::string& name,
const std::string& category,
const native_types::Object& parameters)
@@ -120,5 +131,53 @@
return instance;
}
+bool CommandInstance::SetProgress(int progress) {
+ if (progress < 0 || progress > 100)
+ return false;
+ if (progress != progress_) {
+ progress_ = progress;
+ SetStatus(kStatusInProgress);
+ if (proxy_)
+ proxy_->OnProgressChanged(progress_);
+ }
+ return true;
+}
+
+void CommandInstance::Abort() {
+ SetStatus(kStatusAborted);
+ RemoveFromQueue();
+ // The command will be destroyed after that, so do not access any members.
+}
+
+void CommandInstance::Cancel() {
+ SetStatus(kStatusCanceled);
+ RemoveFromQueue();
+ // The command will be destroyed after that, so do not access any members.
+}
+
+void CommandInstance::Done() {
+ SetProgress(100);
+ SetStatus(kStatusDone);
+ RemoveFromQueue();
+ // The command will be destroyed after that, so do not access any members.
+}
+
+void CommandInstance::SetStatus(const std::string& status) {
+ if (status != status_) {
+ status_ = status;
+ if (proxy_)
+ proxy_->OnStatusChanged(status_);
+ }
+}
+
+void CommandInstance::RemoveFromQueue() {
+ if (queue_) {
+ // Store this instance in unique_ptr until the end of this method,
+ // otherwise it will be destroyed as soon as CommandQueue::Remove() returns.
+ std::unique_ptr<CommandInstance> this_instance = queue_->Remove(GetID());
+ // The command instance will survive till the end of this scope.
+ }
+}
+
} // namespace buffet
diff --git a/buffet/commands/command_instance.h b/buffet/commands/command_instance.h
index 6e482ad..4b71149 100644
--- a/buffet/commands/command_instance.h
+++ b/buffet/commands/command_instance.h
@@ -22,6 +22,8 @@
namespace buffet {
class CommandDictionary;
+class CommandProxyInterface;
+class CommandQueue;
class CommandInstance final {
public:
@@ -56,8 +58,45 @@
// Sets the command ID (normally done by CommandQueue when the command
// instance is added to it).
void SetID(const std::string& id) { id_ = id; }
+ // Sets the command proxy for the dispatch technology used.
+ // The proxy object is not owned by this class.
+ void SetProxy(CommandProxyInterface* proxy) { proxy_ = proxy; }
+ // Sets the pointer to queue this command is part of.
+ void SetCommandQueue(CommandQueue* queue) { queue_ = queue; }
+
+ // Updates the command execution progress. The |progress| must be between
+ // 0 and 100. Returns false if the progress value is incorrect.
+ bool SetProgress(int progress);
+ // Aborts command execution.
+ void Abort();
+ // Cancels command execution.
+ void Cancel();
+ // Marks the command as completed successfully.
+ void Done();
+
+ // Command state getters.
+ int GetProgress() const { return progress_; }
+ const std::string& GetStatus() const { return status_; }
+
+ // Values for command execution status.
+ static const char kStatusQueued[];
+ static const char kStatusInProgress[];
+ static const char kStatusPaused[];
+ static const char kStatusError[];
+ static const char kStatusDone[];
+ static const char kStatusCanceled[];
+ static const char kStatusAborted[];
+ static const char kStatusExpired[];
private:
+ // Helper function to update the command status.
+ // Used by Abort(), Cancel(), Done() methods.
+ void SetStatus(const std::string& status);
+ // Helper method that removes this command from the command queue.
+ // Note that since the command queue owns the lifetime of the command instance
+ // object, removing a command from the queue will also destroy it.
+ void RemoveFromQueue();
+
// Unique command ID within a command queue.
std::string id_;
// Full command name as "<package_name>.<command_name>".
@@ -68,6 +107,17 @@
std::string category_;
// Command parameters and their values.
native_types::Object parameters_;
+ // Current command status.
+ std::string status_ = kStatusQueued;
+ // Current command execution progress.
+ int progress_ = 0;
+ // Command proxy class for the current dispatch technology (e.g. D-Bus).
+ // This is a weak pointer. The proxy's lifetime is managed by the command
+ // dispatcher.
+ CommandProxyInterface* proxy_ = nullptr;
+ // Pointer to the command queue this command instance is added to.
+ // The queue owns the command instance, so it outlives this object.
+ CommandQueue* queue_ = nullptr;
DISALLOW_COPY_AND_ASSIGN(CommandInstance);
};
diff --git a/buffet/commands/command_proxy_interface.h b/buffet/commands/command_proxy_interface.h
new file mode 100644
index 0000000..f9e78d6
--- /dev/null
+++ b/buffet/commands/command_proxy_interface.h
@@ -0,0 +1,25 @@
+// 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_PROXY_INTERFACE_H_
+#define BUFFET_COMMANDS_COMMAND_PROXY_INTERFACE_H_
+
+#include <string>
+
+namespace buffet {
+
+// This interface lets the command instance to update its proxy of command
+// state changes, so that the proxy can then notify clients of the changes over
+// their supported protocol (e.g. D-Bus).
+class CommandProxyInterface {
+ public:
+ virtual ~CommandProxyInterface() = default;
+
+ virtual void OnStatusChanged(const std::string& status) = 0;
+ virtual void OnProgressChanged(int progress) = 0;
+};
+
+} // namespace buffet
+
+#endif // BUFFET_COMMANDS_COMMAND_PROXY_INTERFACE_H_
diff --git a/buffet/commands/command_queue.cc b/buffet/commands/command_queue.cc
index 557a303..8583921 100644
--- a/buffet/commands/command_queue.cc
+++ b/buffet/commands/command_queue.cc
@@ -10,6 +10,7 @@
std::string CommandQueue::Add(std::unique_ptr<CommandInstance> instance) {
std::string id = std::to_string(++next_id_);
instance->SetID(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";
@@ -25,6 +26,7 @@
auto p = map_.find(id);
if (p != map_.end()) {
instance = std::move(p->second);
+ instance->SetCommandQueue(nullptr);
map_.erase(p);
if (dispatch_interface_)
dispatch_interface_->OnCommandRemoved(instance.get());
diff --git a/buffet/commands/dbus_command_proxy.cc b/buffet/commands/dbus_command_proxy.cc
index 56bc706..1003ce7 100644
--- a/buffet/commands/dbus_command_proxy.cc
+++ b/buffet/commands/dbus_command_proxy.cc
@@ -51,18 +51,35 @@
itf->AddProperty(dbus_constants::kCommandId, &id_);
itf->AddProperty(dbus_constants::kCommandStatus, &status_);
itf->AddProperty(dbus_constants::kCommandProgress, &progress_);
+ itf->AddProperty(dbus_constants::kCommandParameters, ¶meters_);
// Set the initial property values before registering the DBus object.
name_.SetValue(command_instance_->GetName());
category_.SetValue(command_instance_->GetCategory());
id_.SetValue(command_instance_->GetID());
- status_.SetValue(dbus_constants::kCommandStatusQueued);
- progress_.SetValue(0);
+ status_.SetValue(command_instance_->GetStatus());
+ progress_.SetValue(command_instance_->GetProgress());
+ // Convert a string-to-PropValue map into a string-to-Any map which can be
+ // sent over D-Bus.
+ chromeos::dbus_utils::Dictionary params;
+ for (const auto& param_pair : command_instance_->GetParameters()) {
+ params.insert(std::make_pair(param_pair.first,
+ param_pair.second->GetValueAsAny()));
+ }
+ parameters_.SetValue(params);
// Register the command DBus object and expose its methods and properties.
dbus_object_.RegisterAsync(completion_callback);
}
+void DBusCommandProxy::OnStatusChanged(const std::string& status) {
+ status_.SetValue(status);
+}
+
+void DBusCommandProxy::OnProgressChanged(int progress) {
+ progress_.SetValue(progress);
+}
+
void DBusCommandProxy::HandleSetProgress(chromeos::ErrorPtr* error,
int32_t progress) {
LOG(INFO) << "Received call to Command<"
@@ -73,28 +90,26 @@
IntPropType progress_type;
progress_type.AddMinMaxConstraint(0, 100);
if (progress_type.ValidateValue(progress, error)) {
- status_.SetValue(dbus_constants::kCommandStatusInProgress);
- progress_.SetValue(progress);
+ command_instance_->SetProgress(progress);
}
}
void DBusCommandProxy::HandleAbort(chromeos::ErrorPtr* error) {
LOG(INFO) << "Received call to Command<"
<< command_instance_->GetName() << ">::Abort()";
- status_.SetValue(dbus_constants::kCommandStatusAborted);
+ command_instance_->Abort();
}
void DBusCommandProxy::HandleCancel(chromeos::ErrorPtr* error) {
LOG(INFO) << "Received call to Command<"
<< command_instance_->GetName() << ">::Cancel()";
- status_.SetValue(dbus_constants::kCommandStatusCanceled);
+ command_instance_->Cancel();
}
void DBusCommandProxy::HandleDone(chromeos::ErrorPtr* error) {
LOG(INFO) << "Received call to Command<"
<< command_instance_->GetName() << ">::Done()";
- status_.SetValue(dbus_constants::kCommandStatusDone);
- progress_.SetValue(100);
+ command_instance_->Done();
}
diff --git a/buffet/commands/dbus_command_proxy.h b/buffet/commands/dbus_command_proxy.h
index a4b2a53..1221099 100644
--- a/buffet/commands/dbus_command_proxy.h
+++ b/buffet/commands/dbus_command_proxy.h
@@ -9,8 +9,11 @@
#include <string>
#include <base/basictypes.h>
+#include <chromeos/dbus/data_serialization.h>
#include <chromeos/dbus/dbus_object.h>
+#include "buffet/commands/command_proxy_interface.h"
+
namespace chromeos {
namespace dbus_utils {
class ExportedObjectManager;
@@ -21,17 +24,21 @@
class CommandInstance;
-class DBusCommandProxy {
+class DBusCommandProxy : public CommandProxyInterface {
public:
DBusCommandProxy(chromeos::dbus_utils::ExportedObjectManager* object_manager,
const scoped_refptr<dbus::Bus>& bus,
CommandInstance* command_instance);
- virtual ~DBusCommandProxy() = default;
+ ~DBusCommandProxy() override = default;
void RegisterAsync(
const chromeos::dbus_utils::AsyncEventSequencer::CompletionAction&
completion_callback);
+ // CommandProxyInterface implementation/overloads.
+ void OnStatusChanged(const std::string& status) override;
+ void OnProgressChanged(int progress) override;
+
private:
// DBus properties for org.chromium.Buffet.Command interface.
chromeos::dbus_utils::ExportedProperty<std::string> name_;
@@ -39,6 +46,8 @@
chromeos::dbus_utils::ExportedProperty<std::string> id_;
chromeos::dbus_utils::ExportedProperty<std::string> status_;
chromeos::dbus_utils::ExportedProperty<int32_t> progress_;
+ chromeos::dbus_utils::ExportedProperty<chromeos::dbus_utils::Dictionary>
+ parameters_;
// Handles calls to org.chromium.Buffet.Command.SetProgress(progress).
void HandleSetProgress(chromeos::ErrorPtr* error, int32_t progress);
@@ -55,6 +64,7 @@
chromeos::dbus_utils::DBusObject dbus_object_;
friend class DBusCommandProxyTest;
+ friend class DBusCommandDispacherTest;
DISALLOW_COPY_AND_ASSIGN(DBusCommandProxy);
};
diff --git a/buffet/commands/dbus_command_proxy_unittest.cc b/buffet/commands/dbus_command_proxy_unittest.cc
index 8dd11a6..45a872a 100644
--- a/buffet/commands/dbus_command_proxy_unittest.cc
+++ b/buffet/commands/dbus_command_proxy_unittest.cc
@@ -22,8 +22,10 @@
using ::testing::Invoke;
using ::testing::_;
-using chromeos::dbus_utils::ExportedObjectManager;
using buffet::unittests::CreateDictionaryValue;
+using chromeos::dbus_utils::AsyncEventSequencer;
+using chromeos::dbus_utils::Dictionary;
+using chromeos::dbus_utils::ExportedObjectManager;
namespace buffet {
@@ -32,8 +34,6 @@
const char kTestCommandCategoty[] = "test_command_category";
const char kTestCommandId[] = "cmd_1";
-void NoAction(bool all_succeeded) {}
-
} // namespace
class DBusCommandProxyTest : public ::testing::Test {
@@ -92,11 +92,14 @@
command_proxy_.reset(new DBusCommandProxy(nullptr, bus_,
command_instance_.get()));
- command_proxy_->RegisterAsync(base::Bind(NoAction));
+ command_instance_->SetProxy(command_proxy_.get());
+ command_proxy_->RegisterAsync(
+ AsyncEventSequencer::GetDefaultCompletionAction());
}
void TearDown() override {
EXPECT_CALL(*mock_exported_object_command_, Unregister()).Times(1);
+ command_instance_->SetProxy(nullptr);
command_proxy_.reset();
command_instance_.reset();
dict_.Clear();
@@ -115,6 +118,10 @@
return command_proxy_->progress_.value();
}
+ Dictionary GetParameters() const {
+ return command_proxy_->parameters_.value();
+ }
+
std::unique_ptr<dbus::Response> CallMethod(
const std::string& method_name,
const std::function<void(dbus::MessageWriter*)>& param_callback) {
@@ -168,17 +175,24 @@
};
TEST_F(DBusCommandProxyTest, Init) {
- EXPECT_EQ(dbus_constants::kCommandStatusQueued, GetStatus());
+ Dictionary params = {
+ {"height", int32_t{53}},
+ {"_jumpType", std::string{"_withKick"}},
+ };
+ EXPECT_EQ(CommandInstance::kStatusQueued, GetStatus());
EXPECT_EQ(0, GetProgress());
+ EXPECT_EQ(params, GetParameters());
EXPECT_EQ("robot.jump",
GetPropertyValue<std::string>(dbus_constants::kCommandName));
EXPECT_EQ(kTestCommandCategoty,
GetPropertyValue<std::string>(dbus_constants::kCommandCategory));
EXPECT_EQ(kTestCommandId,
GetPropertyValue<std::string>(dbus_constants::kCommandId));
- EXPECT_EQ(dbus_constants::kCommandStatusQueued,
+ EXPECT_EQ(CommandInstance::kStatusQueued,
GetPropertyValue<std::string>(dbus_constants::kCommandStatus));
EXPECT_EQ(0, GetPropertyValue<int32_t>(dbus_constants::kCommandProgress));
+ EXPECT_EQ(params,
+ GetPropertyValue<Dictionary>(dbus_constants::kCommandParameters));
}
TEST_F(DBusCommandProxyTest, SetProgress) {
@@ -188,9 +202,9 @@
writer->AppendInt32(10);
});
VerifyResponse(response, {});
- EXPECT_EQ(dbus_constants::kCommandStatusInProgress, GetStatus());
+ EXPECT_EQ(CommandInstance::kStatusInProgress, GetStatus());
EXPECT_EQ(10, GetProgress());
- EXPECT_EQ(dbus_constants::kCommandStatusInProgress,
+ EXPECT_EQ(CommandInstance::kStatusInProgress,
GetPropertyValue<std::string>(dbus_constants::kCommandStatus));
EXPECT_EQ(10, GetPropertyValue<int32_t>(dbus_constants::kCommandProgress));
}
@@ -201,7 +215,7 @@
writer->AppendInt32(110);
});
EXPECT_TRUE(IsResponseError(response));
- EXPECT_EQ(dbus_constants::kCommandStatusQueued, GetStatus());
+ EXPECT_EQ(CommandInstance::kStatusQueued, GetStatus());
EXPECT_EQ(0, GetProgress());
}
@@ -209,21 +223,25 @@
EXPECT_CALL(*mock_exported_object_command_, SendSignal(_)).Times(1);
auto response = CallMethod(dbus_constants::kCommandAbort, {});
VerifyResponse(response, {});
- EXPECT_EQ(dbus_constants::kCommandStatusAborted, GetStatus());
+ EXPECT_EQ(CommandInstance::kStatusAborted, GetStatus());
}
TEST_F(DBusCommandProxyTest, Cancel) {
EXPECT_CALL(*mock_exported_object_command_, SendSignal(_)).Times(1);
auto response = CallMethod(dbus_constants::kCommandCancel, {});
VerifyResponse(response, {});
- EXPECT_EQ(dbus_constants::kCommandStatusCanceled, GetStatus());
+ EXPECT_EQ(CommandInstance::kStatusCanceled, GetStatus());
}
TEST_F(DBusCommandProxyTest, Done) {
- EXPECT_CALL(*mock_exported_object_command_, SendSignal(_)).Times(2);
+ // 3 property updates:
+ // status: queued -> inProgress
+ // progress: 0 -> 100
+ // status: inProgress -> done
+ EXPECT_CALL(*mock_exported_object_command_, SendSignal(_)).Times(3);
auto response = CallMethod(dbus_constants::kCommandDone, {});
VerifyResponse(response, {});
- EXPECT_EQ(dbus_constants::kCommandStatusDone, GetStatus());
+ EXPECT_EQ(CommandInstance::kStatusDone, GetStatus());
EXPECT_EQ(100, GetProgress());
}
diff --git a/buffet/dbus_constants.cc b/buffet/dbus_constants.cc
index 7fc0acf..c3921f9 100644
--- a/buffet/dbus_constants.cc
+++ b/buffet/dbus_constants.cc
@@ -35,15 +35,7 @@
const char kCommandId[] = "Id";
const char kCommandStatus[] = "Status";
const char kCommandProgress[] = "Progress";
-
-const char kCommandStatusQueued[] = "queued";
-const char kCommandStatusInProgress[] = "inProgress";
-const char kCommandStatusPaused[] = "paused";
-const char kCommandStatusError[] = "error";
-const char kCommandStatusDone[] = "done";
-const char kCommandStatusCanceled[] = "canceled";
-const char kCommandStatusAborted[] = "aborted";
-const char kCommandStatusExpired[] = "expired";
+const char kCommandParameters[] = "Parameters";
} // namespace dbus_constants
diff --git a/buffet/dbus_constants.h b/buffet/dbus_constants.h
index aa6cf9f..398f848 100644
--- a/buffet/dbus_constants.h
+++ b/buffet/dbus_constants.h
@@ -43,16 +43,7 @@
extern const char kCommandId[];
extern const char kCommandStatus[];
extern const char kCommandProgress[];
-
-// Values for command execution status.
-extern const char kCommandStatusQueued[];
-extern const char kCommandStatusInProgress[];
-extern const char kCommandStatusPaused[];
-extern const char kCommandStatusError[];
-extern const char kCommandStatusDone[];
-extern const char kCommandStatusCanceled[];
-extern const char kCommandStatusAborted[];
-extern const char kCommandStatusExpired[];
+extern const char kCommandParameters[];
} // namespace dbus_constants
