Enhance PSU tracing support - add device access info
Move file names for file errors to the journal message. Enhance usablility.
Logging messages for file errors contain the file names inside structured
entries rather than directly in the log message. This makes viewing errors
more difficult than need be. Before this change, the journal report needed to
be run with '-o verbose' to see the file names.
Tested: Ran unit tests. All successful
Tested:
* Changes to phosphor-power-supply/main.cpp were tested by running with the
.json configuration file missing in either and both default locations.
* Change to loadJSONFromFile() in utility.cpp for an invalid file was tested by
giving invalid "psu_config.json" file to main program.
* Empty file message was tested by changing main() to allow loadJSONFromFile()
to be called it when the file did not exist.
* Changes to other files were tested by running phosphor-power initialization
and manipulating the code to force execution of various error paths.
* Methods that are changed but were not directly called by phosphor-power
initialization:
PMBus::readBit(), PMBus::readBinary() and PMBus::write().
The changed log messages in these were tested by adding calls to them from
PMBus::read() and then changing code to force execution through paths that
created the log messages.
Signed-off-by: Jay Meyer <jaymeyer@us.ibm.com>
Change-Id: I1f0c01716495ac7022d5234e9a8dfdab8195c186
diff --git a/phosphor-power-supply/main.cpp b/phosphor-power-supply/main.cpp
index 542be4b..8960a53 100644
--- a/phosphor-power-supply/main.cpp
+++ b/phosphor-power-supply/main.cpp
@@ -53,8 +53,10 @@
if (!std::filesystem::exists(configfile))
{
- log<level::ERR>("Configuration file does not exist",
- entry("FILENAME=%s", configfile.c_str()));
+ log<level::ERR>((std::string("Configuration file does not exist: "
+ "FILENAME=") +
+ configfile)
+ .c_str());
return -1;
}
diff --git a/phosphor-power-supply/power_supply.cpp b/phosphor-power-supply/power_supply.cpp
index 2b77bcd..7fffd70 100644
--- a/phosphor-power-supply/power_supply.cpp
+++ b/phosphor-power-supply/power_supply.cpp
@@ -321,7 +321,9 @@
}
catch (std::exception& e)
{
- log<level::ERR>(e.what(), entry("PATH=%s", inventoryPath.c_str()));
+ log<level::ERR>(
+ std::string(e.what() + std::string(" PATH=") + inventoryPath)
+ .c_str());
}
#endif
}
diff --git a/pmbus.cpp b/pmbus.cpp
index b2fe2fd..3440667 100644
--- a/pmbus.cpp
+++ b/pmbus.cpp
@@ -100,8 +100,10 @@
}
catch (std::exception& e)
{
- log<level::ERR>("Unable to read PMBus device name",
- entry("PATH=%s", path.c_str()));
+ log<level::ERR>((std::string("Unable to read PMBus device name "
+ "PATH=") +
+ path.string())
+ .c_str());
}
return name;
@@ -136,9 +138,10 @@
if (*err)
{
- log<level::ERR>("Invalid character in sysfs file",
- entry("FILE=%s", path.c_str()),
- entry("CONTENTS=%s", val.c_str()));
+ log<level::ERR>((std::string("Invalid character in sysfs file"
+ " FILE=") +
+ path.string() + std::string(" CONTENTS=") + val)
+ .c_str());
// Catch below and handle as a read failure
elog<InternalFailure>();
@@ -148,8 +151,11 @@
{
auto rc = errno;
- log<level::ERR>("Failed to read sysfs file",
- entry("FILENAME=%s", path.c_str()));
+ log<level::ERR>((std::string("Failed to read sysfs file "
+ "errno=") +
+ std::to_string(rc) + std::string(" FILENAME=") +
+ path.string())
+ .c_str());
using metadata = xyz::openbmc_project::Common::Device::ReadFailure;
@@ -186,8 +192,10 @@
catch (std::exception& e)
{
auto rc = errno;
- log<level::ERR>("Failed to read sysfs file",
- entry("FILENAME=%s", path.c_str()));
+ log<level::ERR>((std::string("Failed to read sysfs file "
+ "errno=") +
+ std::to_string(rc) + " FILENAME=" + path.string())
+ .c_str());
using metadata = xyz::openbmc_project::Common::Device::ReadFailure;
@@ -217,8 +225,10 @@
catch (std::exception& e)
{
auto rc = errno;
- log<level::ERR>("Failed to read sysfs file",
- entry("FILENAME=%s", path.c_str()));
+ log<level::ERR>((std::string("Failed to read sysfs file "
+ "errno=") +
+ std::to_string(rc) + " FILENAME=" + path.string())
+ .c_str());
using metadata = xyz::openbmc_project::Common::Device::ReadFailure;
@@ -256,9 +266,11 @@
else if (ferror(file.get()))
{
auto rc = errno;
- log<level::ERR>("Failed to read sysfs file",
- entry("FILENAME=%s", path.c_str()));
-
+ log<level::ERR>((std::string("Failed to read sysfs file "
+ "errno=") +
+ std::to_string(rc) +
+ " FILENAME=" + path.string())
+ .c_str());
using metadata =
xyz::openbmc_project::Common::Device::ReadFailure;
@@ -291,9 +303,10 @@
catch (const std::exception& e)
{
auto rc = errno;
-
- log<level::ERR>("Failed to write sysfs file",
- entry("FILENAME=%s", path.c_str()));
+ log<level::ERR>((std::string("Failed to write sysfs file "
+ "errno=") +
+ std::to_string(rc) + " FILENAME=" + path.string())
+ .c_str());
using metadata = xyz::openbmc_project::Common::Device::WriteFailure;
@@ -318,16 +331,20 @@
{
// I need to specify binary mode when I construct the ofstream
file.open(path, std::ios::out | std::ios_base::binary);
- log<level::DEBUG>("Write data to sysfs file",
- entry("FILENAME=%s", path.c_str()));
+ log<level::DEBUG>(std::string("Write data to sysfs file "
+ "FILENAME=" +
+ path.string())
+ .c_str());
file.write(reinterpret_cast<const char*>(&data[0]), data.size());
}
catch (const std::exception& e)
{
auto rc = errno;
-
- log<level::ERR>("Failed to write binary data to sysfs file",
- entry("FILENAME=%s", path.c_str()));
+ log<level::ERR>(
+ (std::string("Failed to write binary data to sysfs file "
+ "errno=") +
+ std::to_string(rc) + " FILENAME=" + path.string())
+ .c_str());
using metadata = xyz::openbmc_project::Common::Device::WriteFailure;
@@ -364,9 +381,11 @@
// and let accesses fail later
if (hwmonDir.empty())
{
- log<level::INFO>("Unable to find hwmon directory "
- "in device base path",
- entry("DEVICE_PATH=%s", basePath.c_str()));
+ log<level::INFO>(std::string("Unable to find hwmon directory "
+ "in device base path"
+ " DEVICE_PATH=" +
+ basePath.string())
+ .c_str());
}
}
diff --git a/utility.cpp b/utility.cpp
index a55ffe8..7fdd6b2 100644
--- a/utility.cpp
+++ b/utility.cpp
@@ -51,9 +51,11 @@
{
if (logError)
{
- log<level::ERR>("Error in mapper response for getting service name",
- entry("PATH=%s", path.c_str()),
- entry("INTERFACE=%s", interface.c_str()));
+ log<level::ERR>(
+ std::string("Error in mapper response for getting service name "
+ "PATH=" +
+ path + " INTERFACE=" + interface)
+ .c_str());
}
return std::string{};
}
@@ -66,13 +68,19 @@
std::ifstream ifs(path);
if (!ifs.good())
{
- log<level::ERR>("Unable to open file", entry("PATH=%s", path));
+ log<level::ERR>(std::string("Unable to open file "
+ "PATH=" +
+ std::string(path))
+ .c_str());
return nullptr;
}
auto data = json::parse(ifs, nullptr, false);
if (data.is_discarded())
{
- log<level::ERR>("Failed to parse json", entry("PATH=%s", path));
+ log<level::ERR>(std::string("Failed to parse json "
+ "PATH=" +
+ std::string(path))
+ .c_str());
return nullptr;
}
return data;