Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f27c063b authored by David Anderson's avatar David Anderson
Browse files

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
parent 3c0fea92

fs_mgr/libsnapshot/include/libsnapshot/snapshot.h

+4 −1
Original line number Diff line number Diff line
@@ -219,9 +219,12 @@ class SnapshotManager final { 
    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.

fs_mgr/libsnapshot/snapshot.cpp

+33 −10
Original line number Diff line number Diff line
@@ -165,7 +165,7 @@ bool SnapshotManager::CreateSnapshot(LockedFile* lock, const std::string& name, 
    // 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 @@ bool SnapshotManager::MapSnapshot(LockedFile* lock, const std::string& name, 
    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 @@ bool SnapshotManager::SwitchSnapshotToMerge(LockedFile* lock, const std::string& 
    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 @@ bool SnapshotManager::SwitchSnapshotToMerge(LockedFile* lock, const std::string& 
        return false;

    }



    status.state = "merging";

    status.state = SnapshotState::Merging;



    DmTargetSnapshot::Status dm_status;

    if (!QuerySnapshotStatus(dm_name, nullptr, &dm_status)) {
@@ -657,7 +657,7 @@ UpdateState SnapshotManager::CheckTargetMergeState(LockedFile* lock, const std:: 
    // 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 @@ UpdateState SnapshotManager::CheckTargetMergeState(LockedFile* lock, const std:: 


    // 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 @@ UpdateState SnapshotManager::CheckTargetMergeState(LockedFile* lock, const std:: 
    // 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 @@ bool SnapshotManager::ReadSnapshotStatus(LockedFile* lock, const std::string& na 
        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 @@ bool SnapshotManager::ReadSnapshotStatus(LockedFile* lock, const std::string& na 
    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 @@ bool SnapshotManager::WriteSnapshotStatus(LockedFile* lock, const std::string& n 
    }



    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),

fs_mgr/libsnapshot/snapshot_test.cpp

+1 −1
Original line number Diff line number Diff line
@@ -148,7 +148,7 @@ TEST_F(SnapshotTest, CreateSnapshot) { 
    {

        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);

    }