bus: Only close connections we own
If we have multiple code segments in the same thread acquiring the
default bus, we end up with the potential for one of those handles to be
destroyed, calling sd_event_flush_close_unref(). This terminates the bus
for all users of the same reference. Any of the reference which still
exist after the first destroy will then return ENOTCONN for all bus
operations since it is now closed. This behavior is undesirable as we
expect to be able to have transient bus references.
However, we do want to make sure we clean up any buses created by
sd_bus_open().
See openbmc/phosphor-time-manager@4e84539349dac086ce2a58e5b9900ed4e40a2eaf
for a specific example of this behavior being unwanted.
Change-Id: I8aad7e282e9d66993b63e85532dce37c179ad5dc
Signed-off-by: William A. Kennington III <wak@google.com>
diff --git a/sdbusplus/bus.hpp.in b/sdbusplus/bus.hpp.in
index d3a2605..6fe3914 100644
--- a/sdbusplus/bus.hpp.in
+++ b/sdbusplus/bus.hpp.in
@@ -76,8 +76,8 @@
}
SdBusInterface* m_interface;
- decltype(&SdBusInterface::sd_bus_flush_close_unref) deleter =
- &SdBusInterface::sd_bus_flush_close_unref;
+ decltype(&SdBusInterface::sd_bus_unref) deleter =
+ &SdBusInterface::sd_bus_unref;
};
/** @brief Convert a vector of strings to c-style char** array. */
@@ -144,11 +144,23 @@
/** @brief Constructor for 'bus'.
*
* Takes ownership of the bus-pointer and releases it when done.
- * This method will also cause the bus to be flushed and closed
- * on destruction.
*/
bus(busp_t b, std::false_type);
+ /** @brief Sets the bus to be closed when this handle is destroyed. */
+ void set_should_close(bool shouldClose)
+ {
+ if (shouldClose)
+ {
+ _bus.get_deleter().deleter =
+ &SdBusInterface::sd_bus_flush_close_unref;
+ }
+ else
+ {
+ _bus.get_deleter().deleter = &SdBusInterface::sd_bus_unref;
+ }
+ }
+
/** @brief Release ownership of the stored bus-pointer. */
busp_t release()
{
@@ -468,8 +480,6 @@
inline bus::bus(busp_t b, sdbusplus::SdBusInterface* intf) :
_intf(intf), _bus(_intf->sd_bus_ref(b), details::BusDeleter(intf))
{
- _bus.get_deleter().deleter = &SdBusInterface::sd_bus_unref;
-
#if @WANT_TRANSACTION@
// Emitting object added causes a message to get the properties
// which can trigger a 'transaction' in the server bindings. If
@@ -487,8 +497,6 @@
_intf(&sdbus_impl),
_bus(_intf->sd_bus_ref(b), details::BusDeleter(&sdbus_impl))
{
- _bus.get_deleter().deleter = &SdBusInterface::sd_bus_unref;
-
#if @WANT_TRANSACTION@
// Emitting object added causes a message to get the properties
// which can trigger a 'transaction' in the server bindings. If
@@ -547,21 +555,27 @@
{
sd_bus* b = nullptr;
sd_bus_open(&b);
- return bus(b, std::false_type());
+ bus bus(b, std::false_type());
+ bus.set_should_close(true);
+ return bus;
}
inline bus new_user()
{
sd_bus* b = nullptr;
sd_bus_open_user(&b);
- return bus(b, std::false_type());
+ bus bus(b, std::false_type());
+ bus.set_should_close(true);
+ return bus;
}
inline bus new_system()
{
sd_bus* b = nullptr;
sd_bus_open_system(&b);
- return bus(b, std::false_type());
+ bus bus(b, std::false_type());
+ bus.set_should_close(true);
+ return bus;
}
} // namespace bus
diff --git a/test/bus/match.cpp b/test/bus/match.cpp
index 3a5cc49..2074cde 100644
--- a/test/bus/match.cpp
+++ b/test/bus/match.cpp
@@ -6,7 +6,7 @@
class Match : public ::testing::Test
{
protected:
- decltype(sdbusplus::bus::new_default()) bus = sdbusplus::bus::new_default();
+ sdbusplus::bus::bus bus = sdbusplus::bus::new_bus();
static constexpr auto busName = "xyz.openbmc_project.sdbusplus.test.Match";