libsnapshot: Don't compare strings for snapshot state.
Convert the string field to an enum. We still write a string back to the
state file.
Bug: N/A
Test: libsnapshot_test gtest
Change-Id: I7cc1cb597dacd7d6faaaba05fb01c0a86bd54c8f
diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h
index 4fb6808..1630ee5 100644
--- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h
+++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h
@@ -219,9 +219,12 @@
bool WriteUpdateState(LockedFile* file, UpdateState state);
std::string GetStateFilePath() const;
+ enum class SnapshotState : int { Created, Merging, MergeCompleted };
+ static std::string to_string(SnapshotState state);
+
// This state is persisted per-snapshot in /metadata/ota/snapshots/.
struct SnapshotStatus {
- std::string state;
+ SnapshotState state;
uint64_t device_size;
uint64_t snapshot_size;
// These are non-zero when merging.
diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp
index 75a1f26..88800ed 100644
--- a/fs_mgr/libsnapshot/snapshot.cpp
+++ b/fs_mgr/libsnapshot/snapshot.cpp
@@ -165,7 +165,7 @@
// actual backing image. This is harmless, since it'll get removed when
// CancelUpdate is called.
SnapshotStatus status = {
- .state = "created",
+ .state = SnapshotState::Created,
.device_size = device_size,
.snapshot_size = snapshot_size,
};
@@ -190,7 +190,7 @@
if (!ReadSnapshotStatus(lock, name, &status)) {
return false;
}
- if (status.state == "merge-completed") {
+ if (status.state == SnapshotState::MergeCompleted) {
LOG(ERROR) << "Should not create a snapshot device for " << name
<< " after merging has completed.";
return false;
@@ -422,8 +422,8 @@
if (!ReadSnapshotStatus(lock, name, &status)) {
return false;
}
- if (status.state != "created") {
- LOG(WARNING) << "Snapshot " << name << " has unexpected state: " << status.state;
+ if (status.state != SnapshotState::Created) {
+ LOG(WARNING) << "Snapshot " << name << " has unexpected state: " << to_string(status.state);
}
// After this, we return true because we technically did switch to a merge
@@ -433,7 +433,7 @@
return false;
}
- status.state = "merging";
+ status.state = SnapshotState::Merging;
DmTargetSnapshot::Status dm_status;
if (!QuerySnapshotStatus(dm_name, nullptr, &dm_status)) {
@@ -657,7 +657,7 @@
// rebooted after this check, the device will still be a snapshot-merge
// target. If the have rebooted, the device will now be a linear target,
// and we can try cleanup again.
- if (snapshot_status.state == "merge-complete" && !IsSnapshotDevice(dm_name)) {
+ if (snapshot_status.state == SnapshotState::MergeCompleted && !IsSnapshotDevice(dm_name)) {
// NB: It's okay if this fails now, we gave cleanup our best effort.
OnSnapshotMergeComplete(lock, name, snapshot_status);
return UpdateState::MergeCompleted;
@@ -678,7 +678,7 @@
// These two values are equal when merging is complete.
if (status.sectors_allocated != status.metadata_sectors) {
- if (snapshot_status.state == "merge-complete") {
+ if (snapshot_status.state == SnapshotState::MergeCompleted) {
LOG(ERROR) << "Snapshot " << name << " is merging after being marked merge-complete.";
return UpdateState::MergeFailed;
}
@@ -693,7 +693,7 @@
// This makes it simpler to reason about the next reboot: no matter what
// part of cleanup failed, first-stage init won't try to create another
// snapshot device for this partition.
- snapshot_status.state = "merge-complete";
+ snapshot_status.state = SnapshotState::MergeCompleted;
if (!WriteSnapshotStatus(lock, name, snapshot_status)) {
return UpdateState::MergeFailed;
}
@@ -1068,7 +1068,16 @@
return false;
}
- status->state = pieces[0];
+ if (pieces[0] == "created") {
+ status->state = SnapshotState::Created;
+ } else if (pieces[0] == "merging") {
+ status->state = SnapshotState::Merging;
+ } else if (pieces[0] == "merge-completed") {
+ status->state = SnapshotState::MergeCompleted;
+ } else {
+ LOG(ERROR) << "Unrecognized state " << pieces[0] << " for snapshot: " << name;
+ }
+
if (!android::base::ParseUint(pieces[1], &status->device_size)) {
LOG(ERROR) << "Invalid device size in status line for: " << path;
return false;
@@ -1088,6 +1097,20 @@
return true;
}
+std::string SnapshotManager::to_string(SnapshotState state) {
+ switch (state) {
+ case SnapshotState::Created:
+ return "created";
+ case SnapshotState::Merging:
+ return "merging";
+ case SnapshotState::MergeCompleted:
+ return "merge-completed";
+ default:
+ LOG(ERROR) << "Unknown snapshot state: " << (int)state;
+ return "unknown";
+ }
+}
+
bool SnapshotManager::WriteSnapshotStatus(LockedFile* lock, const std::string& name,
const SnapshotStatus& status) {
// The caller must take an exclusive lock to modify snapshots.
@@ -1102,7 +1125,7 @@
}
std::vector<std::string> pieces = {
- status.state,
+ to_string(status.state),
std::to_string(status.device_size),
std::to_string(status.snapshot_size),
std::to_string(status.sectors_allocated),
diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp
index 34ea331..049f090 100644
--- a/fs_mgr/libsnapshot/snapshot_test.cpp
+++ b/fs_mgr/libsnapshot/snapshot_test.cpp
@@ -148,7 +148,7 @@
{
SnapshotManager::SnapshotStatus status;
ASSERT_TRUE(sm->ReadSnapshotStatus(lock_.get(), "test-snapshot", &status));
- ASSERT_EQ(status.state, "created");
+ ASSERT_EQ(status.state, SnapshotManager::SnapshotState::Created);
ASSERT_EQ(status.device_size, kDeviceSize);
ASSERT_EQ(status.snapshot_size, kDeviceSize);
}