commit 596466b809154900dd12680724bd8827d9e7963c
author Adriana Kobylak <anoo@us.ibm.com> Tue Feb 13 14:48:53 2018 -0600
committer Adriana Kobylak <anoo@us.ibm.com> Tue Feb 13 15:41:27 2018 -0600
tree 7316a571305c36277944b8f22a4e591dfc94add0
parent 2b2cd9f74e8e0158f93e3d4bb77346616c889698

Fix error logging

- The logging interfaces don't support std::string, so need to pass
  c-strings to the metadata fields, or the pointer address will be
  the one added to the log.
- Log the name of the file uploaded to the BMC instead of the
  manifest path when there is a manifest error. The manifest path
  is always /tmp/imgXXXX/MANIFEST which doesn't help debug.
- Check the status of the child process after waitpid returns. The
  execl call returns when there was an error executing execl, not
  if the command executed failed. This will catch when tar returns
  a non-0 return code.

Change-Id: Ia4bd2666fc6beec28dee7e821d959a336800d282
Signed-off-by: Adriana Kobylak <anoo@us.ibm.com>

image_manager.cpp

diff --git a/image_manager.cpp b/image_manager.cpp
index 69df9dd..8ff0a5c 100644
--- a/image_manager.cpp
+++ b/image_manager.cpp
@@ -56,7 +56,7 @@
     if (!fs::is_regular_file(tarFilePath))
     {
         log<level::ERR>("Error tarball does not exist",
-                        entry("FILENAME=%s", tarFilePath));
+                        entry("FILENAME=%s", tarFilePath.c_str()));
         report<ManifestFileFailure>(ManifestFail::PATH(tarFilePath.c_str()));
         return -1;
 
@@ -87,14 +87,21 @@
         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 untar file",
-                        entry("FILENAME=%s", tarFilePath));
-        report<ManifestFileFailure>(ManifestFail::PATH(manifestPath.c_str()));
+        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
     {
@@ -106,8 +113,9 @@
     // Verify the manifest file
     if (!fs::is_regular_file(manifestPath))
     {
-        log<level::ERR>("Error No manifest file");
-        report<ManifestFileFailure>(ManifestFail::PATH(manifestPath.c_str()));
+        log<level::ERR>("Error No manifest file",
+                        entry("FILENAME=%s", tarFilePath.c_str()));
+        report<ManifestFileFailure>(ManifestFail::PATH(tarFilePath.c_str()));
         return -1;
     }
 
@@ -116,7 +124,7 @@
     if (version.empty())
     {
         log<level::ERR>("Error unable to read version from manifest file");
-        report<ManifestFileFailure>(ManifestFail::PATH(manifestPath.c_str()));
+        report<ManifestFileFailure>(ManifestFail::PATH(tarFilePath.c_str()));
         return -1;
     }
 
@@ -125,7 +133,7 @@
     if (purposeString.empty())
     {
         log<level::ERR>("Error unable to read purpose from manifest file");
-        report<ManifestFileFailure>(ManifestFail::PATH(manifestPath.c_str()));
+        report<ManifestFileFailure>(ManifestFail::PATH(tarFilePath.c_str()));
         return -1;
     }
 
@@ -188,7 +196,7 @@
     else
     {
         log<level::INFO>("Software Object with the same version already exists",
-                         entry("VERSION_ID=%s", id));
+                         entry("VERSION_ID=%s", id.c_str()));
     }
     return 0;
 }
@@ -230,13 +238,13 @@
     if (extractDirPath.empty())
     {
         log<level::ERR>("Error ExtractDirPath is empty");
-        report<UnTarFailure>(UnTarFail::PATH(tarFilePath.c_str()));
+        report<UnTarFailure>(UnTarFail::PATH(extractDirPath.c_str()));
         return -1;
     }
 
     log<level::INFO>("Untaring",
-                     entry("FILENAME=%s", tarFilePath),
-                     entry("EXTRACTIONDIR=%s", extractDirPath));
+                     entry("FILENAME=%s", tarFilePath.c_str()),
+                     entry("EXTRACTIONDIR=%s", extractDirPath.c_str()));
     int status = 0;
     pid_t pid = fork();
 
@@ -246,14 +254,21 @@
         execl("/bin/tar", "tar", "-xf", tarFilePath.c_str(),
               "-C", extractDirPath.c_str(), (char*)0);
         // execl only returns on fail
-        log<level::ERR>("Failed to untar file",
-                        entry("FILENAME=%s", tarFilePath));
+        log<level::ERR>("Failed to execute untar file",
+                        entry("FILENAME=%s", tarFilePath.c_str()));
         report<UnTarFailure>(UnTarFail::PATH(tarFilePath.c_str()));
         return -1;
     }
     else if (pid > 0)
     {
         waitpid(pid, &status, 0);
+        if (WEXITSTATUS(status))
+        {
+            log<level::ERR>("Failed to untar file",
+                            entry("FILENAME=%s", tarFilePath.c_str()));
+            report<UnTarFailure>(UnTarFail::PATH(tarFilePath.c_str()));
+            return -1;
+        }
     }
     else
     {