update_engine: Make SystemState accessible from everywhere
SystemState is supposed to be a global context and is used lamost
everywhere. So instead of passing it to functions and keeping multiple
pointers to it, its better to do what we did in dlcservice and make it a
singleton class with a getter that can be get from everywhere.
BUG=b:171829801
TEST=unittests
Change-Id: I3b2de9394b7769b3911195ca52d61dbe49afd4dd
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2521792
Commit-Queue: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Jae Hoon Kim <kimjae@chromium.org>
diff --git a/common/download_action.h b/common/download_action.h
index c167c2d..18e5853 100644
--- a/common/download_action.h
+++ b/common/download_action.h
@@ -28,7 +28,6 @@
#include "update_engine/common/boot_control_interface.h"
#include "update_engine/common/http_fetcher.h"
#include "update_engine/common/multi_range_http_fetcher.h"
-#include "update_engine/common/system_state.h"
#include "update_engine/payload_consumer/delta_performer.h"
#include "update_engine/payload_consumer/install_plan.h"
@@ -71,12 +70,11 @@
// Takes ownership of the passed in HttpFetcher. Useful for testing.
// A good calling pattern is:
- // DownloadAction(prefs, boot_contol, hardware, system_state,
+ // DownloadAction(prefs, boot_contol, hardware,
// new WhateverHttpFetcher, false);
DownloadAction(PrefsInterface* prefs,
BootControlInterface* boot_control,
HardwareInterface* hardware,
- SystemState* system_state,
HttpFetcher* http_fetcher,
bool interactive);
~DownloadAction() override;
@@ -141,14 +139,11 @@
// Pointer to the current payload in install_plan_.payloads.
InstallPlan::Payload* payload_{nullptr};
- // SystemState required pointers.
+ // Required pointers.
PrefsInterface* prefs_;
BootControlInterface* boot_control_;
HardwareInterface* hardware_;
- // Global context for the system.
- SystemState* system_state_;
-
// Pointer to the MultiRangeHttpFetcher that does the http work.
std::unique_ptr<MultiRangeHttpFetcher> http_fetcher_;
diff --git a/common/metrics_reporter_interface.h b/common/metrics_reporter_interface.h
index d7c5347..08636e3 100644
--- a/common/metrics_reporter_interface.h
+++ b/common/metrics_reporter_interface.h
@@ -25,19 +25,12 @@
#include "update_engine/common/constants.h"
#include "update_engine/common/error_code.h"
#include "update_engine/common/metrics_constants.h"
-#include "update_engine/common/system_state.h"
namespace chromeos_update_engine {
enum class ServerToCheck;
enum class CertificateCheckResult;
-namespace metrics {
-
-std::unique_ptr<MetricsReporterInterface> CreateMetricsReporter();
-
-} // namespace metrics
-
class MetricsReporterInterface {
public:
virtual ~MetricsReporterInterface() = default;
@@ -92,7 +85,6 @@
// if it's set, |kMetricCheckRollbackTargetVersion| reports the same, but only
// if rollback is also allowed using enterprise policy.
virtual void ReportUpdateCheckMetrics(
- SystemState* system_state,
metrics::CheckResult result,
metrics::CheckReaction reaction,
metrics::DownloadErrorCode download_error_code) = 0;
@@ -120,8 +112,7 @@
// |kMetricAttemptTimeSinceLastAttemptUptimeMinutes| metrics are
// automatically calculated and reported by maintaining persistent and
// process-local state variables.
- virtual void ReportUpdateAttemptMetrics(SystemState* system_state,
- int attempt_number,
+ virtual void ReportUpdateAttemptMetrics(int attempt_number,
PayloadType payload_type,
base::TimeDelta duration,
base::TimeDelta duration_uptime,
@@ -242,6 +233,12 @@
bool has_time_restriction_policy, int time_to_update_days) = 0;
};
+namespace metrics {
+
+std::unique_ptr<MetricsReporterInterface> CreateMetricsReporter();
+
+} // namespace metrics
+
} // namespace chromeos_update_engine
#endif // UPDATE_ENGINE_COMMON_METRICS_REPORTER_INTERFACE_H_
diff --git a/common/metrics_reporter_stub.h b/common/metrics_reporter_stub.h
index 1470aaa..80cf469 100644
--- a/common/metrics_reporter_stub.h
+++ b/common/metrics_reporter_stub.h
@@ -39,13 +39,11 @@
void ReportDailyMetrics(base::TimeDelta os_age) override {}
void ReportUpdateCheckMetrics(
- SystemState* system_state,
metrics::CheckResult result,
metrics::CheckReaction reaction,
metrics::DownloadErrorCode download_error_code) override {}
- void ReportUpdateAttemptMetrics(SystemState* system_state,
- int attempt_number,
+ void ReportUpdateAttemptMetrics(int attempt_number,
PayloadType payload_type,
base::TimeDelta duration,
base::TimeDelta duration_uptime,
diff --git a/common/mock_metrics_reporter.h b/common/mock_metrics_reporter.h
index 922d1ee..1bb1e84 100644
--- a/common/mock_metrics_reporter.h
+++ b/common/mock_metrics_reporter.h
@@ -36,15 +36,13 @@
MOCK_METHOD1(ReportDailyMetrics, void(base::TimeDelta os_age));
- MOCK_METHOD4(ReportUpdateCheckMetrics,
- void(SystemState* system_state,
- metrics::CheckResult result,
+ MOCK_METHOD3(ReportUpdateCheckMetrics,
+ void(metrics::CheckResult result,
metrics::CheckReaction reaction,
metrics::DownloadErrorCode download_error_code));
- MOCK_METHOD8(ReportUpdateAttemptMetrics,
- void(SystemState* system_state,
- int attempt_number,
+ MOCK_METHOD7(ReportUpdateAttemptMetrics,
+ void(int attempt_number,
PayloadType payload_type,
base::TimeDelta duration,
base::TimeDelta duration_uptime,
diff --git a/common/system_state.cc b/common/system_state.cc
new file mode 100644
index 0000000..40bf760
--- /dev/null
+++ b/common/system_state.cc
@@ -0,0 +1,23 @@
+//
+// Copyright (C) 2020 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+
+#include "update_engine/common/system_state.h"
+
+namespace chromeos_update_engine {
+
+std::unique_ptr<SystemState> SystemState::g_instance_;
+
+} // namespace chromeos_update_engine
diff --git a/common/system_state.h b/common/system_state.h
index 7a67046..dc40d36 100644
--- a/common/system_state.h
+++ b/common/system_state.h
@@ -17,6 +17,10 @@
#ifndef UPDATE_ENGINE_COMMON_SYSTEM_STATE_H_
#define UPDATE_ENGINE_COMMON_SYSTEM_STATE_H_
+#include <memory>
+
+#include <base/logging.h>
+
namespace chromeos_update_manager {
class UpdateManager;
@@ -51,13 +55,14 @@
// the current state of the system, high-level objects whose lifetime is same
// as main, system interfaces, etc.
// Carved out separately so it can be mocked for unit tests.
-// Currently it has only one method, but we should start migrating other
-// methods to use this as and when needed to unit test them.
-// TODO(jaysri): Consider renaming this to something like GlobalContext.
class SystemState {
public:
- // Destructs this object.
- virtual ~SystemState() {}
+ virtual ~SystemState() = default;
+
+ static SystemState* Get() {
+ CHECK(g_instance_);
+ return g_instance_.get();
+ }
// Sets or gets the latest device policy.
virtual void set_device_policy(const policy::DevicePolicy* device_policy) = 0;
@@ -113,6 +118,9 @@
// Returns a pointer to the DlcServiceInterface singleton.
virtual DlcServiceInterface* dlcservice() = 0;
+
+ protected:
+ static std::unique_ptr<SystemState> g_instance_;
};
} // namespace chromeos_update_engine