update_engine: Use powerd for rebooting.
Make update_engine call powerd's RequestRestart method to
restart the system instead of running "shutdown -r now"
directly. This avoids races where Chrome asks powerd to shut
down the system but then update_engine sees that an update
has been applied and reboots it instead.
BUG=chromium:372074
TEST=applied an update and requested a reboot from the UI;
checked that the request was logged by powerd. held the
power button to shut down while an update was pending
and checked that the system shut down instead of
rebooting. built update_engine with the
power_management USE flag unset and checked that it was
still able to reboot the system.
CQ-DEPEND=I73f67cca9875b00bd944e4ff9779ad24fc4cd738
CQ-DEPEND=I2888fdde50a71460b74f2ef33d63ac00c5938626
Change-Id: Ic5f7c33c3f7fc5fe92ac8a2e1800830678cac235
Reviewed-on: https://chromium-review.googlesource.com/206074
Tested-by: Daniel Erat <derat@chromium.org>
Reviewed-by: Chris Sosa <sosa@chromium.org>
Commit-Queue: Daniel Erat <derat@chromium.org>
diff --git a/dbus_wrapper_interface.h b/dbus_wrapper_interface.h
index 360657d..7bed2e6 100644
--- a/dbus_wrapper_interface.h
+++ b/dbus_wrapper_interface.h
@@ -46,6 +46,11 @@
GError** error,
GHashTable** out1) = 0;
+ virtual gboolean ProxyCall_1_0(DBusGProxy* proxy,
+ const char* method,
+ GError** error,
+ gint in1) = 0;
+
virtual gboolean ProxyCall_3_0(DBusGProxy* proxy,
const char* method,
GError** error,
diff --git a/mock_dbus_wrapper.h b/mock_dbus_wrapper.h
index 569e289..444d0d8 100644
--- a/mock_dbus_wrapper.h
+++ b/mock_dbus_wrapper.h
@@ -26,6 +26,10 @@
const char *method,
GError **error,
GHashTable** out1));
+ MOCK_METHOD4(ProxyCall_1_0, gboolean(DBusGProxy *proxy,
+ const char *method,
+ GError **error,
+ gint in1));
MOCK_METHOD6(ProxyCall_3_0, gboolean(DBusGProxy* proxy,
const char* method,
GError** error,
diff --git a/real_dbus_wrapper.h b/real_dbus_wrapper.h
index 4d31651..22dddde 100644
--- a/real_dbus_wrapper.h
+++ b/real_dbus_wrapper.h
@@ -7,6 +7,7 @@
// A mockable interface for DBus.
+#include <base/macros.h>
#include <dbus/dbus-glib-lowlevel.h>
#include <dbus/dbus-glib.h>
@@ -18,25 +19,25 @@
virtual DBusGProxy* ProxyNewForName(DBusGConnection* connection,
const char* name,
const char* path,
- const char* interface) {
+ const char* interface) OVERRIDE {
return dbus_g_proxy_new_for_name(connection,
name,
path,
interface);
}
- virtual void ProxyUnref(DBusGProxy* proxy) {
+ virtual void ProxyUnref(DBusGProxy* proxy) OVERRIDE {
g_object_unref(proxy);
}
- virtual DBusGConnection* BusGet(DBusBusType type, GError** error) {
+ virtual DBusGConnection* BusGet(DBusBusType type, GError** error) OVERRIDE {
return dbus_g_bus_get(type, error);
}
virtual gboolean ProxyCall_0_1(DBusGProxy* proxy,
const char* method,
GError** error,
- GHashTable** out1) {
+ GHashTable** out1) OVERRIDE {
return dbus_g_proxy_call(proxy, method, error, G_TYPE_INVALID,
dbus_g_type_get_map("GHashTable",
G_TYPE_STRING,
@@ -44,12 +45,21 @@
out1, G_TYPE_INVALID);
}
+ virtual gboolean ProxyCall_1_0(DBusGProxy* proxy,
+ const char* method,
+ GError** error,
+ gint in1) OVERRIDE {
+ return dbus_g_proxy_call(proxy, method, error,
+ G_TYPE_INT, in1,
+ G_TYPE_INVALID, G_TYPE_INVALID);
+ }
+
virtual gboolean ProxyCall_3_0(DBusGProxy* proxy,
const char* method,
GError** error,
const char* in1,
const char* in2,
- const char* in3) {
+ const char* in3) OVERRIDE {
return dbus_g_proxy_call(
proxy, method, error,
G_TYPE_STRING, in1, G_TYPE_STRING, in2, G_TYPE_STRING, in3,
@@ -59,7 +69,7 @@
virtual void ProxyAddSignal_2(DBusGProxy* proxy,
const char* signal_name,
GType type1,
- GType type2) {
+ GType type2) OVERRIDE {
dbus_g_proxy_add_signal(proxy, signal_name, type1, type2, G_TYPE_INVALID);
}
@@ -67,7 +77,7 @@
const char* signal_name,
GCallback handler,
void* data,
- GClosureNotify free_data_func) {
+ GClosureNotify free_data_func) OVERRIDE {
dbus_g_proxy_connect_signal(proxy, signal_name, handler, data,
free_data_func);
}
@@ -75,17 +85,18 @@
virtual void ProxyDisconnectSignal(DBusGProxy* proxy,
const char* signal_name,
GCallback handler,
- void* data) {
+ void* data) OVERRIDE {
dbus_g_proxy_disconnect_signal(proxy, signal_name, handler, data);
}
- virtual DBusConnection* ConnectionGetConnection(DBusGConnection* gbus) {
+ virtual DBusConnection* ConnectionGetConnection(
+ DBusGConnection* gbus) OVERRIDE {
return dbus_g_connection_get_connection(gbus);
}
virtual void DBusBusAddMatch(DBusConnection* connection,
const char* rule,
- DBusError* error) {
+ DBusError* error) OVERRIDE {
dbus_bus_add_match(connection, rule, error);
}
@@ -93,7 +104,7 @@
DBusConnection* connection,
DBusHandleMessageFunction function,
void* user_data,
- DBusFreeFunction free_data_function) {
+ DBusFreeFunction free_data_function) OVERRIDE {
return dbus_connection_add_filter(connection,
function,
user_data,
@@ -102,13 +113,13 @@
virtual void DBusConnectionRemoveFilter(DBusConnection* connection,
DBusHandleMessageFunction function,
- void* user_data) {
+ void* user_data) OVERRIDE {
dbus_connection_remove_filter(connection, function, user_data);
}
dbus_bool_t DBusMessageIsSignal(DBusMessage* message,
const char* interface,
- const char* signal_name) {
+ const char* signal_name) OVERRIDE {
return dbus_message_is_signal(message, interface, signal_name);
}
@@ -116,7 +127,7 @@
DBusError* error,
char** out1,
char** out2,
- char** out3) {
+ char** out3) OVERRIDE {
return dbus_message_get_args(message, error,
DBUS_TYPE_STRING, out1,
DBUS_TYPE_STRING, out2,
diff --git a/update_attempter.cc b/update_attempter.cc
index 520d9c9..24f727b 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -14,6 +14,7 @@
#include <base/file_util.h>
#include <base/logging.h>
#include <base/rand_util.h>
+#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
#include <chromeos/dbus/service_constants.h>
@@ -26,6 +27,7 @@
#include "update_engine/clock_interface.h"
#include "update_engine/constants.h"
#include "update_engine/dbus_service.h"
+#include "update_engine/dbus_wrapper_interface.h"
#include "update_engine/download_action.h"
#include "update_engine/filesystem_copier_action.h"
#include "update_engine/hardware_interface.h"
@@ -122,13 +124,15 @@
const std::string& update_completed_marker)
: processor_(new ActionProcessor()),
system_state_(system_state),
+ dbus_iface_(dbus_iface),
chrome_proxy_resolver_(dbus_iface),
update_completed_marker_(update_completed_marker) {
if (!update_completed_marker_.empty() &&
- utils::FileExists(update_completed_marker_.c_str()))
+ utils::FileExists(update_completed_marker_.c_str())) {
status_ = UPDATE_STATUS_UPDATED_NEED_REBOOT;
- else
+ } else {
status_ = UPDATE_STATUS_IDLE;
+ }
}
void UpdateAttempter::Init() {
@@ -812,8 +816,11 @@
<< UpdateStatusToString(status_) << ", so not rebooting.";
return false;
}
- TEST_AND_RETURN_FALSE(utils::Reboot());
- return true;
+
+ if (USE_POWER_MANAGEMENT && RequestPowerManagerReboot())
+ return true;
+
+ return RebootDirectly();
}
void UpdateAttempter::WriteUpdateCompletedMarker() {
@@ -828,6 +835,49 @@
contents.length());
}
+bool UpdateAttempter::RequestPowerManagerReboot() {
+ GError* error = NULL;
+ DBusGConnection* bus = dbus_iface_->BusGet(DBUS_BUS_SYSTEM, &error);
+ if (!bus) {
+ LOG(ERROR) << "Failed to get system bus: "
+ << utils::GetAndFreeGError(&error);
+ return false;
+ }
+
+ LOG(INFO) << "Calling " << power_manager::kPowerManagerInterface << "."
+ << power_manager::kRequestRestartMethod;
+ DBusGProxy* proxy = dbus_iface_->ProxyNewForName(
+ bus,
+ power_manager::kPowerManagerServiceName,
+ power_manager::kPowerManagerServicePath,
+ power_manager::kPowerManagerInterface);
+ const gboolean success = dbus_iface_->ProxyCall_1_0(
+ proxy,
+ power_manager::kRequestRestartMethod,
+ &error,
+ power_manager::REQUEST_RESTART_FOR_UPDATE);
+ dbus_iface_->ProxyUnref(proxy);
+
+ if (!success) {
+ LOG(ERROR) << "Failed to call " << power_manager::kRequestRestartMethod
+ << ": " << utils::GetAndFreeGError(&error);
+ return false;
+ }
+
+ return true;
+}
+
+bool UpdateAttempter::RebootDirectly() {
+ vector<string> command;
+ command.push_back("/sbin/shutdown");
+ command.push_back("-r");
+ command.push_back("now");
+ LOG(INFO) << "Running \"" << JoinString(command, ' ') << "\"";
+ int rc = 0;
+ Subprocess::SynchronousExec(command, &rc, NULL);
+ return rc == 0;
+}
+
// Delegate methods:
void UpdateAttempter::ProcessingDone(const ActionProcessor* processor,
ErrorCode code) {
diff --git a/update_attempter.h b/update_attempter.h
index b765a2b..420b876 100644
--- a/update_attempter.h
+++ b/update_attempter.h
@@ -34,6 +34,7 @@
namespace chromeos_update_engine {
+class DBusWrapperInterface;
class UpdateCheckScheduler;
enum UpdateStatus {
@@ -347,6 +348,14 @@
// |update_completed_marker_| is empty.
void WriteUpdateCompletedMarker();
+ // Sends a D-Bus message to the Chrome OS power manager asking it to reboot
+ // the system. Returns true on success.
+ bool RequestPowerManagerReboot();
+
+ // Reboots the system directly by calling /sbin/shutdown. Returns true on
+ // success.
+ bool RebootDirectly();
+
// Last status notification timestamp used for throttling. Use monotonic
// TimeTicks to ensure that notifications are sent even if the system clock is
// set back in the middle of an update.
@@ -359,6 +368,9 @@
// carved out separately to mock out easily in unit tests.
SystemState* system_state_;
+ // Interface for getting D-Bus connections.
+ DBusWrapperInterface* dbus_iface_ = nullptr;
+
// If non-null, this UpdateAttempter will send status updates over this
// dbus service.
UpdateEngineService* dbus_service_ = nullptr;
diff --git a/utils.cc b/utils.cc
index af536d0..f78861a 100644
--- a/utils.cc
+++ b/utils.cc
@@ -765,17 +765,6 @@
return message;
}
-bool Reboot() {
- vector<string> command;
- command.push_back("/sbin/shutdown");
- command.push_back("-r");
- command.push_back("now");
- int rc = 0;
- Subprocess::SynchronousExec(command, &rc, NULL);
- TEST_AND_RETURN_FALSE(rc == 0);
- return true;
-}
-
namespace {
// Do the actual trigger. We do it as a main-loop callback to (try to) get a
// consistent stack trace.
diff --git a/utils.h b/utils.h
index 04efffa..b35bf30 100644
--- a/utils.h
+++ b/utils.h
@@ -223,9 +223,6 @@
// object and resets error to NULL.
std::string GetAndFreeGError(GError** error);
-// Initiates a system reboot. Returns true on success, false otherwise.
-bool Reboot();
-
// Schedules a Main Loop callback to trigger the crash reporter to perform an
// upload as if this process had crashed.
void ScheduleCrashReporterUpload();