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>

5 files changed, 38 insertions, 23 deletions

diff --git a/activation.cpp b/activation.cpp
index 33bb84d..de7d982 100644
--- a/activation.cpp
+++ b/activation.cpp
@@ -144,7 +144,7 @@ void Activation::deleteImageManagerObject()
     if (mapperResponseMsg.is_method_error())
     {
         log<level::ERR>("Error in Deleting image from image manager",
-                        entry("VERSIONPATH=%s", path));
+                        entry("VERSIONPATH=%s", path.c_str()));
         return;
     }
 }
diff --git a/download_manager.cpp b/download_manager.cpp
index b89ac22..1f31d32 100644
--- a/download_manager.cpp
+++ b/download_manager.cpp
@@ -52,8 +52,8 @@ void Download::downloadViaTFTP(std::string fileName,
     }
 
     log<level::INFO>("Downloading via TFTP",
-                     entry("FILENAME=%s", fileName),
-                     entry("SERVERADDRESS=%s", serverAddress));
+                     entry("FILENAME=%s", fileName.c_str()),
+                     entry("SERVERADDRESS=%s", serverAddress.c_str()));
 
     // Check if IMAGE DIR exists
     fs::path imgDirPath(IMG_UPLOAD_DIR);
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 @@ int Manager::processImage(const std::string& tarFilePath)
     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 @@ int Manager::processImage(const std::string& tarFilePath)
         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 @@ int Manager::processImage(const std::string& tarFilePath)
     // 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 @@ int Manager::processImage(const std::string& tarFilePath)
     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 @@ int Manager::processImage(const std::string& tarFilePath)
     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 @@ int Manager::processImage(const std::string& tarFilePath)
     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 @@ int Manager::unTar(const std::string& tarFilePath,
     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 @@ int Manager::unTar(const std::string& tarFilePath,
         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
     {
diff --git a/item_updater.cpp b/item_updater.cpp
index df53747..e4c9e9c 100644
--- a/item_updater.cpp
+++ b/item_updater.cpp
@@ -96,7 +96,7 @@ void ItemUpdater::createActivation(sdbusplus::message::message& msg)
     if (pos == std::string::npos)
     {
         log<level::ERR>("No version id found in object path",
-                        entry("OBJPATH=%s", path));
+                        entry("OBJPATH=%s", path.c_str()));
         return;
     }
 
@@ -171,7 +171,7 @@ void ItemUpdater::processBMCImage()
             if (!fs::is_regular_file(osRelease))
             {
                 log<level::ERR>("Failed to read osRelease",
-                                entry("FILENAME=%s", osRelease.string()));
+                                entry("FILENAME=%s", osRelease.string().c_str()));
                 ItemUpdater::erase(id);
                 continue;
             }
@@ -179,7 +179,7 @@ void ItemUpdater::processBMCImage()
             if (version.empty())
             {
                 log<level::ERR>("Failed to read version from osRelease",
-                                entry("FILENAME=%s", osRelease.string()));
+                                entry("FILENAME=%s", osRelease.string().c_str()));
                 activationState = server::Activation::Activations::Invalid;
             }
 
@@ -251,7 +251,7 @@ void ItemUpdater::processBMCImage()
                     else
                     {
                         log<level::ERR>("Unable to restore priority from file.",
-                                entry("VERSIONID=%s", id));
+                                entry("VERSIONID=%s", id.c_str()));
                     }
                 }
                 activations.find(id)->second->redundancyPriority =
@@ -629,7 +629,7 @@ void ItemUpdater::updateUbootEnvVars(const std::string& versionId)
     if (result.is_method_error())
     {
         log<level::ERR>("Failed to update u-boot env variables",
-                        entry("VERSIONID=%s", versionId));
+                        entry("VERSIONID=%s", versionId.c_str()));
     }
 }
 
diff --git a/watch.cpp b/watch.cpp
index c3aa7e9..c7c3653 100644
--- a/watch.cpp
+++ b/watch.cpp
@@ -104,7 +104,7 @@ int Watch::callback(sd_event_source* s,
             if (rc < 0)
             {
                 log<level::ERR>("Error processing image",
-                                entry("IMAGE=%s", tarballPath));
+                                entry("IMAGE=%s", tarballPath.c_str()));
             }
 
         }