Fix sensor override

c1d019a Sensor Optimization

Recently changed the way Ids were calculated in the sensor subsystem.
Unfortunately, it wasn't clear to the author that this would effect the
sensor override system, which relies on matching up a member ID with a
dbus path, and was broken by this change.

This commit breaks out the code to calculate the type and name from a
given URI segment into a helper method.

Tested: Inspection only.  Very few systems support this feature.  Code appears more correct than previously, which is known broken, so the lack of testing here seems reasonable.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I9aa8099a947a36b5ce914bc07ae60f1ebf0d209b

meson.build

@@ -367,6 +367,7 @@ srcfiles_unittest = files(
   'test/redfish-core/include/utils/stl_utils_test.cpp',
   'test/redfish-core/include/utils/time_utils_test.cpp',
   'test/redfish-core/lib/chassis_test.cpp',
+  'test/redfish-core/lib/sensors_test.cpp',
   'test/redfish-core/lib/log_services_dump_test.cpp',
   'test/redfish-core/lib/service_root_test.cpp',
   'test/redfish-core/lib/thermal_subsystem_test.cpp',

redfish-core/lib/sensors.hpp

@@ -21,6 +21,7 @@
 #include <boost/algorithm/string/classification.hpp>
 #include <boost/algorithm/string/find.hpp>
 #include <boost/algorithm/string/predicate.hpp>
+#include <boost/algorithm/string/replace.hpp>
 #include <boost/algorithm/string/split.hpp>
 #include <boost/range/algorithm/replace_copy_if.hpp>
 #include <dbus_singleton.hpp>
@@ -732,7 +733,7 @@ inline void objectPropertiesToJson(
 {
     if (chassisSubNode == sensors::node::sensors)
     {
-        std::string subNodeEscaped(chassisSubNode);
+        std::string subNodeEscaped(sensorType);
         subNodeEscaped.erase(
             std::remove(subNodeEscaped.begin(), subNodeEscaped.end(), '_'),
             subNodeEscaped.end());
@@ -2578,6 +2579,24 @@ inline bool
     return false;
 }
 
+inline std::pair<std::string, std::string>
+    splitSensorNameAndType(std::string_view sensorId)
+{
+    size_t index = sensorId.find('_');
+    if (index == std::string::npos)
+    {
+        return std::make_pair<std::string, std::string>("", "");
+    }
+    std::string sensorType{sensorId.substr(0, index)};
+    std::string sensorName{sensorId.substr(index + 1)};
+    // fan_pwm and fan_tach need special handling
+    if (sensorType == "fantach" || sensorType == "fanpwm")
+    {
+        sensorType.insert(3, 1, '_');
+    }
+    return std::make_pair(sensorType, sensorName);
+}
+
 /**
  * @brief Entry point for overriding sensor values of given sensor
  *
@@ -2634,8 +2653,10 @@ inline void setSensorsOverride(
         for (const auto& item : overrideMap)
         {
             const auto& sensor = item.first;
-            if (!findSensorNameUsingSensorPath(sensor, *sensorsList,
-                                               *sensorNames))
+            std::pair<std::string, std::string> sensorNameType =
+                splitSensorNameAndType(sensor);
+            if (!findSensorNameUsingSensorPath(sensorNameType.second,
+                                               *sensorsList, *sensorNames))
             {
                 BMCWEB_LOG_INFO << "Unable to find memberId " << item.first;
                 messages::resourceNotFound(sensorAsyncResp->asyncResp->res,
@@ -2876,33 +2897,28 @@ inline void handleSensorGet(App& app, const crow::Request& req,
     {
         return;
     }
-    size_t index = sensorId.find('_');
-    if (index == std::string::npos)
+    std::pair<std::string, std::string> nameType =
+        splitSensorNameAndType(sensorId);
+    if (nameType.first.empty() || nameType.second.empty())
     {
         messages::resourceNotFound(asyncResp->res, sensorId, "Sensor");
         return;
     }
+
     asyncResp->res.jsonValue["@odata.id"] = crow::utility::urlFromPieces(
         "redfish", "v1", "Chassis", chassisId, "Sensors", sensorId);
-    std::string sensorType = sensorId.substr(0, index);
-    std::string sensorName = sensorId.substr(index + 1);
-    // fan_pwm and fan_tach need special handling
-    if (sensorType == "fantach" || sensorType == "fanpwm")
-    {
-        sensorType.insert(3, 1, '_');
-    }
 
     BMCWEB_LOG_DEBUG << "Sensor doGet enter";
 
     const std::array<const char*, 1> interfaces = {
         "xyz.openbmc_project.Sensor.Value"};
-    std::string sensorPath =
-        "/xyz/openbmc_project/sensors/" + sensorType + '/' + sensorName;
+    std::string sensorPath = "/xyz/openbmc_project/sensors/" + nameType.first +
+                             '/' + nameType.second;
     // Get a list of all of the sensors that implement Sensor.Value
     // and get the path and service name associated with the sensor
     crow::connections::systemBus->async_method_call(
-        [asyncResp, sensorPath,
-         sensorName](const boost::system::error_code ec,
+        [asyncResp,
+         sensorPath](const boost::system::error_code ec,
                      const ::dbus::utility::MapperGetObject& subtree) {
         BMCWEB_LOG_DEBUG << "respHandler1 enter";
         if (ec)

test/redfish-core/lib/sensors_test.cpp

@@ -0,0 +1,36 @@
+#include "sensors.hpp"
+
+#include <gmock/gmock.h> // IWYU pragma: keep
+#include <gtest/gtest.h> // IWYU pragma: keep
+
+// IWYU pragma: no_include <gtest/gtest-message.h>
+// IWYU pragma: no_include <gtest/gtest-test-part.h>
+// IWYU pragma: no_include "gtest/gtest_pred_impl.h"
+// IWYU pragma: no_include <gmock/gmock-matchers.h>
+// IWYU pragma: no_include <gtest/gtest-matchers.h>
+
+namespace redfish
+{
+namespace
+{
+
+TEST(SplitSensorNameAndType, Type)
+{
+    EXPECT_EQ(splitSensorNameAndType("fantach_foo_1").first, "fan_tach");
+    EXPECT_EQ(splitSensorNameAndType("temperature_foo2").first, "temperature");
+}
+
+TEST(SplitSensorNameAndType, Name)
+{
+    EXPECT_EQ(splitSensorNameAndType("fantach_foo_1").second, "foo_1");
+    EXPECT_EQ(splitSensorNameAndType("temperature_foo2").second, "foo2");
+}
+
+TEST(SplitSensorNameAndType, Error)
+{
+    EXPECT_TRUE(splitSensorNameAndType("fantach").first.empty());
+    EXPECT_TRUE(splitSensorNameAndType("temperature").second.empty());
+}
+
+} // namespace
+} // namespace redfish