From 529a37d3cb69dd2d8012c9cfce1d27d49ec727d1 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Wed, 22 Apr 2020 00:38:21 -0700 Subject: [PATCH 1/3] [incfs] Fix bad path::join() path preparation Bug: 151241369 Test: atest IncrementalServiceTest Change-Id: I6e7655634de722c46e96e66b58c0c4c8e5098874 --- services/incremental/path.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/incremental/path.cpp b/services/incremental/path.cpp index 0d86f2a984d63..338659d40b468 100644 --- a/services/incremental/path.cpp +++ b/services/incremental/path.cpp @@ -44,7 +44,7 @@ bool PathLess::operator()(std::string_view l, std::string_view r) const { PathCharsLess()); } -static void preparePathComponent(std::string_view path, bool trimFront) { +static void preparePathComponent(std::string_view& path, bool trimFront) { if (trimFront) { while (!path.empty() && path.front() == '/') { path.remove_prefix(1); From 683f2b1acce54441f3f34def0be5bd3255fb9e71 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Wed, 22 Apr 2020 00:36:34 -0700 Subject: [PATCH 2/3] [incfs] Notify IncrementalService about system startup later It needs system to be ready to bind to services, which is much later in the boot process Bug: 151241369 Test: boot up Android, make sure no error messages from IncrementalService trying to bind a DataLoader when the OS can't bind services yet Change-Id: Ibcef3f405965ee07c5b8adbb41f068be734bde9e --- services/java/com/android/server/SystemServer.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/java/com/android/server/SystemServer.java b/services/java/com/android/server/SystemServer.java index 289d79dad3676..0fc333f4b38c2 100644 --- a/services/java/com/android/server/SystemServer.java +++ b/services/java/com/android/server/SystemServer.java @@ -2174,12 +2174,6 @@ public final class SystemServer { mPackageManagerService.systemReady(); t.traceEnd(); - if (mIncrementalServiceHandle != 0) { - t.traceBegin("MakeIncrementalServiceReady"); - setIncrementalServiceSystemReady(mIncrementalServiceHandle); - t.traceEnd(); - } - t.traceBegin("MakeDisplayManagerServiceReady"); try { // TODO: use boot phase and communicate these flags some other way @@ -2449,6 +2443,12 @@ public final class SystemServer { reportWtf("Notifying incident daemon running", e); } t.traceEnd(); + + if (mIncrementalServiceHandle != 0) { + t.traceBegin("MakeIncrementalServiceReady"); + setIncrementalServiceSystemReady(mIncrementalServiceHandle); + t.traceEnd(); + } }, t); t.traceEnd(); // startOtherServices From 360bbdc7203c851288d8e09d2eee2829975b210f Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Sat, 18 Apr 2020 21:03:09 -0700 Subject: [PATCH 3/3] [incfs] Cleanup storage creation and don't use temp directory Bug: 153704006 Test: atest PackageManagerShellCommandTest \ PackageManagerShellCommandIncrementalTest \ IncrementalServiceTest Change-Id: I8942a2db766cfdb7c119112cc9b58ee0287f1767 --- .../incremental/IncrementalFileStorages.java | 136 +++++++----------- 1 file changed, 52 insertions(+), 84 deletions(-) diff --git a/core/java/android/os/incremental/IncrementalFileStorages.java b/core/java/android/os/incremental/IncrementalFileStorages.java index 251995a140902..321dc9e2246e1 100644 --- a/core/java/android/os/incremental/IncrementalFileStorages.java +++ b/core/java/android/os/incremental/IncrementalFileStorages.java @@ -38,16 +38,10 @@ import android.content.pm.DataLoaderParams; import android.content.pm.IDataLoaderStatusListener; import android.content.pm.InstallationFileParcel; import android.text.TextUtils; -import android.util.Slog; import java.io.File; import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; import java.util.List; -import java.util.Objects; -import java.util.Random; /** * This class manages storage instances used during a package installation session. @@ -56,13 +50,9 @@ import java.util.Random; public final class IncrementalFileStorages { private static final String TAG = "IncrementalFileStorages"; - private static final String TMP_DIR_ROOT = "/data/incremental/tmp"; - private static final Random TMP_DIR_RANDOM = new Random(); - + private @NonNull final IncrementalManager mIncrementalManager; + private @NonNull final File mStageDir; private @Nullable IncrementalStorage mDefaultStorage; - private @Nullable String mDefaultDir; - private @NonNull IncrementalManager mIncrementalManager; - private @NonNull File mStageDir; /** * Set up files and directories used in an installation session. Only used by Incremental. @@ -85,72 +75,63 @@ public final class IncrementalFileStorages { throw new IOException("Failed to obtain incrementalManager."); } - IncrementalFileStorages result = null; - try { - result = new IncrementalFileStorages(stageDir, incrementalManager, dataLoaderParams, - dataLoaderStatusListener); - - if (!addedFiles.isEmpty()) { - result.mDefaultStorage.bind(stageDir.getAbsolutePath()); - } - - for (InstallationFileParcel file : addedFiles) { - if (file.location == LOCATION_DATA_APP) { - try { - result.addApkFile(file); - } catch (IOException e) { - // TODO(b/146080380): add incremental-specific error code - throw new IOException( - "Failed to add file to IncFS: " + file.name + ", reason: ", e); - } - } else { - throw new IOException("Unknown file location: " + file.location); + final IncrementalFileStorages result = + new IncrementalFileStorages(stageDir, incrementalManager, dataLoaderParams, + dataLoaderStatusListener); + for (InstallationFileParcel file : addedFiles) { + if (file.location == LOCATION_DATA_APP) { + try { + result.addApkFile(file); + } catch (IOException e) { + // TODO(b/146080380): add incremental-specific error code + throw new IOException( + "Failed to add file to IncFS: " + file.name + ", reason: ", e); } + } else { + throw new IOException("Unknown file location: " + file.location); } - - // TODO(b/146080380): remove 5 secs wait in startLoading - if (!result.mDefaultStorage.startLoading()) { - // TODO(b/146080380): add incremental-specific error code - throw new IOException("Failed to start loading data for Incremental installation."); - } - - return result; - } catch (IOException e) { - Slog.e(TAG, "Failed to initialize Incremental file storages. Cleaning up...", e); - if (result != null) { - result.cleanUp(); - } - throw e; } + + if (!result.mDefaultStorage.startLoading()) { + // TODO(b/146080380): add incremental-specific error code + throw new IOException("Failed to start loading data for Incremental installation."); + } + + return result; } private IncrementalFileStorages(@NonNull File stageDir, @NonNull IncrementalManager incrementalManager, @NonNull DataLoaderParams dataLoaderParams, @Nullable IDataLoaderStatusListener dataLoaderStatusListener) throws IOException { - mStageDir = stageDir; - mIncrementalManager = incrementalManager; - if (dataLoaderParams.getComponentName().getPackageName().equals("local")) { - final String incrementalPath = dataLoaderParams.getArguments(); - mDefaultDir = incrementalPath; - if (TextUtils.isEmpty(mDefaultDir)) { - throw new IOException("Failed to create storage: incrementalPath is empty"); + try { + mStageDir = stageDir; + mIncrementalManager = incrementalManager; + if (dataLoaderParams.getComponentName().getPackageName().equals("local")) { + final String incrementalPath = dataLoaderParams.getArguments(); + if (TextUtils.isEmpty(incrementalPath)) { + throw new IOException("Failed to create storage: incrementalPath is empty"); + } + mDefaultStorage = mIncrementalManager.openStorage(incrementalPath); + if (mDefaultStorage == null) { + throw new IOException( + "Couldn't open incremental storage at " + incrementalPath); + } + mDefaultStorage.bind(stageDir.getAbsolutePath()); + } else { + mDefaultStorage = mIncrementalManager.createStorage(stageDir.getAbsolutePath(), + dataLoaderParams, + dataLoaderStatusListener, + IncrementalManager.CREATE_MODE_CREATE + | IncrementalManager.CREATE_MODE_TEMPORARY_BIND, false); + if (mDefaultStorage == null) { + throw new IOException( + "Couldn't create incremental storage at " + stageDir); + } } - mDefaultStorage = mIncrementalManager.openStorage(incrementalPath); - } else { - mDefaultDir = getTempDir(); - if (mDefaultDir == null) { - throw new IOException("Failed to create storage: tempDir is empty"); - } - mDefaultStorage = mIncrementalManager.createStorage(mDefaultDir, - dataLoaderParams, - dataLoaderStatusListener, - IncrementalManager.CREATE_MODE_CREATE - | IncrementalManager.CREATE_MODE_TEMPORARY_BIND, false); - } - - if (mDefaultStorage == null) { - throw new IOException("Failed to create storage"); + } catch (IOException e) { + cleanUp(); + throw e; } } @@ -167,27 +148,14 @@ public final class IncrementalFileStorages { * TODO(b/136132412): make sure unnecessary binds are removed but useful storages are kept */ public void cleanUp() { - Objects.requireNonNull(mDefaultStorage); + if (mDefaultStorage == null) { + return; + } try { - mDefaultStorage.unBind(mDefaultDir); mDefaultStorage.unBind(mStageDir.getAbsolutePath()); } catch (IOException ignored) { } - - mDefaultDir = null; mDefaultStorage = null; } - - private static String getTempDir() { - final Path tmpDir = Paths.get(TMP_DIR_ROOT, - String.valueOf(TMP_DIR_RANDOM.nextInt(Integer.MAX_VALUE - 1))); - try { - Files.createDirectories(tmpDir); - } catch (Exception ex) { - Slog.e(TAG, "Failed to create dir", ex); - return null; - } - return tmpDir.toAbsolutePath().toString(); - } }