From e24a2c454e25e887ffd1735f9b9a5da41e7ecc5b Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 13 Oct 2020 13:54:15 +0800 Subject: [PATCH 1/2] Assert there is only one data folder created during test (1/n) See b/170689774#comment2. If the assertion fails, it will log the content of the 'after' list which gives us more information for debugging. Bug: 170689774 Test: atest StagedRollbackTest Change-Id: Ib4ccd1be6f39c1d4c2556cbf6a06cfdf14240220 --- .../tests/rollback/host/StagedRollbackTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/RollbackTest/StagedRollbackTest/src/com/android/tests/rollback/host/StagedRollbackTest.java b/tests/RollbackTest/StagedRollbackTest/src/com/android/tests/rollback/host/StagedRollbackTest.java index 226d2d5db8200..9bf6b6f2aaa4d 100644 --- a/tests/RollbackTest/StagedRollbackTest/src/com/android/tests/rollback/host/StagedRollbackTest.java +++ b/tests/RollbackTest/StagedRollbackTest/src/com/android/tests/rollback/host/StagedRollbackTest.java @@ -18,6 +18,8 @@ package com.android.tests.rollback.host; import static com.android.tests.rollback.host.WatchdogEventLogger.watchdogEventOccurred; +import static com.google.common.truth.Truth.assertThat; + import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -272,6 +274,8 @@ public class StagedRollbackTest extends BaseHostJUnit4Test { List after = getSnapshotDirectories("/data/misc_ce/0/rollback"); // Only check directories newly created during the test after.removeAll(before); + // There should be only one /data/misc_ce/0/rollback/ created during test + assertThat(after).hasSize(1); after.forEach(dir -> assertDirectoryIsEmpty(dir)); } @@ -358,6 +362,8 @@ public class StagedRollbackTest extends BaseHostJUnit4Test { List after = getSnapshotDirectories("/data/misc/apexrollback"); // Only check directories newly created during the test after.removeAll(before); + // There should be only one /data/misc/apexrollback/ created during test + assertThat(after).hasSize(1); after.forEach(dir -> assertDirectoryIsEmpty(dir)); } @@ -405,6 +411,8 @@ public class StagedRollbackTest extends BaseHostJUnit4Test { List after = getSnapshotDirectories("/data/misc_de/0/apexrollback"); // Only check directories newly created during the test after.removeAll(before); + // There should be only one /data/misc_de/0/apexrollback/ created during test + assertThat(after).hasSize(1); after.forEach(dir -> assertDirectoryIsEmpty(dir)); } @@ -450,6 +458,8 @@ public class StagedRollbackTest extends BaseHostJUnit4Test { List after = getSnapshotDirectories("/data/misc_ce/0/apexrollback"); // Only check directories newly created during the test after.removeAll(before); + // There should be only one /data/misc_ce/0/apexrollback/ created during test + assertThat(after).hasSize(1); after.forEach(dir -> assertDirectoryIsEmpty(dir)); } @@ -509,6 +519,8 @@ public class StagedRollbackTest extends BaseHostJUnit4Test { List after = getSnapshotDirectories("/data/misc_ce/0/apexrollback"); // Only check directories newly created during the test after.removeAll(before); + // There should be only one /data/misc_ce/0/apexrollback/ created during test + assertThat(after).hasSize(1); // Expire all rollbacks and check CE snapshot directories are deleted runPhase("testCleanUp"); for (String dir : after) { From ca07fd6d7c2fc9993da904c899af7efcb57b9bb7 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 13 Oct 2020 15:04:51 +0800 Subject: [PATCH 2/2] Use AtomicFile This should guarantee the integrity of the file content and prevent the issue like b/170697897#comment2. Bug: 170697897 Test: atest StagedRollbackTest Change-Id: I4c2acb80fd9c806e31aa04d6defb480dafcf2c50 --- .../com/android/server/rollback/RollbackStore.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/services/core/java/com/android/server/rollback/RollbackStore.java b/services/core/java/com/android/server/rollback/RollbackStore.java index a4fa745c48963..44a63367dab52 100644 --- a/services/core/java/com/android/server/rollback/RollbackStore.java +++ b/services/core/java/com/android/server/rollback/RollbackStore.java @@ -25,6 +25,7 @@ import android.content.rollback.PackageRollbackInfo; import android.content.rollback.PackageRollbackInfo.RestoreInfo; import android.content.rollback.RollbackInfo; import android.os.UserHandle; +import android.util.AtomicFile; import android.util.Slog; import android.util.SparseIntArray; @@ -37,6 +38,7 @@ import org.json.JSONException; import org.json.JSONObject; import java.io.File; +import java.io.FileOutputStream; import java.io.IOException; import java.io.PrintWriter; import java.nio.file.Files; @@ -66,8 +68,6 @@ class RollbackStore { // // * XXX, YYY are the rollbackIds for the corresponding rollbacks. // * rollback.json contains all relevant metadata for the rollback. - // - // TODO: Use AtomicFile for all the .json files? private final File mRollbackDataDir; RollbackStore(File rollbackDataDir) { @@ -259,6 +259,8 @@ class RollbackStore { * Saves the given rollback to persistent storage. */ static void saveRollback(Rollback rollback) { + FileOutputStream fos = null; + AtomicFile file = new AtomicFile(new File(rollback.getBackupDir(), "rollback.json")); try { JSONObject dataJson = new JSONObject(); dataJson.put("info", rollbackInfoToJson(rollback.info)); @@ -272,11 +274,16 @@ class RollbackStore { dataJson.putOpt( "extensionVersions", extensionVersionsToJson(rollback.getExtensionVersions())); - PrintWriter pw = new PrintWriter(new File(rollback.getBackupDir(), "rollback.json")); + fos = file.startWrite(); + PrintWriter pw = new PrintWriter(fos); pw.println(dataJson.toString()); pw.close(); + file.finishWrite(fos); } catch (JSONException | IOException e) { Slog.e(TAG, "Unable to save rollback for: " + rollback.info.getRollbackId(), e); + if (fos != null) { + file.failWrite(fos); + } } }