summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorLei YU <mine260309@gmail.com>2019-03-08 16:41:51 +0800
committerLei YU <mine260309@gmail.com>2019-04-02 10:32:49 +0800
commit5d9302860ff4b064ae09bb91683cb7fa00ba7450 (patch)
treef20a0f3293ecdc13f61b1bf2e5a90f31b6658e7d
parentab8231c66364286a10b0da6d64d9742752323b4d (diff)
downloadphosphor-bmc-code-mgmt-master.zip
phosphor-bmc-code-mgmt-master.tar.gz
Refactor: only untar onceHEADmaster
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>
-rw-r--r--image_manager.cpp70
1 files changed, 17 insertions, 53 deletions
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;
OpenPOWER on IntegriCloud