From 38a5656b774903272bc21303ecf29e2895d0333a Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 11 Feb 2020 15:37:19 +0800 Subject: [PATCH 1/2] Let completeEnableRollback return a boolean (1/n) Now #completeEnableRollback returns a boolean to indicate whether the rollback object is successfully enable-completed or not. Bug: 149258640 Test: atest RollbackTest StagedRollbackTest Change-Id: Ib9803b4f6b12b3c31ea9a2d058f8be33150f0d51 --- .../rollback/RollbackManagerServiceImpl.java | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java b/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java index 9e150fd6a8b3a..950aae8e42aba 100644 --- a/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java +++ b/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java @@ -1002,11 +1002,10 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { } } - Rollback rollback = completeEnableRollback(newRollback); - if (rollback == null) { + if (!completeEnableRollback(newRollback)) { result.offer(-1); } else { - result.offer(rollback.info.getRollbackId()); + result.offer(newRollback.info.getRollbackId()); } }); @@ -1167,10 +1166,7 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { // mRollbacks here and add it back in #completeEnableRollback later. mRollbacks.remove(rollback); } - // TODO: Now #completeEnableRollback returns the same rollback object as the - // parameter on success. It would be more readable to return a boolean to indicate - // success or failure. - if (completeEnableRollback(rollback) != null) { + if (completeEnableRollback(rollback)) { makeRollbackAvailable(rollback); } } else { @@ -1190,11 +1186,11 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { /** * Add a rollback to the list of rollbacks. It does not make the rollback available yet. * - * @return the Rollback instance for a successfully enable-completed rollback, - * or null on error. + * @return {code true} if {code rollback} is successfully enable-completed, + * or {code false} otherwise. */ @WorkerThread - private Rollback completeEnableRollback(Rollback rollback) { + private boolean completeEnableRollback(Rollback rollback) { if (LOCAL_LOGV) { Slog.v(TAG, "completeEnableRollback id=" + rollback.info.getRollbackId()); } @@ -1206,7 +1202,7 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { if (!rollback.allPackagesEnabled()) { Slog.e(TAG, "Failed to enable rollback for all packages in session."); rollback.delete(mAppDataRollbackHelper); - return null; + return false; } rollback.saveRollback(); @@ -1224,7 +1220,7 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { mRollbacks.add(rollback); } - return rollback; + return true; } @WorkerThread From b18925badcb1a2f93fb981c25321dc7726144664 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 11 Feb 2020 16:20:06 +0800 Subject: [PATCH 2/2] Tweak the lifecycle of rollbacks (2/n) 1. #createNewRollbackLocked now adds newly created rollbacks to mRollbacks 2. #completeEnableRollback no longer adds the rollback object to mRollbacks. 3. #completeEnableRollback delete and remove rollbacks from mRollbacks should any error happen. Bug: 149258640 Test: atest RollbackTest StagedRollbackTest Change-Id: Ie6668cf6bd00ce9b83096383776bc75198c4f194 --- .../rollback/RollbackManagerServiceImpl.java | 45 +++++++++---------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java b/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java index 950aae8e42aba..f9981d0d9c968 100644 --- a/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java +++ b/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java @@ -802,7 +802,6 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { newRollback = getRollbackForSessionLocked(packageSession.getSessionId()); if (newRollback == null) { newRollback = createNewRollbackLocked(parentSession); - mRollbacks.add(newRollback); } } newRollback.addToken(token); @@ -1157,16 +1156,10 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { Rollback rollback; synchronized (mLock) { rollback = getRollbackForSessionLocked(sessionId); - if (rollback == null || rollback.isStaged() || !rollback.isEnabling() - || !rollback.notifySessionWithSuccess()) { - return; - } - // All child sessions finished with success. We can enable this rollback now. - // TODO: refactor #completeEnableRollback so we won't remove 'rollback' from - // mRollbacks here and add it back in #completeEnableRollback later. - mRollbacks.remove(rollback); } - if (completeEnableRollback(rollback)) { + if (rollback != null && !rollback.isStaged() && rollback.isEnabling() + && rollback.notifySessionWithSuccess() + && completeEnableRollback(rollback)) { makeRollbackAvailable(rollback); } } else { @@ -1184,7 +1177,8 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { } /** - * Add a rollback to the list of rollbacks. It does not make the rollback available yet. + * Persist a rollback as enable-completed. It does not make the rollback available yet. + * This rollback will be deleted and removed from {@link #mRollbacks} should any error happens. * * @return {code true} if {code rollback} is successfully enable-completed, * or {code false} otherwise. @@ -1201,24 +1195,22 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { // rollback for the embedded apk-in-apex, if any. if (!rollback.allPackagesEnabled()) { Slog.e(TAG, "Failed to enable rollback for all packages in session."); + mRollbacks.remove(rollback); rollback.delete(mAppDataRollbackHelper); return false; } + // Note: There is a small window of time between when + // the session has been committed by the package + // manager and when we make the rollback available + // here. Presumably the window is small enough that + // nobody will want to roll back the newly installed + // package before we make the rollback available. + // TODO: We'll lose the rollback if the + // device reboots between when the session is + // committed and this point. Revisit this after + // adding support for rollback of staged installs. rollback.saveRollback(); - synchronized (mLock) { - // Note: There is a small window of time between when - // the session has been committed by the package - // manager and when we make the rollback available - // here. Presumably the window is small enough that - // nobody will want to roll back the newly installed - // package before we make the rollback available. - // TODO: We'll lose the rollback if the - // device reboots between when the session is - // committed and this point. Revisit this after - // adding support for rollback of staged installs. - mRollbacks.add(rollback); - } return true; } @@ -1300,6 +1292,10 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { } } + /** + * Creates and returns a Rollback according to the given SessionInfo + * and adds it to {@link #mRollbacks}. + */ @WorkerThread @GuardedBy("mLock") private Rollback createNewRollbackLocked(PackageInstaller.SessionInfo parentSession) { @@ -1334,6 +1330,7 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { installerPackageName, packageSessionIds); } + mRollbacks.add(rollback); return rollback; }