Fix error attribute naming for Linux 5.0

There was a slight change to one of the error attributes as part of the
OCC driver upstreaming process. This commit also adds unit tests for the
error attributes. This required some refactoring to support the unit
tests.

Resolves openbmc/openbmc#3505

Signed-off-by: Eddie James <eajames@us.ibm.com>
Change-Id: I665b46e44b18befc8a728f7246bcda82f1f1a71c

8 files changed, 218 insertions, 29 deletions

diff --git a/occ_device.cpp b/occ_device.cpp
index b05dc09..6cd6a1f 100644
--- a/occ_device.cpp
+++ b/occ_device.cpp
@@ -12,10 +12,31 @@ namespace occ
 fs::path Device::bindPath = fs::path(OCC_HWMON_PATH) / "bind";
 fs::path Device::unBindPath = fs::path(OCC_HWMON_PATH) / "unbind";
 
+std::string Device::getPathBack(const fs::path& path)
+{
+    if (path.empty())
+        return std::string();
+
+    // Points to the last element in the path
+    auto conf = --path.end();
+
+    // The last element will be '.' if the path ends in '/'
+    // This behavior differs between filesystem and experimental::filesystem
+    // Verify there is an element before too
+    if (!conf->compare(".") && conf != path.begin())
+    {
+        return *(--conf);
+    }
+    else
+    {
+        return *conf;
+    }
+}
+
 bool Device::master() const
 {
     int master;
-    auto masterFile = fs::path(DEV_PATH) / config / "occ_master";
+    auto masterFile = devPath / "occ_master";
     std::ifstream file(masterFile, std::ios::in);
 
     if (!file)
diff --git a/occ_device.hpp b/occ_device.hpp
index 448253a..937c6dc 100644
--- a/occ_device.hpp
+++ b/occ_device.hpp
@@ -8,6 +8,7 @@
 
 #include <experimental/filesystem>
 #include <fstream>
+#include <org/open_power/OCC/Device/error.hpp>
 
 namespace open_power
 {
@@ -17,6 +18,7 @@ namespace occ
 class Manager;
 class Status;
 namespace fs = std::experimental::filesystem;
+using namespace sdbusplus::org::open_power::OCC::Device::Error;
 
 /** @class Device
  *  @brief Binds and unbinds the OCC driver upon request
@@ -34,25 +36,25 @@ class Device
     /** @brief Constructs the Device object
      *
      *  @param[in] event    - Unique ptr reference to sd_event
-     *  @param[in] name     - OCC instance name
+     *  @param[in] path     - Path to the OCC instance
      *  @param[in] manager  - OCC manager instance
      *  @param[in] callback - Optional callback on errors
      */
-    Device(EventPtr& event, const std::string& name, const Manager& manager,
+    Device(EventPtr& event, const fs::path& path, const Manager& manager,
            Status& status, std::function<void(bool)> callBack = nullptr) :
-        config(name),
-        errorFile(fs::path(config) / "occ_error"), statusObject(status),
-        error(event, errorFile, callBack),
-        presence(event, fs::path(config) / "occs_present", manager, callBack),
+        config(getPathBack(path)),
+        devPath(path), statusObject(status),
+        error(event, path / "occ_error", callBack),
+        presence(event, path / "occs_present", manager, callBack),
         throttleProcTemp(
-            event, fs::path(config) / "occ_dvfs_ot",
+            event, path / "occ_dvfs_overtemp",
             std::bind(std::mem_fn(&Device::throttleProcTempCallback), this,
                       std::placeholders::_1)),
         throttleProcPower(
-            event, fs::path(config) / "occ_dvfs_power",
+            event, path / "occ_dvfs_power",
             std::bind(std::mem_fn(&Device::throttleProcPowerCallback), this,
                       std::placeholders::_1)),
-        throttleMemTemp(event, fs::path(config) / "occ_mem_throttle",
+        throttleMemTemp(event, path / "occ_mem_throttle",
                         std::bind(std::mem_fn(&Device::throttleMemTempCallback),
                                   this, std::placeholders::_1))
     {
@@ -86,13 +88,29 @@ class Device
         return fs::exists(OCC_HWMON_PATH + config);
     }
 
-    /** @brief Starts to monitor for errors */
-    inline void addErrorWatch()
+    /** @brief Starts to monitor for errors
+     *
+     *  @param[in] poll - Indicates whether or not the error file should
+     *                    actually be polled for changes. Disabling polling is
+     *                    necessary for error files that don't support the poll
+     *                    file operation.
+     */
+    inline void addErrorWatch(bool poll = true)
     {
-        throttleProcTemp.addWatch();
-        throttleProcPower.addWatch();
-        throttleMemTemp.addWatch();
-        error.addWatch();
+        try
+        {
+            throttleProcTemp.addWatch(poll);
+        }
+        catch (const OpenFailure& e)
+        {
+            // try the old kernel version
+            throttleProcTemp.setFile(devPath / "occ_dvfs_ot");
+            throttleProcTemp.addWatch(poll);
+        }
+
+        throttleProcPower.addWatch(poll);
+        throttleMemTemp.addWatch(poll);
+        error.addWatch(poll);
     }
 
     /** @brief stops monitoring for errors */
@@ -101,7 +119,6 @@ class Device
         // we can always safely remove watch even if we don't add it
         presence.removeWatch();
         error.removeWatch();
-        error.removeWatch();
         throttleMemTemp.removeWatch();
         throttleProcPower.removeWatch();
         throttleProcTemp.removeWatch();
@@ -116,12 +133,19 @@ class Device
         }
     }
 
+    /** @brief helper function to get the last part of the path
+     *
+     * @param[in] path - Path to parse
+     * @return         - Last directory name in the path
+     */
+    static std::string getPathBack(const fs::path& path);
+
   private:
     /** @brief Config value to be used to do bind and unbind */
     const std::string config;
 
-    /** @brief This file contains 0 for success, non-zero for errors */
-    const fs::path errorFile;
+    /** @brief This directory contains the error files */
+    const fs::path devPath;
 
     /**  @brief To bind the device to the OCC driver, do:
      *
diff --git a/occ_errors.cpp b/occ_errors.cpp
index ae66a62..ab7663a 100644
--- a/occ_errors.cpp
+++ b/occ_errors.cpp
@@ -56,15 +56,18 @@ void Error::registerCallBack()
 }
 
 // Starts to watch for errors
-void Error::addWatch()
+void Error::addWatch(bool poll)
 {
     if (!watching)
     {
         // Open the file
         openFile();
 
-        // register the callback handler
-        registerCallBack();
+        if (poll)
+        {
+            // register the callback handler
+            registerCallBack();
+        }
 
         // Set we are watching the error
         watching = true;
diff --git a/occ_errors.hpp b/occ_errors.hpp
index ebacc8a..f623704 100644
--- a/occ_errors.hpp
+++ b/occ_errors.hpp
@@ -36,7 +36,7 @@ class Error
     Error(EventPtr& event, const fs::path& file,
           std::function<void(bool)> callBack = nullptr) :
         event(event),
-        file(fs::path(DEV_PATH) / file), callBack(callBack)
+        file(file), callBack(callBack)
     {
         // Nothing to do here.
     }
@@ -49,12 +49,23 @@ class Error
         }
     }
 
-    /** @brief Starts to monitor for error conditions */
-    void addWatch();
+    /** @brief Starts to monitor for error conditions
+     *
+     *  @param[in] poll - Indicates whether or not the error file should
+     *                    actually be polled for changes. Disabling polling is
+     *                    necessary for error files that don't support the poll
+     *                    file operation.
+     */
+    void addWatch(bool poll = true);
 
     /** @brief Removes error watch */
     void removeWatch();
 
+    inline void setFile(const fs::path& f)
+    {
+        file = f;
+    }
+
   private:
     /** @brief sd_event wrapped in unique_ptr */
     EventPtr& event;
@@ -95,7 +106,7 @@ class Error
     int fd = -1;
 
     /** Error file */
-    const fs::path file;
+    fs::path file;
 
     /** @brief Optional function to call on error scenario */
     std::function<void(bool)> callBack;
diff --git a/occ_status.hpp b/occ_status.hpp
index 9e6c687..859b5b0 100644
--- a/occ_status.hpp
+++ b/occ_status.hpp
@@ -65,9 +65,10 @@ class Status : public Interface
         instance(((this->path.back() - '0'))),
         device(event,
 #ifdef I2C_OCC
-               i2c_occ::getI2cDeviceName(path),
+               fs::path(DEV_PATH) / i2c_occ::getI2cDeviceName(path),
 #else
-               sysfsName + "." + std::to_string(instance + 1),
+               fs::path(DEV_PATH) /
+                   fs::path(sysfsName + "." + std::to_string(instance + 1)),
 #endif
                manager, *this,
                std::bind(std::mem_fn(&Status::deviceErrorHandler), this,
diff --git a/test/Makefile.am b/test/Makefile.am
index ff56179..0ded744 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -6,7 +6,8 @@ TESTS = $(check_PROGRAMS)
 
 utest_LDADD = $(top_builddir)/libocc_control.la -lstdc++fs
 
-utest_SOURCES = utest.cpp \
+utest_SOURCES = error_files_tests.cpp \
+                utest.cpp \
                 TestI2cOcc.cpp
 
 utest_CPPFLAGS = $(GTEST_CPPFLAGS) \
diff --git a/test/error_files_tests.cpp b/test/error_files_tests.cpp
new file mode 100644
index 0000000..532818a
--- /dev/null
+++ b/test/error_files_tests.cpp
@@ -0,0 +1,103 @@
+#include "occ_manager.hpp"
+
+#include <stdlib.h>
+
+#include <filesystem>
+#include <fstream>
+
+#include <gtest/gtest.h>
+
+constexpr auto num_error_files = 8;
+constexpr auto device = "occ-hwmon.1";
+constexpr auto error = "occ_error";
+constexpr auto errorMem = "occ_mem_throttle";
+constexpr auto errorPower = "occ_dvfs_power";
+constexpr auto errorTemp = "occ_dvfs_overtemp";
+constexpr auto legacyDevice = "occ-hwmon.2";
+constexpr auto legacyErrorTemp = "occ_dvfs_ot";
+constexpr auto noError = "0";
+
+namespace fs = std::experimental::filesystem;
+using namespace open_power::occ;
+
+class ErrorFiles : public ::testing::Test
+{
+  public:
+    ErrorFiles() :
+        bus(sdbusplus::bus::new_default()), rc(sd_event_default(&event)),
+        pEvent(event), manager(bus, pEvent),
+        status(bus, pEvent, "/dummy1", manager)
+    {
+        EXPECT_GE(rc, 0);
+        event = nullptr;
+    }
+
+    virtual void SetUp()
+    {
+        fs::path files[num_error_files];
+        char tmpDirTemplate[64];
+
+        strcpy(tmpDirTemplate, "/tmp/occXXXXXX");
+        auto path = mkdtemp(tmpDirTemplate);
+        assert(path != nullptr);
+
+        occPath = path;
+        devicePath = occPath / device;
+        legacyDevicePath = occPath / legacyDevice;
+
+        fs::create_directory(devicePath);
+        fs::create_directory(legacyDevicePath);
+
+        files[0] = devicePath / error;
+        files[1] = devicePath / errorMem;
+        files[2] = devicePath / errorPower;
+        files[3] = devicePath / errorTemp;
+        files[4] = legacyDevicePath / error;
+        files[5] = legacyDevicePath / errorMem;
+        files[6] = legacyDevicePath / errorPower;
+        files[7] = legacyDevicePath / legacyErrorTemp;
+
+        for (const fs::path& f : files)
+        {
+            auto stream = std::ofstream(f.c_str());
+
+            if (stream)
+            {
+                stream << noError;
+            }
+        }
+    }
+
+    virtual void TearDown()
+    {
+        fs::remove_all(occPath);
+    }
+
+    sdbusplus::bus::bus bus;
+    sd_event* event;
+    int rc;
+    open_power::occ::EventPtr pEvent;
+
+    Manager manager;
+    Status status;
+
+    fs::path devicePath;
+    fs::path legacyDevicePath;
+    fs::path occPath;
+};
+
+TEST_F(ErrorFiles, AddDeviceErrorWatch)
+{
+    Device occDevice(pEvent, devicePath, manager, status);
+
+    occDevice.addErrorWatch(false);
+    occDevice.removeErrorWatch();
+}
+
+TEST_F(ErrorFiles, AddLegacyDeviceErrorWatch)
+{
+    Device legacyOccDevice(pEvent, legacyDevicePath, manager, status);
+
+    legacyOccDevice.addErrorWatch(false);
+    legacyOccDevice.removeErrorWatch();
+}
diff --git a/test/utest.cpp b/test/utest.cpp
index eb9ca0b..39e5250 100644
--- a/test/utest.cpp
+++ b/test/utest.cpp
@@ -1,5 +1,6 @@
 #include "powercap.hpp"
 
+#include <experimental/filesystem>
 #include <occ_events.hpp>
 #include <occ_manager.hpp>
 
@@ -43,3 +44,27 @@ TEST_F(VerifyOccInput, PcapEnabled)
     uint32_t occInput = pcap.getOccInput(100, true);
     EXPECT_EQ(occInput, 90);
 }
+
+TEST(VerifyPathParsing, EmptyPath)
+{
+    std::experimental::filesystem::path path = "";
+    std::string parsed = Device::getPathBack(path);
+
+    EXPECT_STREQ(parsed.c_str(), "");
+}
+
+TEST(VerifyPathParsing, FilenamePath)
+{
+    std::experimental::filesystem::path path = "/test/foo.bar";
+    std::string parsed = Device::getPathBack(path);
+
+    EXPECT_STREQ(parsed.c_str(), "foo.bar");
+}
+
+TEST(VerifyPathParsing, DirectoryPath)
+{
+    std::experimental::filesystem::path path = "/test/bar/";
+    std::string parsed = Device::getPathBack(path);
+
+    EXPECT_STREQ(parsed.c_str(), "bar");
+}