Make checkpointing atomic am: 5eece04aaf am: f9916e90f7 am: 9176559700
Original change: https://android-review.googlesource.com/c/platform/system/update_engine/+/2597085
Change-Id: I87f74f5fe491629a1a80a7d2fdc7141414699553
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/common/fake_prefs.h b/common/fake_prefs.h
index 7ae9fb9..721cf24 100644
--- a/common/fake_prefs.h
+++ b/common/fake_prefs.h
@@ -59,6 +59,9 @@
void AddObserver(std::string_view key, ObserverInterface* observer) override;
void RemoveObserver(std::string_view key,
ObserverInterface* observer) override;
+ bool StartTransaction() override { return false; }
+ bool CancelTransaction() override { return false; }
+ bool SubmitTransaction() override { return false; }
private:
enum class PrefType {
diff --git a/common/mock_prefs.h b/common/mock_prefs.h
index f308074..028650d 100644
--- a/common/mock_prefs.h
+++ b/common/mock_prefs.h
@@ -47,6 +47,9 @@
MOCK_METHOD2(AddObserver, void(std::string_view key, ObserverInterface*));
MOCK_METHOD2(RemoveObserver, void(std::string_view key, ObserverInterface*));
+ MOCK_METHOD(bool, StartTransaction, (), (override));
+ MOCK_METHOD(bool, CancelTransaction, (), (override));
+ MOCK_METHOD(bool, SubmitTransaction, (), (override));
};
} // namespace chromeos_update_engine
diff --git a/common/prefs.cc b/common/prefs.cc
index a070302..57d4dcd 100644
--- a/common/prefs.cc
+++ b/common/prefs.cc
@@ -17,6 +17,8 @@
#include "update_engine/common/prefs.h"
#include <algorithm>
+#include <filesystem>
+#include <unistd.h>
#include <base/files/file_enumerator.h>
#include <base/files/file_util.h>
@@ -25,6 +27,7 @@
#include <base/strings/string_split.h>
#include <base/strings/string_util.h>
+#include "update_engine/common/platform_constants.h"
#include "update_engine/common/utils.h"
using std::string;
@@ -160,8 +163,62 @@
return file_storage_.Init(prefs_dir);
}
+bool PrefsBase::StartTransaction() {
+ return storage_->CreateTemporaryPrefs();
+}
+
+bool PrefsBase::CancelTransaction() {
+ return storage_->DeleteTemporaryPrefs();
+}
+
+bool PrefsBase::SubmitTransaction() {
+ return storage_->SwapPrefs();
+}
+
+std::string Prefs::FileStorage::GetTemporaryDir() const {
+ return prefs_dir_.value() + "_tmp";
+}
+
+bool Prefs::FileStorage::CreateTemporaryPrefs() {
+ // Delete any existing prefs_tmp
+ DeleteTemporaryPrefs();
+ // Get the paths to the source and destination directories.
+ std::filesystem::path source_directory(prefs_dir_.value());
+ std::filesystem::path destination_directory(GetTemporaryDir());
+
+ if (!std::filesystem::exists(source_directory)) {
+ LOG(ERROR) << "prefs directory does not exist: " << source_directory;
+ return false;
+ }
+ // Copy the directory.
+ std::filesystem::copy(source_directory, destination_directory);
+
+ return true;
+}
+
+bool Prefs::FileStorage::DeleteTemporaryPrefs() {
+ std::filesystem::path destination_directory(GetTemporaryDir());
+
+ if (std::filesystem::exists(destination_directory)) {
+ return std::filesystem::remove_all(destination_directory);
+ }
+ return true;
+}
+
+bool Prefs::FileStorage::SwapPrefs() {
+ if (std::filesystem::exists(prefs_dir_.value())) {
+ std::filesystem::remove_all(prefs_dir_.value());
+ }
+ if (rename(GetTemporaryDir().c_str(), prefs_dir_.value().c_str()) != 0) {
+ LOG(ERROR) << "Error replacing prefs with prefs_tmp" << strerror(errno);
+ return false;
+ }
+ return true;
+}
+
bool Prefs::FileStorage::Init(const base::FilePath& prefs_dir) {
prefs_dir_ = prefs_dir;
+
// Delete empty directories. Ignore errors when deleting empty directories.
DeleteEmptyDirectories(prefs_dir_);
return true;
@@ -231,8 +288,14 @@
for (char c : key)
TEST_AND_RETURN_FALSE(base::IsAsciiAlpha(c) || base::IsAsciiDigit(c) ||
c == '_' || c == '-' || c == kKeySeparator);
- *filename = prefs_dir_.Append(
- base::FilePath::StringPieceType(key.data(), key.size()));
+ if (std::filesystem::exists(GetTemporaryDir())) {
+ *filename =
+ base::FilePath(GetTemporaryDir())
+ .Append(base::FilePath::StringPieceType(key.data(), key.size()));
+ } else {
+ *filename = prefs_dir_.Append(
+ base::FilePath::StringPieceType(key.data(), key.size()));
+ }
return true;
}
diff --git a/common/prefs.h b/common/prefs.h
index c3105c6..d32e732 100644
--- a/common/prefs.h
+++ b/common/prefs.h
@@ -60,6 +60,17 @@
// key was deleted.
virtual bool DeleteKey(std::string_view key) = 0;
+ // Makes a copy of prefs directory called prefs_tmp, which is modified
+ // during update_engine checkpointing
+ virtual bool CreateTemporaryPrefs() { return false; }
+
+ // Deletes prefs_tmp directory
+ virtual bool DeleteTemporaryPrefs() { return false; }
+
+ // Replaces prefs with prefs_tmp to make update_engine checkpointing more
+ // atomic
+ virtual bool SwapPrefs() { return false; }
+
private:
DISALLOW_COPY_AND_ASSIGN(StorageInterface);
};
@@ -73,6 +84,9 @@
bool SetInt64(std::string_view key, const int64_t value) override;
bool GetBoolean(std::string_view key, bool* value) const override;
bool SetBoolean(std::string_view key, const bool value) override;
+ bool StartTransaction() override;
+ bool CancelTransaction() override;
+ bool SubmitTransaction() override;
bool Exists(std::string_view key) const override;
bool Delete(std::string_view key) override;
@@ -128,6 +142,9 @@
bool SetKey(std::string_view key, std::string_view value) override;
bool KeyExists(std::string_view key) const override;
bool DeleteKey(std::string_view key) override;
+ bool CreateTemporaryPrefs() override;
+ bool DeleteTemporaryPrefs() override;
+ bool SwapPrefs() override;
private:
FRIEND_TEST(PrefsTest, GetFileNameForKey);
@@ -139,6 +156,9 @@
bool GetFileNameForKey(std::string_view key,
base::FilePath* filename) const;
+ // Returns path of prefs_tmp used during update_engine checkpointing
+ std::string GetTemporaryDir() const;
+
// Preference store directory.
base::FilePath prefs_dir_;
};
diff --git a/common/prefs_interface.h b/common/prefs_interface.h
index 69ccf68..d56435f 100644
--- a/common/prefs_interface.h
+++ b/common/prefs_interface.h
@@ -105,6 +105,17 @@
virtual void RemoveObserver(std::string_view key,
ObserverInterface* observer) = 0;
+ // create tmp_prefs to write to during checkpointing. Flush will replace prefs
+ // with tmp_prefs
+ virtual bool StartTransaction() = 0;
+
+ // cancel any pending transaction by deleting tmp_prefs
+ virtual bool CancelTransaction() = 0;
+
+ // swap prefs with tmp_prefs checkpointing will use new tmp prefs to resume
+ // updates, otherwise we fall back on old prefs
+ virtual bool SubmitTransaction() = 0;
+
protected:
// Key separator used to create sub key and get file names,
static const char kKeySeparator = '/';
diff --git a/payload_consumer/delta_performer.cc b/payload_consumer/delta_performer.cc
index 3b9f2b6..3bd131d 100644
--- a/payload_consumer/delta_performer.cc
+++ b/payload_consumer/delta_performer.cc
@@ -1404,9 +1404,12 @@
return false;
}
Terminator::set_exit_blocked(true);
+ LOG_IF(WARNING, !prefs_->StartTransaction())
+ << "unable to start transaction in checkpointing";
+ DEFER {
+ prefs_->CancelTransaction();
+ };
if (last_updated_operation_num_ != next_operation_num_ || force) {
- // Resets the progress in case we die in the middle of the state update.
- ResetUpdateProgress(prefs_, true);
if (!signatures_message_data_.empty()) {
// Save the signature blob because if the update is interrupted after the
// download phase we don't go through this path anymore. Some alternatives
@@ -1458,6 +1461,9 @@
}
TEST_AND_RETURN_FALSE(
prefs_->SetInt64(kPrefsUpdateStateNextOperation, next_operation_num_));
+ if (!prefs_->SubmitTransaction()) {
+ LOG(ERROR) << "Failed to submit transaction in checkpointing";
+ }
return true;
}
diff --git a/payload_consumer/delta_performer.h b/payload_consumer/delta_performer.h
index 12238a7..ce37bd1 100644
--- a/payload_consumer/delta_performer.h
+++ b/payload_consumer/delta_performer.h
@@ -151,9 +151,9 @@
// Attempts to parse the update metadata starting from the beginning of
// |payload|. On success, returns kMetadataParseSuccess. Returns
- // kMetadataParseInsufficientData if more data is needed to parse the complete
- // metadata. Returns kMetadataParseError if the metadata can't be parsed given
- // the payload.
+ // kMetadataParseInsufficientData if more data is needed to parse the
+ // complete metadata. Returns kMetadataParseError if the metadata can't be
+ // parsed given the payload.
MetadataParseResult ParsePayloadMetadata(const brillo::Blob& payload,
ErrorCode* error);