Return the error reason to the caller of SetTargetChannel.
This patch sends back to the caller an error message indicating why the
channel change didn't work.
Bug: 25595865
Test: Deployed on a device and attempted to change to "foo" channel. Error message lists available channels.
Test: FEATURES=test emerge-link update_engine
Change-Id: Idcc67d5c7878ce7af60652d7bf5bf81135325f97
diff --git a/dbus_service.cc b/dbus_service.cc
index 1bde41c..16b10d4 100644
--- a/dbus_service.cc
+++ b/dbus_service.cc
@@ -170,10 +170,10 @@
}
LOG(INFO) << "Setting destination channel to: " << in_target_channel;
+ string error_message;
if (!system_state_->request_params()->SetTargetChannel(
- in_target_channel, in_is_powerwash_allowed)) {
- // TODO(dgarrett): Give a more specific error code/reason.
- LogAndSetError(error, FROM_HERE, "Setting channel failed.");
+ in_target_channel, in_is_powerwash_allowed, &error_message)) {
+ LogAndSetError(error, FROM_HERE, error_message);
return false;
}
return true;
diff --git a/dbus_service_unittest.cc b/dbus_service_unittest.cc
index 4ea7eec..db63b2d 100644
--- a/dbus_service_unittest.cc
+++ b/dbus_service_unittest.cc
@@ -80,7 +80,7 @@
EXPECT_CALL(*mock_update_attempter_, RefreshDevicePolicy());
// If SetTargetChannel is called it means the policy check passed.
EXPECT_CALL(*fake_system_state_.mock_request_params(),
- SetTargetChannel("stable-channel", true))
+ SetTargetChannel("stable-channel", true, _))
.WillOnce(Return(true));
EXPECT_TRUE(dbus_service_.SetChannel(&error_, "stable-channel", true));
ASSERT_EQ(nullptr, error_);
@@ -93,7 +93,7 @@
EXPECT_CALL(mock_device_policy, GetReleaseChannelDelegated(_))
.WillOnce(DoAll(SetArgumentPointee<0>(true), Return(true)));
EXPECT_CALL(*fake_system_state_.mock_request_params(),
- SetTargetChannel("beta-channel", true))
+ SetTargetChannel("beta-channel", true, _))
.WillOnce(Return(true));
EXPECT_TRUE(dbus_service_.SetChannel(&error_, "beta-channel", true));
@@ -105,7 +105,7 @@
TEST_F(UpdateEngineServiceTest, SetChannelWithInvalidChannel) {
EXPECT_CALL(*mock_update_attempter_, RefreshDevicePolicy());
EXPECT_CALL(*fake_system_state_.mock_request_params(),
- SetTargetChannel("foo-channel", true)).WillOnce(Return(false));
+ SetTargetChannel("foo-channel", true, _)).WillOnce(Return(false));
EXPECT_FALSE(dbus_service_.SetChannel(&error_, "foo-channel", true));
ASSERT_NE(nullptr, error_);
diff --git a/mock_omaha_request_params.h b/mock_omaha_request_params.h
index 38d370c..5d5d47b 100644
--- a/mock_omaha_request_params.h
+++ b/mock_omaha_request_params.h
@@ -38,7 +38,7 @@
ON_CALL(*this, GetAppId())
.WillByDefault(testing::Invoke(
this, &MockOmahaRequestParams::FakeGetAppId));
- ON_CALL(*this, SetTargetChannel(testing::_, testing::_))
+ ON_CALL(*this, SetTargetChannel(testing::_, testing::_, testing::_))
.WillByDefault(testing::Invoke(
this, &MockOmahaRequestParams::FakeSetTargetChannel));
ON_CALL(*this, UpdateDownloadChannel())
@@ -51,8 +51,9 @@
MOCK_CONST_METHOD0(to_more_stable_channel, bool(void));
MOCK_CONST_METHOD0(GetAppId, std::string(void));
- MOCK_METHOD2(SetTargetChannel, bool(const std::string& channel,
- bool is_powerwash_allowed));
+ MOCK_METHOD3(SetTargetChannel, bool(const std::string& channel,
+ bool is_powerwash_allowed,
+ std::string* error));
MOCK_METHOD0(UpdateDownloadChannel, void(void));
MOCK_CONST_METHOD0(is_powerwash_allowed, bool(void));
MOCK_CONST_METHOD0(IsUpdateUrlOfficial, bool(void));
@@ -69,8 +70,11 @@
}
bool FakeSetTargetChannel(const std::string& channel,
- bool is_powerwash_allowed) {
- return OmahaRequestParams::SetTargetChannel(channel, is_powerwash_allowed);
+ bool is_powerwash_allowed,
+ std::string* error) {
+ return OmahaRequestParams::SetTargetChannel(channel,
+ is_powerwash_allowed,
+ error);
}
void FakeUpdateDownloadChannel() {
diff --git a/omaha_request_params.cc b/omaha_request_params.cc
index f6fad1c..ef137b4 100644
--- a/omaha_request_params.cc
+++ b/omaha_request_params.cc
@@ -26,7 +26,9 @@
#include <base/files/file_util.h>
#include <base/strings/string_util.h>
+#include <base/strings/stringprintf.h>
#include <brillo/key_value_store.h>
+#include <brillo/strings/string_utils.h>
#include <policy/device_policy.h>
#include "update_engine/constants.h"
@@ -131,7 +133,8 @@
}
bool OmahaRequestParams::SetTargetChannel(const string& new_target_channel,
- bool is_powerwash_allowed) {
+ bool is_powerwash_allowed,
+ string* error_message) {
LOG(INFO) << "SetTargetChannel called with " << new_target_channel
<< ", Is Powerwash Allowed = "
<< utils::ToString(is_powerwash_allowed)
@@ -139,13 +142,28 @@
<< ", existing target channel = "
<< mutable_image_props_.target_channel
<< ", download channel = " << download_channel_;
- TEST_AND_RETURN_FALSE(IsValidChannel(new_target_channel));
+ if (!IsValidChannel(new_target_channel)) {
+ string valid_channels = brillo::string_utils::JoinRange(
+ ", ",
+ std::begin(kChannelsByStability),
+ std::end(kChannelsByStability));
+ if (error_message) {
+ *error_message = base::StringPrintf(
+ "Invalid channel name \"%s\", valid names are: %s",
+ new_target_channel.c_str(), valid_channels.c_str());
+ }
+ return false;
+ }
MutableImageProperties new_props;
new_props.target_channel = new_target_channel;
new_props.is_powerwash_allowed = is_powerwash_allowed;
- TEST_AND_RETURN_FALSE(StoreMutableImageProperties(system_state_, new_props));
+ if (!StoreMutableImageProperties(system_state_, new_props)) {
+ if (error_message)
+ *error_message = "Error storing the new channel value.";
+ return false;
+ }
mutable_image_props_ = new_props;
return true;
}
diff --git a/omaha_request_params.h b/omaha_request_params.h
index b6b6f04..90cbc6e 100644
--- a/omaha_request_params.h
+++ b/omaha_request_params.h
@@ -206,7 +206,8 @@
// numerous edge cases around ensuring the powerwash happens as intended in
// all such cases.
virtual bool SetTargetChannel(const std::string& channel,
- bool is_powerwash_allowed);
+ bool is_powerwash_allowed,
+ std::string* error_message);
// Updates the download channel for this particular attempt from the current
// value of target channel. This method takes a "snapshot" of the current
diff --git a/omaha_request_params_unittest.cc b/omaha_request_params_unittest.cc
index 473b101..7ee8d98 100644
--- a/omaha_request_params_unittest.cc
+++ b/omaha_request_params_unittest.cc
@@ -353,7 +353,7 @@
OmahaRequestParams params(&fake_system_state_);
params.set_root(test_dir_);
EXPECT_TRUE(params.Init("", "", false));
- params.SetTargetChannel("canary-channel", false);
+ params.SetTargetChannel("canary-channel", false, nullptr);
EXPECT_FALSE(params.is_powerwash_allowed());
}
OmahaRequestParams out(&fake_system_state_);
@@ -375,7 +375,7 @@
OmahaRequestParams params(&fake_system_state_);
params.set_root(test_dir_);
EXPECT_TRUE(params.Init("", "", false));
- params.SetTargetChannel("canary-channel", true);
+ params.SetTargetChannel("canary-channel", true, nullptr);
EXPECT_TRUE(params.is_powerwash_allowed());
}
OmahaRequestParams out(&fake_system_state_);
@@ -398,7 +398,11 @@
params.set_root(test_dir_);
SetLockDown(true);
EXPECT_TRUE(params.Init("", "", false));
- params.SetTargetChannel("dogfood-channel", true);
+ string error_message;
+ EXPECT_FALSE(
+ params.SetTargetChannel("dogfood-channel", true, &error_message));
+ // The error message should include a message about the valid channels.
+ EXPECT_NE(string::npos, error_message.find("stable-channel"));
EXPECT_FALSE(params.is_powerwash_allowed());
}
OmahaRequestParams out(&fake_system_state_);
@@ -459,23 +463,23 @@
// When an invalid value is set, it should be ignored and the
// value from lsb-release should be used instead.
params_.Init("", "", false);
- EXPECT_FALSE(params_.SetTargetChannel("invalid-channel", false));
+ EXPECT_FALSE(params_.SetTargetChannel("invalid-channel", false, nullptr));
EXPECT_EQ("dev-channel", params_.target_channel());
// When set to a valid value, it should take effect.
params_.Init("", "", false);
- EXPECT_TRUE(params_.SetTargetChannel("beta-channel", true));
+ EXPECT_TRUE(params_.SetTargetChannel("beta-channel", true, nullptr));
EXPECT_EQ("beta-channel", params_.target_channel());
// When set to the same value, it should be idempotent.
params_.Init("", "", false);
- EXPECT_TRUE(params_.SetTargetChannel("beta-channel", true));
+ EXPECT_TRUE(params_.SetTargetChannel("beta-channel", true, nullptr));
EXPECT_EQ("beta-channel", params_.target_channel());
// When set to a valid value while a change is already pending, it should
// succeed.
params_.Init("", "", false);
- EXPECT_TRUE(params_.SetTargetChannel("stable-channel", true));
+ EXPECT_TRUE(params_.SetTargetChannel("stable-channel", true, nullptr));
EXPECT_EQ("stable-channel", params_.target_channel());
// Set a different channel in stateful LSB release.
@@ -487,7 +491,7 @@
// When set to a valid value while a change is already pending, it should
// succeed.
params_.Init("", "", false);
- EXPECT_TRUE(params_.SetTargetChannel("beta-channel", true));
+ EXPECT_TRUE(params_.SetTargetChannel("beta-channel", true, nullptr));
// The target channel should reflect the change, but the download channel
// should continue to retain the old value ...
EXPECT_EQ("beta-channel", params_.target_channel());
diff --git a/omaha_response_handler_action_unittest.cc b/omaha_response_handler_action_unittest.cc
index 5e62f45..eb6a5af 100644
--- a/omaha_response_handler_action_unittest.cc
+++ b/omaha_response_handler_action_unittest.cc
@@ -387,7 +387,7 @@
params.set_root(test_dir);
params.Init("5.6.7.8", "", 0);
EXPECT_EQ("stable-channel", params.current_channel());
- params.SetTargetChannel("canary-channel", false);
+ params.SetTargetChannel("canary-channel", false, nullptr);
EXPECT_EQ("canary-channel", params.target_channel());
EXPECT_FALSE(params.to_more_stable_channel());
EXPECT_FALSE(params.is_powerwash_allowed());
diff --git a/update_attempter.cc b/update_attempter.cc
index 09dd108..7d6aaf2 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -381,7 +381,11 @@
LOG(INFO) << "Setting target channel as mandated: " << target_channel;
// Pass in false for powerwash_allowed until we add it to the policy
// protobuf.
- omaha_request_params_->SetTargetChannel(target_channel, false);
+ string error_message;
+ if (!omaha_request_params_->SetTargetChannel(target_channel, false,
+ &error_message)) {
+ LOG(ERROR) << "Setting the channel failed: " << error_message;
+ }
// Since this is the beginning of a new attempt, update the download
// channel. The download channel won't be updated until the next attempt,