From 5d9302860ff4b064ae09bb91683cb7fa00ba7450 Mon Sep 17 00:00:00 2001
From: Lei YU <mine260309@gmail.com>
Date: Fri, 8 Mar 2019 16:41:51 +0800
Subject: Refactor: only untar once

The code in processImage() execute "tar xf" twice, the first time to
extract the MANIFEST, and the second time to extract the whole tarball.

On a tarball without compression, it's OK. But if the tarball is
compressed, it takes much longer time to de-compress the tarball for
twice.

This commit changes the behavior by:
1. Untar the whole tarball into a temp dir;
2. Parse the manifest as before;
3. If something is wrong, remove the temp dir as before;
4. If it's valid, rename the temp dir to the valid image dir.

It also fixes an issue that it uses const_cast in mkdtemp, which is
undefined behavior because the returned const char* shall be read-only,
writing this memory could cause undefined behavior.

Partially resovles openbmc/bmcweb#60

Tested: Verify the image upload works well, and it takes much less time
        on a gzip compressed tarball.

Change-Id: I0af81acbd948e9c54d5d168c9f72e8ebbf8daebe
Signed-off-by: Lei YU <mine260309@gmail.com>
---
 image_manager.cpp | 70 ++++++++++++++-----------------------------------------
 1 file changed, 17 insertions(+), 53 deletions(-)

(limited to 'image_manager.cpp')

diff --git a/image_manager.cpp b/image_manager.cpp
index 1a530de..5b2ff49 100644
--- a/image_manager.cpp
+++ b/image_manager.cpp
@@ -44,15 +44,10 @@ struct RemovablePath
     }
     ~RemovablePath()
     {
-        if (fs::exists(path))
+        if (!path.empty())
         {
-            fs::remove_all(path);
-        }
-        else
-        {
-            // Path should exist
-            log<level::ERR>("Error removable path does not exist",
-                            entry("PATH=%s", path.c_str()));
+            std::error_code ec;
+            fs::remove_all(path, ec);
         }
     }
 };
@@ -69,9 +64,10 @@ int Manager::processImage(const std::string& tarFilePath)
     RemovablePath tarPathRemove(tarFilePath);
     fs::path tmpDirPath(std::string{IMG_UPLOAD_DIR});
     tmpDirPath /= "imageXXXXXX";
+    auto tmpDir = tmpDirPath.string();
 
-    // Need tmp dir to write MANIFEST file to.
-    if (!mkdtemp(const_cast<char*>(tmpDirPath.c_str())))
+    // Create a tmp dir to extract tarball.
+    if (!mkdtemp(tmpDir.data()))
     {
         log<level::ERR>("Error occurred during mkdtemp",
                         entry("ERRNO=%d", errno));
@@ -79,39 +75,16 @@ int Manager::processImage(const std::string& tarFilePath)
         return -1;
     }
 
-    RemovablePath tmpDirRemove(tmpDirPath);
+    tmpDirPath = tmpDir;
+    RemovablePath tmpDirToRemove(tmpDirPath);
     fs::path manifestPath = tmpDirPath;
     manifestPath /= MANIFEST_FILE_NAME;
-    int status = 0;
-    pid_t pid = fork();
 
-    // Get the MANIFEST FILE
-    if (pid == 0)
-    {
-        // child process
-        execl("/bin/tar", "tar", "-xf", tarFilePath.c_str(), MANIFEST_FILE_NAME,
-              "-C", tmpDirPath.c_str(), (char*)0);
-        // execl only returns on fail
-        log<level::ERR>("Failed to execute extract manifest",
-                        entry("FILENAME=%s", tarFilePath.c_str()));
-        report<ManifestFileFailure>(ManifestFail::PATH(tarFilePath.c_str()));
-        return -1;
-    }
-    else if (pid > 0)
-    {
-        waitpid(pid, &status, 0);
-        if (WEXITSTATUS(status))
-        {
-            log<level::ERR>("Failed to extract manifest",
-                            entry("FILENAME=%s", tarFilePath.c_str()));
-            report<UnTarFailure>(UnTarFail::PATH(tarFilePath.c_str()));
-            return -1;
-        }
-    }
-    else
+    // Untar tarball into the tmp dir
+    auto rc = unTar(tarFilePath, tmpDirPath.string());
+    if (rc < 0)
     {
-        log<level::ERR>("fork() failed.");
-        report<InternalFailure>(InternalFail::FAIL("fork"));
+        log<level::ERR>("Error occurred during untar");
         return -1;
     }
 
@@ -163,21 +136,12 @@ int Manager::processImage(const std::string& tarFilePath)
     {
         fs::remove_all(imageDirPath);
     }
-    if (mkdir(imageDirPath.c_str(), S_IRWXU) != 0)
-    {
-        log<level::ERR>("Error occurred during mkdir",
-                        entry("ERRNO=%d", errno));
-        report<InternalFailure>(InternalFail::FAIL("mkdir"));
-        return -1;
-    }
 
-    // Untar tarball
-    auto rc = unTar(tarFilePath, imageDirPath.string());
-    if (rc < 0)
-    {
-        log<level::ERR>("Error occurred during untar");
-        return -1;
-    }
+    // Rename the temp dir to image dir
+    fs::rename(tmpDirPath, imageDirPath);
+
+    // Clear the path, so it does not attemp to remove a non-existing path
+    tmpDirToRemove.path.clear();
 
     // Create Version object
     auto objPath = std::string{SOFTWARE_OBJPATH} + '/' + id;
-- 
cgit v1.2.3