bus: Consistently leverage mock
We have some project generating matches that are using the mock.
Previously, we never checked the return value of this function and it
allowed our match creations to silently fail. With error checking added,
this was breaking unit tests.
This change makes sure that all users of slot generating functions have
their calls properly mocked.
Change-Id: I5dab3e3ae73e8516db21c928fc39bc00d4218c82
Signed-off-by: William A. Kennington III <wak@google.com>
diff --git a/include/sdbusplus/async/match.hpp b/include/sdbusplus/async/match.hpp
index 40ecbe5..3e1b434 100644
--- a/include/sdbusplus/async/match.hpp
+++ b/include/sdbusplus/async/match.hpp
@@ -55,7 +55,7 @@
friend match_ns::match_completion;
private:
- sdbusplus::slot_t slot{};
+ sdbusplus::slot_t slot;
std::mutex lock{};
std::queue<sdbusplus::message_t> queue{};
@@ -69,6 +69,8 @@
* This must be called with `lock` held (and ownership transfers).
*/
void handle_completion(std::unique_lock<std::mutex>&&) noexcept;
+
+ slot_t makeMatch(context& ctx, const std::string_view& pattern);
};
namespace match_ns
diff --git a/include/sdbusplus/message.hpp b/include/sdbusplus/message.hpp
index ad9cef8..54312fa 100644
--- a/include/sdbusplus/message.hpp
+++ b/include/sdbusplus/message.hpp
@@ -451,7 +451,7 @@
{
throw exception::SdBusError(-r, "sd_bus_call_async");
}
- slot_t ret(std::move(slot));
+ slot_t ret(slot, _intf);
if constexpr (std::is_pointer_v<CbT>)
{
diff --git a/include/sdbusplus/sdbus.hpp b/include/sdbusplus/sdbus.hpp
index 7f0a569..9022f1c 100644
--- a/include/sdbusplus/sdbus.hpp
+++ b/include/sdbusplus/sdbus.hpp
@@ -28,6 +28,11 @@
const sd_bus_vtable* vtable,
void* userdata) = 0;
+ virtual int sd_bus_add_match(sd_bus* bus, sd_bus_slot** slot,
+ const char* path,
+ sd_bus_message_handler_t callback,
+ void* userdata) = 0;
+
virtual int sd_bus_attach_event(sd_bus* bus, sd_event* e, int priority) = 0;
virtual int sd_bus_call(sd_bus* bus, sd_bus_message* m, uint64_t usec,
@@ -164,6 +169,7 @@
uint64_t* cookie) = 0;
virtual sd_bus* sd_bus_unref(sd_bus* bus) = 0;
+ virtual sd_bus_slot* sd_bus_slot_unref(sd_bus_slot* slot) = 0;
virtual sd_bus* sd_bus_flush_close_unref(sd_bus* bus) = 0;
virtual int sd_bus_flush(sd_bus* bus) = 0;
@@ -198,6 +204,13 @@
userdata);
}
+ int sd_bus_add_match(sd_bus* bus, sd_bus_slot** slot, const char* path,
+ sd_bus_message_handler_t callback,
+ void* userdata) override
+ {
+ return ::sd_bus_add_match(bus, slot, path, callback, userdata);
+ }
+
int sd_bus_attach_event(sd_bus* bus, sd_event* e, int priority) override
{
return ::sd_bus_attach_event(bus, e, priority);
@@ -529,6 +542,11 @@
return ::sd_bus_unref(bus);
}
+ sd_bus_slot* sd_bus_slot_unref(sd_bus_slot* slot) override
+ {
+ return ::sd_bus_slot_unref(slot);
+ }
+
sd_bus* sd_bus_flush_close_unref(sd_bus* bus) override
{
return ::sd_bus_flush_close_unref(bus);
diff --git a/include/sdbusplus/server/interface.hpp b/include/sdbusplus/server/interface.hpp
index be450ac..78d836f 100644
--- a/include/sdbusplus/server/interface.hpp
+++ b/include/sdbusplus/server/interface.hpp
@@ -110,9 +110,8 @@
bus_t _bus;
std::string _path;
std::string _interf;
- slot_t _slot{};
- SdBusInterface* _intf;
bool _interface_added;
+ slot_t _slot;
};
} // namespace interface
diff --git a/include/sdbusplus/server/manager.hpp b/include/sdbusplus/server/manager.hpp
index 797fb0f..2414660 100644
--- a/include/sdbusplus/server/manager.hpp
+++ b/include/sdbusplus/server/manager.hpp
@@ -21,38 +21,26 @@
*/
struct manager : private sdbusplus::bus::details::bus_friend
{
- /* Define all of the basic class operations:
- * Not allowed:
- * - Default constructor to avoid nullptrs.
- * - Copy operations due to internal unique_ptr.
- * Allowed:
- * - Move operations.
- * - Destructor.
- */
- manager() = delete;
- manager(const manager&) = delete;
- manager& operator=(const manager&) = delete;
- manager(manager&&) = default;
- manager& operator=(manager&&) = default;
- ~manager() = default;
+ private:
+ slot_t _slot;
+ static slot_t makeManager(SdBusInterface* intf, sd_bus* bus,
+ const char* path)
+ {
+ sd_bus_slot* slot = nullptr;
+ intf->sd_bus_add_object_manager(bus, &slot, path);
+ return slot_t{slot, intf};
+ }
+
+ public:
/** @brief Register an object manager at a path.
*
* @param[in] bus - The bus to register on.
* @param[in] path - The path to register.
*/
- manager(sdbusplus::bus_t& bus, const char* path)
- {
- sd_bus_slot* slot = nullptr;
- sdbusplus::SdBusInterface* intf = bus.getInterface();
-
- intf->sd_bus_add_object_manager(get_busp(bus), &slot, path);
-
- _slot = std::move(slot);
- }
-
- private:
- slot_t _slot{};
+ manager(sdbusplus::bus_t& bus, const char* path) :
+ _slot(makeManager(bus.getInterface(), get_busp(bus), path))
+ {}
};
} // namespace manager
diff --git a/include/sdbusplus/slot.hpp b/include/sdbusplus/slot.hpp
index 10a1d0d..f4bbe55 100644
--- a/include/sdbusplus/slot.hpp
+++ b/include/sdbusplus/slot.hpp
@@ -19,10 +19,14 @@
/** @brief unique_ptr functor to release a slot reference. */
struct SlotDeleter
{
+ explicit SlotDeleter(SdBusInterface* intf) : intf(intf) {}
+
void operator()(slotp_t ptr) const
{
- sd_bus_slot_unref(ptr);
+ intf->sd_bus_slot_unref(ptr);
}
+
+ SdBusInterface* intf;
};
using slot = std::unique_ptr<sd_bus_slot, SlotDeleter>;
@@ -36,40 +40,13 @@
*/
struct slot
{
- /* Define all of the basic class operations:
- * Not allowed:
- * - Copy operations due to internal unique_ptr.
- * Allowed:
- * - Default constructor, assigned as a nullptr.
- * - Move operations.
- * - Destructor.
- */
- slot(const slot&) = delete;
- slot& operator=(const slot&) = delete;
- slot(slot&&) = default;
- slot& operator=(slot&&) = default;
- ~slot() = default;
-
/** @brief Conversion constructor for 'slotp_t'.
*
* Takes ownership of the slot-pointer and releases it when done.
*/
- 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;
- }
+ slot(slotp_t s, sdbusplus::SdBusInterface* intf) :
+ _slot(s, details::SlotDeleter(intf))
+ {}
/** @brief Release ownership of the stored slot-pointer. */
slotp_t release()
diff --git a/include/sdbusplus/test/sdbus_mock.hpp b/include/sdbusplus/test/sdbus_mock.hpp
index 0f74f3e..6e1a1a3 100644
--- a/include/sdbusplus/test/sdbus_mock.hpp
+++ b/include/sdbusplus/test/sdbus_mock.hpp
@@ -20,6 +20,11 @@
(sd_bus*, sd_bus_slot**, const char*, const char*,
const sd_bus_vtable*, void*),
(override));
+
+ MOCK_METHOD(int, sd_bus_add_match,
+ (sd_bus*, sd_bus_slot**, const char*, sd_bus_message_handler_t,
+ void*),
+ (override));
MOCK_METHOD(int, sd_bus_attach_event, (sd_bus*, sd_event*, int),
(override));
MOCK_METHOD(int, sd_bus_call,
@@ -151,11 +156,24 @@
MOCK_METHOD(int, sd_bus_send, (sd_bus*, sd_bus_message*, uint64_t*),
(override));
MOCK_METHOD(sd_bus*, sd_bus_unref, (sd_bus*), (override));
+ MOCK_METHOD(sd_bus_slot*, sd_bus_slot_unref, (sd_bus_slot*), (override));
MOCK_METHOD(sd_bus*, sd_bus_flush_close_unref, (sd_bus*), (override));
MOCK_METHOD(int, sd_bus_flush, (sd_bus*), (override));
MOCK_METHOD(void, sd_bus_close, (sd_bus*), (override));
MOCK_METHOD(int, sd_bus_is_open, (sd_bus*), (override));
MOCK_METHOD(int, sd_bus_wait, (sd_bus*, uint64_t), (override));
+
+ SdBusMock()
+ {
+ auto slotcb = [](sd_bus*, sd_bus_slot** slot, auto&&...) {
+ *slot = reinterpret_cast<sd_bus_slot*>(0xdefa);
+ return 0;
+ };
+ ON_CALL(*this, sd_bus_add_object_manager).WillByDefault(slotcb);
+ ON_CALL(*this, sd_bus_add_object_vtable).WillByDefault(slotcb);
+ ON_CALL(*this, sd_bus_add_match).WillByDefault(slotcb);
+ ON_CALL(*this, sd_bus_call_async).WillByDefault(slotcb);
+ }
};
inline bus_t get_mocked_new(SdBusMock* sdbus)
diff --git a/src/async/match.cpp b/src/async/match.cpp
index ae8ed73..ed5237b 100644
--- a/src/async/match.cpp
+++ b/src/async/match.cpp
@@ -3,7 +3,7 @@
namespace sdbusplus::async
{
-match::match(context& ctx, const std::string_view& pattern)
+slot_t match::makeMatch(context& ctx, const std::string_view& pattern)
{
// C-style callback to redirect into this::handle_match.
static auto match_cb =
@@ -12,7 +12,7 @@
return 0;
};
- sd_bus_slot* s = nullptr;
+ sd_bus_slot* s;
auto r = sd_bus_add_match(get_busp(ctx.get_bus()), &s, pattern.data(),
match_cb, this);
if (r < 0)
@@ -20,9 +20,13 @@
throw exception::SdBusError(-r, "sd_bus_add_match (async::match)");
}
- slot = std::move(s);
+ return slot_t{s, &sdbus_impl};
}
+match::match(context& ctx, const std::string_view& pattern) :
+ slot(makeMatch(ctx, pattern))
+{}
+
match::~match()
{
match_ns::match_completion* c = nullptr;
diff --git a/src/bus/match.cpp b/src/bus/match.cpp
index c5b9b19..033a47f 100644
--- a/src/bus/match.cpp
+++ b/src/bus/match.cpp
@@ -7,21 +7,22 @@
namespace sdbusplus::bus::match
{
-static sd_bus_slot* makeMatch(sd_bus* bus, const char* _match,
- sd_bus_message_handler_t handler, void* context)
+static slot_t makeMatch(SdBusInterface* intf, sd_bus* bus, const char* _match,
+ sd_bus_message_handler_t handler, void* context)
{
sd_bus_slot* slot;
- int r = sd_bus_add_match(bus, &slot, _match, handler, context);
+ int r = intf->sd_bus_add_match(bus, &slot, _match, handler, context);
if (r < 0)
{
throw exception::SdBusError(-r, "sd_bus_match");
}
- return slot;
+ return slot_t{slot, intf};
}
match::match(sdbusplus::bus_t& bus, const char* _match,
sd_bus_message_handler_t handler, void* context) :
- _slot(makeMatch(get_busp(bus), _match, handler, context))
+ _slot(
+ makeMatch(bus.getInterface(), get_busp(bus), _match, handler, context))
{}
// The callback is 'noexcept' because it is called from C code (sd-bus).
@@ -36,7 +37,8 @@
match::match(sdbusplus::bus_t& bus, const char* _match, callback_t callback) :
_callback(std::make_unique<callback_t>(std::move(callback))),
- _slot(makeMatch(get_busp(bus), _match, matchCallback, _callback.get()))
+ _slot(makeMatch(bus.getInterface(), get_busp(bus), _match, matchCallback,
+ _callback.get()))
{}
} // namespace sdbusplus::bus::match
diff --git a/src/server/interface.cpp b/src/server/interface.cpp
index e47f6ef..d3a975f 100644
--- a/src/server/interface.cpp
+++ b/src/server/interface.cpp
@@ -9,24 +9,29 @@
namespace interface
{
-interface::interface(sdbusplus::bus_t& bus, const char* path,
- const char* interf, const sdbusplus::vtable_t* vtable,
- void* context) :
- _bus(get_busp(bus), bus.getInterface()),
- _path(path), _interf(interf), _intf(bus.getInterface()),
- _interface_added(false)
+static slot_t makeObjVtable(SdBusInterface* intf, sd_bus* bus, const char* path,
+ const char* interf,
+ const sdbusplus::vtable_t* vtable, void* context)
{
- sd_bus_slot* slot = nullptr;
- int r = _intf->sd_bus_add_object_vtable(
- get_busp(_bus), &slot, _path.c_str(), _interf.c_str(), vtable, context);
+ sd_bus_slot* slot;
+ int r = intf->sd_bus_add_object_vtable(bus, &slot, path, interf, vtable,
+ context);
if (r < 0)
{
throw exception::SdBusError(-r, "sd_bus_add_object_vtable");
}
-
- _slot = std::move(slot);
+ return slot_t{slot, intf};
}
+interface::interface(sdbusplus::bus_t& bus, const char* path,
+ const char* interf, const sdbusplus::vtable_t* vtable,
+ void* context) :
+ _bus(get_busp(bus), bus.getInterface()),
+ _path(path), _interf(interf), _interface_added(false),
+ _slot(makeObjVtable(_bus.getInterface(), get_busp(_bus), _path.c_str(),
+ _interf.c_str(), vtable, context))
+{}
+
interface::~interface()
{
emit_removed();
@@ -38,8 +43,8 @@
// Note: Converting to use _strv version, could also mock two pointer
// use-case explicitly.
- _intf->sd_bus_emit_properties_changed_strv(get_busp(_bus), _path.c_str(),
- _interf.c_str(), values.data());
+ _bus.getInterface()->sd_bus_emit_properties_changed_strv(
+ get_busp(_bus), _path.c_str(), _interf.c_str(), values.data());
}
} // namespace interface