Make the dbushelper own its own bus handle.
The dbushelper implements an interface that should not be sdbusplus
specific. By making the implementation own the sdbusplus aspects, an
alternate implementation can be swapped in.
Signed-off-by: Patrick Venture <venture@google.com>
Change-Id: I4109772499421e2e6497a0fcad663ebbd1210a7c
diff --git a/dbus/dbusactiveread.hpp b/dbus/dbusactiveread.hpp
index 39640cc..8f92cb4 100644
--- a/dbus/dbusactiveread.hpp
+++ b/dbus/dbusactiveread.hpp
@@ -20,9 +20,10 @@
{
public:
DbusActiveRead(sdbusplus::bus::bus& bus, const std::string& path,
- const std::string& service, DbusHelperInterface* helper) :
+ const std::string& service,
+ std::unique_ptr<DbusHelperInterface> helper) :
ReadInterface(),
- _bus(bus), _path(path), _service(service), _helper(helper)
+ _bus(bus), _path(path), _service(service), _helper(std::move(helper))
{}
ReadReturn read(void) override;
@@ -31,7 +32,7 @@
sdbusplus::bus::bus& _bus;
const std::string _path;
const std::string _service; // the sensor service.
- DbusHelperInterface* _helper;
+ std::unique_ptr<DbusHelperInterface> _helper;
};
} // namespace pid_control
diff --git a/dbus/dbusconfiguration.cpp b/dbus/dbusconfiguration.cpp
index 7402de0..341a204 100644
--- a/dbus/dbusconfiguration.cpp
+++ b/dbus/dbusconfiguration.cpp
@@ -391,7 +391,7 @@
}
const std::string& path = sensorConfig[info.inputs.front()].readPath;
- DbusHelper helper;
+ DbusHelper helper(sdbusplus::bus::new_system());
std::string service = helper.getService(bus, interface, path);
double reading = 0;
try
diff --git a/dbus/dbushelper.cpp b/dbus/dbushelper.cpp
index cb847e6..f8f087d 100644
--- a/dbus/dbushelper.cpp
+++ b/dbus/dbushelper.cpp
@@ -44,9 +44,9 @@
const std::string& path)
{
auto mapper =
- bus.new_method_call("xyz.openbmc_project.ObjectMapper",
- "/xyz/openbmc_project/object_mapper",
- "xyz.openbmc_project.ObjectMapper", "GetObject");
+ _bus.new_method_call("xyz.openbmc_project.ObjectMapper",
+ "/xyz/openbmc_project/object_mapper",
+ "xyz.openbmc_project.ObjectMapper", "GetObject");
mapper.append(path);
mapper.append(std::vector<std::string>({intf}));
@@ -55,7 +55,7 @@
try
{
- auto responseMsg = bus.call(mapper);
+ auto responseMsg = _bus.call(mapper);
responseMsg.read(response);
}
@@ -79,8 +79,8 @@
const std::string& path,
struct SensorProperties* prop)
{
- auto pimMsg = bus.new_method_call(service.c_str(), path.c_str(),
- propertiesintf, "GetAll");
+ auto pimMsg = _bus.new_method_call(service.c_str(), path.c_str(),
+ propertiesintf, "GetAll");
pimMsg.append(sensorintf);
@@ -88,7 +88,7 @@
try
{
- auto valueResponseMsg = bus.call(pimMsg);
+ auto valueResponseMsg = _bus.call(pimMsg);
valueResponseMsg.read(propMap);
}
catch (const sdbusplus::exception::SdBusError& ex)
@@ -141,14 +141,14 @@
const std::string& path)
{
- auto critical = bus.new_method_call(service.c_str(), path.c_str(),
- propertiesintf, "GetAll");
+ auto critical = _bus.new_method_call(service.c_str(), path.c_str(),
+ propertiesintf, "GetAll");
critical.append(criticalThreshInf);
PropertyMap criticalMap;
try
{
- auto msg = bus.call(critical);
+ auto msg = _bus.call(critical);
msg.read(criticalMap);
}
catch (sdbusplus::exception_t&)
diff --git a/dbus/dbushelper.hpp b/dbus/dbushelper.hpp
index 7ae60f1..0831bd4 100644
--- a/dbus/dbushelper.hpp
+++ b/dbus/dbushelper.hpp
@@ -19,10 +19,13 @@
static constexpr char criticalThreshInf[] =
"xyz.openbmc_project.Sensor.Threshold.Critical";
- DbusHelper() = default;
+ explicit DbusHelper(sdbusplus::bus::bus bus) : _bus(std::move(bus))
+ {}
~DbusHelper() = default;
- DbusHelper(const DbusHelper&) = default;
- DbusHelper& operator=(const DbusHelper&) = default;
+
+ DbusHelper(const DbusHelper&) = delete;
+ DbusHelper& operator=(const DbusHelper&) = delete;
+
DbusHelper(DbusHelper&&) = default;
DbusHelper& operator=(DbusHelper&&) = default;
@@ -44,15 +47,15 @@
{
namespace log = phosphor::logging;
- auto msg = bus.new_method_call(service.c_str(), path.c_str(),
- propertiesintf, "Get");
+ auto msg = _bus.new_method_call(service.c_str(), path.c_str(),
+ propertiesintf, "Get");
msg.append(interface, propertyName);
std::variant<T> result;
try
{
- auto valueResponseMsg = bus.call(msg);
+ auto valueResponseMsg = _bus.call(msg);
valueResponseMsg.read(result);
}
catch (const sdbusplus::exception::SdBusError& ex)
@@ -64,6 +67,9 @@
prop = std::get<T>(result);
}
+
+ private:
+ sdbusplus::bus::bus _bus;
};
} // namespace pid_control
diff --git a/dbus/dbuspassive.cpp b/dbus/dbuspassive.cpp
index 27255e7..84d1f93 100644
--- a/dbus/dbuspassive.cpp
+++ b/dbus/dbuspassive.cpp
@@ -34,7 +34,7 @@
std::unique_ptr<ReadInterface> DbusPassive::createDbusPassive(
sdbusplus::bus::bus& bus, const std::string& type, const std::string& id,
- DbusHelperInterface* helper, const conf::SensorConfig* info,
+ std::unique_ptr<DbusHelperInterface> helper, const conf::SensorConfig* info,
const std::shared_ptr<DbusPassiveRedundancy>& redundancy)
{
if (helper == nullptr)
@@ -74,18 +74,19 @@
settings.max = 0;
}
- return std::make_unique<DbusPassive>(bus, type, id, helper, settings,
- failed, path, redundancy);
+ return std::make_unique<DbusPassive>(bus, type, id, std::move(helper),
+ settings, failed, path, redundancy);
}
DbusPassive::DbusPassive(
sdbusplus::bus::bus& bus, const std::string& type, const std::string& id,
- DbusHelperInterface* helper, const struct SensorProperties& settings,
- bool failed, const std::string& path,
+ std::unique_ptr<DbusHelperInterface> helper,
+ const struct SensorProperties& settings, bool failed,
+ const std::string& path,
const std::shared_ptr<DbusPassiveRedundancy>& redundancy) :
ReadInterface(),
_bus(bus), _signal(bus, getMatch(type, id).c_str(), dbusHandleSignal, this),
- _id(id), _helper(helper), _failed(failed), path(path),
+ _id(id), _helper(std::move(helper)), _failed(failed), path(path),
redundancy(redundancy)
{
diff --git a/dbus/dbuspassive.hpp b/dbus/dbuspassive.hpp
index 07a6ab2..fa0c5da 100644
--- a/dbus/dbuspassive.hpp
+++ b/dbus/dbuspassive.hpp
@@ -42,12 +42,13 @@
public:
static std::unique_ptr<ReadInterface> createDbusPassive(
sdbusplus::bus::bus& bus, const std::string& type,
- const std::string& id, DbusHelperInterface* helper,
+ const std::string& id, std::unique_ptr<DbusHelperInterface> helper,
const conf::SensorConfig* info,
const std::shared_ptr<DbusPassiveRedundancy>& redundancy);
DbusPassive(sdbusplus::bus::bus& bus, const std::string& type,
- const std::string& id, DbusHelperInterface* helper,
+ const std::string& id,
+ std::unique_ptr<DbusHelperInterface> helper,
const struct SensorProperties& settings, bool failed,
const std::string& path,
const std::shared_ptr<DbusPassiveRedundancy>& redundancy);
@@ -68,7 +69,7 @@
sdbusplus::server::match::match _signal;
int64_t _scale;
std::string _id; // for debug identification
- DbusHelperInterface* _helper;
+ std::unique_ptr<DbusHelperInterface> _helper;
std::mutex _lock;
double _value = 0;
diff --git a/dbus/dbuswrite.cpp b/dbus/dbuswrite.cpp
index 83e5cd1..a6b2835 100644
--- a/dbus/dbuswrite.cpp
+++ b/dbus/dbuswrite.cpp
@@ -21,6 +21,7 @@
#include <phosphor-logging/log.hpp>
#include <sdbusplus/bus.hpp>
+#include <exception>
#include <iostream>
#include <memory>
#include <string>
@@ -33,16 +34,16 @@
using namespace phosphor::logging;
-std::unique_ptr<WriteInterface>
- DbusWritePercent::createDbusWrite(const std::string& path, int64_t min,
- int64_t max, DbusHelperInterface& helper)
+std::unique_ptr<WriteInterface> DbusWritePercent::createDbusWrite(
+ const std::string& path, int64_t min, int64_t max,
+ std::unique_ptr<DbusHelperInterface> helper)
{
auto tempBus = sdbusplus::bus::new_system();
std::string connectionName;
try
{
- connectionName = helper.getService(tempBus, pwmInterface, path);
+ connectionName = helper->getService(tempBus, pwmInterface, path);
}
catch (const std::exception& e)
{
@@ -89,14 +90,15 @@
std::unique_ptr<WriteInterface>
DbusWrite::createDbusWrite(const std::string& path, int64_t min,
- int64_t max, DbusHelperInterface& helper)
+ int64_t max,
+ std::unique_ptr<DbusHelperInterface> helper)
{
auto tempBus = sdbusplus::bus::new_system();
std::string connectionName;
try
{
- connectionName = helper.getService(tempBus, pwmInterface, path);
+ connectionName = helper->getService(tempBus, pwmInterface, path);
}
catch (const std::exception& e)
{
diff --git a/dbus/dbuswrite.hpp b/dbus/dbuswrite.hpp
index 06b7dcc..f890781 100644
--- a/dbus/dbuswrite.hpp
+++ b/dbus/dbuswrite.hpp
@@ -33,7 +33,7 @@
public:
static std::unique_ptr<WriteInterface>
createDbusWrite(const std::string& path, int64_t min, int64_t max,
- DbusHelperInterface& helper);
+ std::unique_ptr<DbusHelperInterface> helper);
DbusWritePercent(const std::string& path, int64_t min, int64_t max,
const std::string& connectionName) :
@@ -54,7 +54,7 @@
public:
static std::unique_ptr<WriteInterface>
createDbusWrite(const std::string& path, int64_t min, int64_t max,
- DbusHelperInterface& helper);
+ std::unique_ptr<DbusHelperInterface> helper);
DbusWrite(const std::string& path, int64_t min, int64_t max,
const std::string& connectionName) :
diff --git a/sensors/builder.cpp b/sensors/builder.cpp
index 39e35f2..de7e020 100644
--- a/sensors/builder.cpp
+++ b/sensors/builder.cpp
@@ -16,6 +16,7 @@
#include <iostream>
#include <map>
+#include <memory>
#include <string>
/* Configuration. */
@@ -40,7 +41,6 @@
{
static constexpr bool deferSignals = true;
-static DbusHelper helper;
SensorManager
buildSensors(const std::map<std::string, struct conf::SensorConfig>& config,
@@ -81,14 +81,18 @@
if (info->type == "fan")
{
ri = DbusPassive::createDbusPassive(
- passiveListeningBus, info->type, name, &helper, info,
- redundancy);
+ passiveListeningBus, info->type, name,
+ std::make_unique<DbusHelper>(
+ sdbusplus::bus::new_system()),
+ info, redundancy);
}
else
{
- ri = DbusPassive::createDbusPassive(passiveListeningBus,
- info->type, name,
- &helper, info, nullptr);
+ ri = DbusPassive::createDbusPassive(
+ passiveListeningBus, info->type, name,
+ std::make_unique<DbusHelper>(
+ sdbusplus::bus::new_system()),
+ info, nullptr);
}
if (ri == nullptr)
{
@@ -129,12 +133,16 @@
if (info->max > 0)
{
wi = DbusWritePercent::createDbusWrite(
- info->writePath, info->min, info->max, helper);
+ info->writePath, info->min, info->max,
+ std::make_unique<DbusHelper>(
+ sdbusplus::bus::new_system()));
}
else
{
wi = DbusWrite::createDbusWrite(
- info->writePath, info->min, info->max, helper);
+ info->writePath, info->min, info->max,
+ std::make_unique<DbusHelper>(
+ sdbusplus::bus::new_system()));
}
if (wi == nullptr)
diff --git a/test/dbus_active_unittest.cpp b/test/dbus_active_unittest.cpp
index 054ad1d..3fb1dc6 100644
--- a/test/dbus_active_unittest.cpp
+++ b/test/dbus_active_unittest.cpp
@@ -3,6 +3,7 @@
#include <sdbusplus/test/sdbus_mock.hpp>
+#include <memory>
#include <string>
#include <gmock/gmock.h>
@@ -23,11 +24,11 @@
sdbusplus::SdBusMock sdbus_mock;
auto bus_mock = sdbusplus::get_mocked_new(&sdbus_mock);
- DbusHelperMock helper;
+ auto helper = std::make_unique<DbusHelperMock>();
std::string path = "/asdf";
std::string service = "asdfasdf.asdfasdf";
- DbusActiveRead ar(bus_mock, path, service, &helper);
+ DbusActiveRead ar(bus_mock, path, service, std::move(helper));
}
TEST(DbusActiveReadTest, Read_VerifyCallsToDbusForValue)
@@ -36,13 +37,11 @@
sdbusplus::SdBusMock sdbus_mock;
auto bus_mock = sdbusplus::get_mocked_new(&sdbus_mock);
- DbusHelperMock helper;
+ auto helper = std::make_unique<DbusHelperMock>();
std::string path = "/asdf";
std::string service = "asdfasdf.asdfasdf";
- DbusActiveRead ar(bus_mock, path, service, &helper);
-
- EXPECT_CALL(helper, getProperties(_, service, path, NotNull()))
+ EXPECT_CALL(*helper, getProperties(_, service, path, NotNull()))
.WillOnce(
Invoke([&](sdbusplus::bus::bus& bus, const std::string& service,
const std::string& path, struct SensorProperties* prop) {
@@ -51,6 +50,8 @@
prop->unit = "x";
}));
+ DbusActiveRead ar(bus_mock, path, service, std::move(helper));
+
ReadReturn r = ar.read();
EXPECT_EQ(10, r.value);
}
diff --git a/test/dbus_passive_unittest.cpp b/test/dbus_passive_unittest.cpp
index 5bddf51..8eb3416 100644
--- a/test/dbus_passive_unittest.cpp
+++ b/test/dbus_passive_unittest.cpp
@@ -5,6 +5,7 @@
#include <sdbusplus/test/sdbus_mock.hpp>
#include <functional>
+#include <memory>
#include <string>
#include <variant>
@@ -35,11 +36,11 @@
std::string type = "invalid";
std::string id = "id";
- DbusHelperMock helper;
+ auto helper = std::make_unique<DbusHelperMock>();
auto info = conf::SensorConfig();
std::unique_ptr<ReadInterface> ri = DbusPassive::createDbusPassive(
- bus_mock, type, id, &helper, &info, nullptr);
+ bus_mock, type, id, std::move(helper), &info, nullptr);
EXPECT_EQ(ri, nullptr);
}
@@ -54,10 +55,11 @@
std::string id = "id";
std::string path = "/xyz/openbmc_project/sensors/unknown/id";
- DbusHelperMock helper;
+ auto helper = std::make_unique<DbusHelperMock>();
struct SensorProperties properties;
- DbusPassive(bus_mock, type, id, &helper, properties, false, path, nullptr);
+ DbusPassive(bus_mock, type, id, std::move(helper), properties, false, path,
+ nullptr);
// Success
}
@@ -66,12 +68,13 @@
protected:
DbusPassiveTestObj() :
sdbus_mock(),
- bus_mock(std::move(sdbusplus::get_mocked_new(&sdbus_mock))), helper()
+ bus_mock(std::move(sdbusplus::get_mocked_new(&sdbus_mock))),
+ helper(std::make_unique<DbusHelperMock>())
{
- EXPECT_CALL(helper, getService(_, StrEq(SensorIntf), StrEq(path)))
+ EXPECT_CALL(*helper, getService(_, StrEq(SensorIntf), StrEq(path)))
.WillOnce(Return("asdf"));
- EXPECT_CALL(helper,
+ EXPECT_CALL(*helper,
getProperties(_, StrEq("asdf"), StrEq(path), NotNull()))
.WillOnce(Invoke(
[&](sdbusplus::bus::bus& bus, const std::string& service,
@@ -82,19 +85,19 @@
prop->min = 0;
prop->max = 0;
}));
- EXPECT_CALL(helper, thresholdsAsserted(_, StrEq("asdf"), StrEq(path)))
+ EXPECT_CALL(*helper, thresholdsAsserted(_, StrEq("asdf"), StrEq(path)))
.WillOnce(Return(false));
auto info = conf::SensorConfig();
- ri = DbusPassive::createDbusPassive(bus_mock, type, id, &helper, &info,
- nullptr);
+ ri = DbusPassive::createDbusPassive(bus_mock, type, id,
+ std::move(helper), &info, nullptr);
passive = reinterpret_cast<DbusPassive*>(ri.get());
EXPECT_FALSE(passive == nullptr);
}
sdbusplus::SdBusMock sdbus_mock;
sdbusplus::bus::bus bus_mock;
- DbusHelperMock helper;
+ std::unique_ptr<DbusHelperMock> helper;
std::string type = "temp";
std::string id = "id";
std::string path = "/xyz/openbmc_project/sensors/temperature/id";
@@ -453,27 +456,28 @@
protected:
DbusPassiveTest3kMaxObj() :
sdbus_mock(),
- bus_mock(std::move(sdbusplus::get_mocked_new(&sdbus_mock))), helper()
+ bus_mock(std::move(sdbusplus::get_mocked_new(&sdbus_mock))),
+ helper(std::make_unique<DbusHelperMock>())
{
- EXPECT_CALL(helper, getService(_, StrEq(SensorIntf), StrEq(path)))
+ EXPECT_CALL(*helper, getService(_, StrEq(SensorIntf), StrEq(path)))
.WillOnce(Return("asdf"));
- EXPECT_CALL(helper,
+ EXPECT_CALL(*helper,
getProperties(_, StrEq("asdf"), StrEq(path), NotNull()))
.WillOnce(_getProps);
- EXPECT_CALL(helper, thresholdsAsserted(_, StrEq("asdf"), StrEq(path)))
+ EXPECT_CALL(*helper, thresholdsAsserted(_, StrEq("asdf"), StrEq(path)))
.WillOnce(Return(false));
auto info = conf::SensorConfig();
- ri = DbusPassive::createDbusPassive(bus_mock, type, id, &helper, &info,
- nullptr);
+ ri = DbusPassive::createDbusPassive(bus_mock, type, id,
+ std::move(helper), &info, nullptr);
passive = reinterpret_cast<DbusPassive*>(ri.get());
EXPECT_FALSE(passive == nullptr);
}
sdbusplus::SdBusMock sdbus_mock;
sdbusplus::bus::bus bus_mock;
- DbusHelperMock helper;
+ std::unique_ptr<DbusHelperMock> helper;
std::string type = "temp";
std::string id = "id";
std::string path = "/xyz/openbmc_project/sensors/temperature/id";
@@ -496,28 +500,29 @@
protected:
DbusPassiveTest3kMaxIgnoredObj() :
sdbus_mock(),
- bus_mock(std::move(sdbusplus::get_mocked_new(&sdbus_mock))), helper()
+ bus_mock(std::move(sdbusplus::get_mocked_new(&sdbus_mock))),
+ helper(std::make_unique<DbusHelperMock>())
{
- EXPECT_CALL(helper, getService(_, StrEq(SensorIntf), StrEq(path)))
+ EXPECT_CALL(*helper, getService(_, StrEq(SensorIntf), StrEq(path)))
.WillOnce(Return("asdf"));
- EXPECT_CALL(helper,
+ EXPECT_CALL(*helper,
getProperties(_, StrEq("asdf"), StrEq(path), NotNull()))
.WillOnce(_getProps);
- EXPECT_CALL(helper, thresholdsAsserted(_, StrEq("asdf"), StrEq(path)))
+ EXPECT_CALL(*helper, thresholdsAsserted(_, StrEq("asdf"), StrEq(path)))
.WillOnce(Return(false));
auto info = conf::SensorConfig();
info.ignoreDbusMinMax = true;
- ri = DbusPassive::createDbusPassive(bus_mock, type, id, &helper, &info,
- nullptr);
+ ri = DbusPassive::createDbusPassive(bus_mock, type, id,
+ std::move(helper), &info, nullptr);
passive = reinterpret_cast<DbusPassive*>(ri.get());
EXPECT_FALSE(passive == nullptr);
}
sdbusplus::SdBusMock sdbus_mock;
sdbusplus::bus::bus bus_mock;
- DbusHelperMock helper;
+ std::unique_ptr<DbusHelperMock> helper;
std::string type = "temp";
std::string id = "id";
std::string path = "/xyz/openbmc_project/sensors/temperature/id";