slot: explicitly transfer pointer
The slot class is intended to be an RAII wrapper around the C-style
`sd_bus_slot*`. As part of the constructor it gets the pointer it is
assuming ownership for.
The previous constructor took the `sd_bus_slot*` by value so it was
possible that the caller still kept the slot rather than fully
transferring ownership. Switch the constructor to an r-value reference
so that the caller's pointer can be erased as part of the ownership
transfer.
Also, add an assignment operator to simplify the syntax when a slot is
first initialized with `nullptr` and then later given ownership of a
newly constructed slot.
While this is an incompatible constructor change, it is not expected to
affect any current users. The only example of referencing a
`slot::slot` I could see in the current OpenBMC org calls the
constructor with a `nullptr`, which will seamlessly call the new r-value
variant.
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Change-Id: I577de2e10e4d5ccae6337c763b848ea4cf2121dd
diff --git a/include/sdbusplus/bus/match.hpp b/include/sdbusplus/bus/match.hpp
index 5763e96..cc6c19c 100644
--- a/include/sdbusplus/bus/match.hpp
+++ b/include/sdbusplus/bus/match.hpp
@@ -42,13 +42,12 @@
* @param[in] context - An optional context to pass to the handler.
*/
match(sdbusplus::bus_t& bus, const char* match,
- sd_bus_message_handler_t handler, void* context = nullptr) :
- _slot(nullptr)
+ sd_bus_message_handler_t handler, void* context = nullptr)
{
sd_bus_slot* slot = nullptr;
sd_bus_add_match(bus.get(), &slot, match, handler, context);
- _slot = decltype(_slot){slot};
+ _slot = std::move(slot);
}
match(sdbusplus::bus_t& bus, const std::string& _match,
sd_bus_message_handler_t handler, void* context = nullptr) :
@@ -64,14 +63,13 @@
* @param[in] callback - The callback for matches.
*/
match(sdbusplus::bus_t& bus, const char* match, callback_t callback) :
- _slot(nullptr),
_callback(std::make_unique<callback_t>(std::move(callback)))
{
sd_bus_slot* slot = nullptr;
sd_bus_add_match(bus.get(), &slot, match, callCallback,
_callback.get());
- _slot = decltype(_slot){slot};
+ _slot = std::move(slot);
}
match(sdbusplus::bus_t& bus, const std::string& _match,
callback_t callback) :
@@ -79,7 +77,7 @@
{}
private:
- slot_t _slot;
+ slot_t _slot{};
std::unique_ptr<callback_t> _callback = nullptr;
static int callCallback(sd_bus_message* m, void* context,
diff --git a/include/sdbusplus/message.hpp b/include/sdbusplus/message.hpp
index 7bd84a8..007880c 100644
--- a/include/sdbusplus/message.hpp
+++ b/include/sdbusplus/message.hpp
@@ -427,25 +427,35 @@
{
throw exception::SdBusError(-r, "sd_bus_call_async");
}
+
+ // Generally, slot_t expects to act like a RAII wrapper but it doesn't
+ // allow direct access to the underlying pointer. We still have some
+ // updates we need to make to the slot after it is saved away in the
+ // RAII wrapper, so we need to explicitly make a copy of the pointer for
+ // later use.
+ sd_bus_slot* slot_saved = slot;
slot_t ret(std::move(slot));
+
if constexpr (std::is_pointer_v<CbT>)
{
- _intf->sd_bus_slot_set_userdata(slot, reinterpret_cast<void*>(cb));
+ _intf->sd_bus_slot_set_userdata(slot_saved,
+ reinterpret_cast<void*>(cb));
}
else if constexpr (std::is_function_v<CbT>)
{
- _intf->sd_bus_slot_set_userdata(slot, reinterpret_cast<void*>(&cb));
+ _intf->sd_bus_slot_set_userdata(slot_saved,
+ reinterpret_cast<void*>(&cb));
}
else
{
r = _intf->sd_bus_slot_set_destroy_callback(
- slot, details::call_async_del<CbT>);
+ slot_saved, details::call_async_del<CbT>);
if (r < 0)
{
throw exception::SdBusError(-r,
"sd_bus_slot_set_destroy_callback");
}
- _intf->sd_bus_slot_set_userdata(slot,
+ _intf->sd_bus_slot_set_userdata(slot_saved,
new CbT(std::forward<Cb>(cb)));
}
return ret;
diff --git a/include/sdbusplus/server/interface.hpp b/include/sdbusplus/server/interface.hpp
index 4e709ea..946c50e 100644
--- a/include/sdbusplus/server/interface.hpp
+++ b/include/sdbusplus/server/interface.hpp
@@ -110,7 +110,7 @@
bus_t _bus;
std::string _path;
std::string _interf;
- slot_t _slot;
+ slot_t _slot{};
SdBusInterface* _intf;
bool _interface_added;
};
diff --git a/include/sdbusplus/server/manager.hpp b/include/sdbusplus/server/manager.hpp
index 9241bd9..32519f4 100644
--- a/include/sdbusplus/server/manager.hpp
+++ b/include/sdbusplus/server/manager.hpp
@@ -48,11 +48,11 @@
intf->sd_bus_add_object_manager(bus.get(), &slot, path);
- _slot = decltype(_slot){slot};
+ _slot = std::move(slot);
}
private:
- slot_t _slot{nullptr};
+ slot_t _slot{};
};
} // namespace manager
diff --git a/include/sdbusplus/slot.hpp b/include/sdbusplus/slot.hpp
index 5fa9141..7144281 100644
--- a/include/sdbusplus/slot.hpp
+++ b/include/sdbusplus/slot.hpp
@@ -36,13 +36,12 @@
{
/* Define all of the basic class operations:
* Not allowed:
- * - Default constructor to avoid nullptrs.
* - Copy operations due to internal unique_ptr.
* Allowed:
+ * - Default constructor, assigned as a nullptr.
* - Move operations.
* - Destructor.
*/
- slot() = delete;
slot(const slot&) = delete;
slot& operator=(const slot&) = delete;
slot(slot&&) = default;
@@ -53,8 +52,22 @@
*
* Takes ownership of the slot-pointer and releases it when done.
*/
- explicit slot(slotp_t s) : _slot(s)
- {}
+ explicit slot(slotp_t&& s = nullptr) : _slot(s)
+ {
+ s = nullptr;
+ }
+
+ /** @brief Conversion from 'slotp_t'.
+ *
+ * Takes ownership of the slot-pointer and releases it when done.
+ */
+ slot& operator=(slotp_t&& s)
+ {
+ _slot.reset(s);
+ s = nullptr;
+
+ return *this;
+ }
/** @brief Release ownership of the stored slot-pointer. */
slotp_t release()
diff --git a/src/server/interface.cpp b/src/server/interface.cpp
index 9832547..653e6d6 100644
--- a/src/server/interface.cpp
+++ b/src/server/interface.cpp
@@ -13,7 +13,7 @@
const char* interf, const sdbusplus::vtable_t* vtable,
void* context) :
_bus(bus.get(), bus.getInterface()),
- _path(path), _interf(interf), _slot(nullptr), _intf(bus.getInterface()),
+ _path(path), _interf(interf), _intf(bus.getInterface()),
_interface_added(false)
{
sd_bus_slot* slot = nullptr;
@@ -24,7 +24,7 @@
throw exception::SdBusError(-r, "sd_bus_add_object_vtable");
}
- _slot = decltype(_slot){slot};
+ _slot = std::move(slot);
}
interface::~interface()