Merge "Move temp mounting out of StageFnInstall"

3 files changed, 62 insertions, 69 deletions

diff --git a/apexd/apexd.cpp b/apexd/apexd.cpp
index 6d30eebd..093f0be2 100644
--- a/apexd/apexd.cpp
+++ b/apexd/apexd.cpp
@@ -633,9 +633,26 @@ Result<void> PrePostinstallPackages(const std::vector<ApexFile>& apexes,
     }
   }
 
-  // 2) If we found hooks, run the pre/post-install.
+  // 2) If we found hooks, temp mount if required, and run the pre/post-install.
   if (has_hooks) {
-    Result<void> install_status = (*call)(apexes);
+    std::vector<std::string> mount_points;
+    for (const ApexFile& apex : apexes) {
+      // Retrieve the mount data if the apex is already temp mounted, temp
+      // mount it otherwise.
+      std::string mount_point =
+          apexd_private::GetPackageTempMountPoint(apex.GetManifest());
+      Result<MountedApexData> mount_data =
+          apexd_private::getTempMountedApexData(apex.GetManifest().name());
+      if (!mount_data.ok()) {
+        mount_data = apexd_private::TempMountPackage(apex, mount_point);
+        if (!mount_data.ok()) {
+          return mount_data.error();
+        }
+      }
+      mount_points.push_back(mount_point);
+    }
+
+    Result<void> install_status = (*call)(apexes, mount_points);
     if (!install_status.ok()) {
       return install_status;
     }
diff --git a/apexd/apexd_prepostinstall.cpp b/apexd/apexd_prepostinstall.cpp
index 9a58aa6a..7bf9cfc2 100644
--- a/apexd/apexd_prepostinstall.cpp
+++ b/apexd/apexd_prepostinstall.cpp
@@ -59,12 +59,9 @@ void CloseSTDDescriptors() {
   close(STDERR_FILENO);
 }
 
-// Instead of temp mounting inside this fuction, we can make a caller do it.
-// This will align with the plan of extending temp mounting to provide a
-// way to run additional pre-reboot verification of an APEX.
-// TODO(b/158470432): pass mount points instead of apex files.
 template <typename Fn>
-Result<void> StageFnInstall(const std::vector<ApexFile>& apexes, Fn fn,
+Result<void> StageFnInstall(const std::vector<ApexFile>& apexes,
+                            const std::vector<std::string>& mount_points, Fn fn,
                             const char* arg, const char* name) {
   // TODO(b/158470023): consider supporting a session with more than one
   //   pre-install hook.
@@ -80,60 +77,14 @@ Result<void> StageFnInstall(const std::vector<ApexFile>& apexes, Fn fn,
   CHECK(hook_idx != -1);
   LOG(VERBOSE) << name << " for " << apexes[hook_idx].GetPath();
 
-  std::vector<MountedApexData> mounted_apexes;
-  std::vector<std::string> activation_dirs;
-  auto preinstall_guard = android::base::make_scope_guard([&]() {
-    for (const std::string& active_point : activation_dirs) {
-      if (0 != rmdir(active_point.c_str())) {
-        PLOG(ERROR) << "Could not delete temporary active point "
-                    << active_point;
-      }
-    }
-  });
-
-  for (const ApexFile& apex : apexes) {
-    // 1) Retrieve the mount data if the apex is already temp mounted, temp
-    // mount it otherwise.
-    Result<MountedApexData> mount_data =
-        apexd_private::getTempMountedApexData(apex.GetManifest().name());
-    if (!mount_data.ok()) {
-      std::string mount_point =
-          apexd_private::GetPackageTempMountPoint(apex.GetManifest());
-      mount_data = apexd_private::TempMountPackage(apex, mount_point);
-      if (!mount_data.ok()) {
-        return mount_data.error();
-      }
-    }
-    mounted_apexes.push_back(std::move(*mount_data));
-
-    // Given the fact, that we only allow updates of existing APEXes, all the
-    // activation points will always be already created. Only scenario, when it
-    // won't be the case might be apexservice_test. But even then, it might be
-    // safer to move active_point creation logic to run after unshare.
-    // TODO(b/158470432): maybe move creation of activation points inside
-    //   RunFnInstall?
-    // 2) Ensure there is an activation point, and we will clean it up.
-    std::string active_point =
-        apexd_private::GetActiveMountPoint(apex.GetManifest());
-    if (0 == mkdir(active_point.c_str(), kMkdirMode)) {
-      activation_dirs.emplace_back(std::move(active_point));
-    } else {
-      int saved_errno = errno;
-      if (saved_errno != EEXIST) {
-        return Error() << "Unable to create mount point" << active_point << ": "
-                       << strerror(saved_errno);
-      }
-    }
-  }
-
-  // 3) Create invocation args.
+  // Create invocation args.
   std::vector<std::string> args{
       "/system/bin/apexd", arg,
-      mounted_apexes[hook_idx].mount_point,  // Make the APEX with hook first.
+      mount_points[hook_idx]  // Make the APEX with hook first.
   };
-  for (size_t i = 0; i < mounted_apexes.size(); i++) {
+  for (size_t i = 0; i < mount_points.size(); i++) {
     if ((int)i != hook_idx) {
-      args.push_back(mounted_apexes[i].mount_point);
+      args.push_back(mount_points[i]);
     }
   }
 
@@ -142,6 +93,16 @@ Result<void> StageFnInstall(const std::vector<ApexFile>& apexes, Fn fn,
 
 template <typename Fn>
 int RunFnInstall(char** in_argv, Fn fn, const char* name) {
+  std::vector<std::string> activation_dirs;
+  auto preinstall_guard = android::base::make_scope_guard([&]() {
+    for (const std::string& active_point : activation_dirs) {
+      if (0 != rmdir(active_point.c_str())) {
+        PLOG(ERROR) << "Could not delete temporary active point "
+                    << active_point;
+      }
+    }
+  });
+
   // 1) Unshare.
   if (unshare(CLONE_NEWNS) != 0) {
     PLOG(ERROR) << "Failed to unshare() for apex " << name;
@@ -157,7 +118,8 @@ int RunFnInstall(char** in_argv, Fn fn, const char* name) {
 
   std::string hook_path;
   {
-    auto bind_fn = [&fn, name](const std::string& mount_point) {
+    auto bind_fn = [&fn, name,
+                    activation_dirs](const std::string& mount_point) mutable {
       std::string hook;
       std::string active_point;
       {
@@ -180,6 +142,14 @@ int RunFnInstall(char** in_argv, Fn fn, const char* name) {
         const auto& manifest = *manifest_or;
         hook = (manifest.*fn)();
         active_point = apexd_private::GetActiveMountPoint(manifest);
+        // Ensure there is an activation point. If not, create one and delete
+        // later.
+        if (0 == mkdir(active_point.c_str(), kMkdirMode)) {
+          activation_dirs.push_back(active_point);
+        } else if (errno != EEXIST) {
+          PLOG(ERROR) << "Unable to create mount point " << active_point;
+          _exit(205);
+        }
       }
 
       // 3) Activate the new apex.
@@ -230,17 +200,19 @@ int RunFnInstall(char** in_argv, Fn fn, const char* name) {
 
 }  // namespace
 
-Result<void> StagePreInstall(const std::vector<ApexFile>& apexes) {
-  return StageFnInstall(apexes, &ApexManifest::preinstallhook, "--pre-install",
-                        "pre-install");
+Result<void> StagePreInstall(const std::vector<ApexFile>& apexes,
+                             const std::vector<std::string>& mount_points) {
+  return StageFnInstall(apexes, mount_points, &ApexManifest::preinstallhook,
+                        "--pre-install", "pre-install");
 }
 
 int RunPreInstall(char** in_argv) {
   return RunFnInstall(in_argv, &ApexManifest::preinstallhook, "pre-install");
 }
 
-Result<void> StagePostInstall(const std::vector<ApexFile>& apexes) {
-  return StageFnInstall(apexes, &ApexManifest::postinstallhook,
+Result<void> StagePostInstall(const std::vector<ApexFile>& apexes,
+                              const std::vector<std::string>& mount_points) {
+  return StageFnInstall(apexes, mount_points, &ApexManifest::postinstallhook,
                         "--post-install", "post-install");
 }
 
diff --git a/apexd/apexd_prepostinstall.h b/apexd/apexd_prepostinstall.h
index 66cd2f57..65125bae 100644
--- a/apexd/apexd_prepostinstall.h
+++ b/apexd/apexd_prepostinstall.h
@@ -27,16 +27,20 @@ namespace apex {
 
 class ApexFile;
 
-// Temp mounts given apexes and then forks into:
-// apexd --pre-install <mount-point-of-apex-with-hook> [<other-mount-points>]
+// Forks into: apexd --pre-install <mount-point-of-apex-with-hook>
+// [<other-mount-points>] The caller must pass the temp mount point for each
+// apex file.
 android::base::Result<void> StagePreInstall(
-    const std::vector<ApexFile>& apexes);
+    const std::vector<ApexFile>& apexes,
+    const std::vector<std::string>& mount_points);
 int RunPreInstall(char** argv);
 
-// Temp mounts given apexes and then forks into:
-// apexd --post-install <mount-point-of-apex-with-hook> [<other-mount-points>]
+// Forks into: apexd --post-install <mount-point-of-apex-with-hook>
+// [<other-mount-points>] The caller must pass the temp mount point for each
+// apex file.
 android::base::Result<void> StagePostInstall(
-    const std::vector<ApexFile>& apexes);
+    const std::vector<ApexFile>& apexes,
+    const std::vector<std::string>& mount_points);
 int RunPostInstall(char** argv);
 
 }  // namespace apex