Emit adding/removing interfaces for object server
The object server currently either creats the objects and interfaces, or
defer the signal by not adding objects.
In practice, we have situations that the code would like to add
interfaces to an existing object, and it's not supported, or needs
tricky code to workaround.
Exmaples:
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-bmc-code-mgmt/+/5820
https://gerrit.openbmc-project.xyz/c/openbmc/openpower-pnor-code-mgmt/+/5346
This commit adds the support by:
1. Adding emit_added() in interface.hpp and the generated server.hpp
2. Adding a enum class in object's constructor to indicate which action
to do, to create the object, or adding the interface, or defer signal
as before.
So the user of object<> could pass `action::emit_interface_added` to the
constructor to tell the object server *only* emit interface added to
DBus, without emitting object added.
The previous code stays the same behavior:
* If `true` is passed in object's constructor, it defers emitting object
added signal;
* If no extra parameter is passed in object's constructor, it emits
object added signal as before.
Tested: 1. Make sure the openbmc builds fine with
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-logging/+/25089
because phosphor-logging uses its own server.hpp for interface, the
above patch removes that.
2. Manually write a small service to verify the interfaces are
added and removed by using the `emit_interface_added` action.
3. Added the unit test cases for object.hpp to check the
ctor/dtor with different actions.
Signed-off-by: Lei YU <mine260309@gmail.com>
Change-Id: I178c5bed3c9ff39ee2ac8d143fbe9131b0753dfa
diff --git a/.gitignore b/.gitignore
index 8164d51..28a5ab9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -66,3 +66,5 @@
/test/utility_tuple_to_array
/test/utility_type_traits
/test/vtable_vtable
+/test/object
+/test/server/Test
diff --git a/sdbusplus/server/interface.hpp b/sdbusplus/server/interface.hpp
index 4c9d133..2b05931 100644
--- a/sdbusplus/server/interface.hpp
+++ b/sdbusplus/server/interface.hpp
@@ -46,7 +46,6 @@
interface& operator=(const interface&) = delete;
interface(interface&&) = default;
interface& operator=(interface&&) = default;
- ~interface() = default;
/** @brief Register the (path, interface, vtable) as a dbus object.
*
@@ -60,7 +59,8 @@
interface(sdbusplus::bus::bus& bus, const char* path, const char* interf,
const sdbusplus::vtable::vtable_t* vtable, void* context) :
_bus(bus.get(), bus.getInterface()),
- _path(path), _interf(interf), _slot(nullptr), _intf(bus.getInterface())
+ _path(path), _interf(interf), _slot(nullptr), _intf(bus.getInterface()),
+ _interface_added(false)
{
sd_bus_slot* slot = nullptr;
_intf->sd_bus_add_object_vtable(_bus.get(), &slot, _path.c_str(),
@@ -69,6 +69,11 @@
_slot = decltype(_slot){slot};
}
+ ~interface()
+ {
+ emit_removed();
+ }
+
/** @brief Create a new signal message.
*
* @param[in] member - The signal name to create.
@@ -93,6 +98,26 @@
_bus.get(), _path.c_str(), _interf.c_str(), static_cast<char**>(p));
}
+ /** @brief Emit the interface is added on D-Bus */
+ void emit_added()
+ {
+ if (!_interface_added)
+ {
+ _bus.emit_interfaces_added(_path.c_str(), {_interf});
+ _interface_added = true;
+ }
+ }
+
+ /** @brief Emit the interface is removed on D-Bus */
+ void emit_removed()
+ {
+ if (_interface_added)
+ {
+ _bus.emit_interfaces_removed(_path.c_str(), {_interf});
+ _interface_added = false;
+ }
+ }
+
bus::bus& bus()
{
return _bus;
@@ -108,6 +133,7 @@
std::string _interf;
slot::slot _slot;
SdBusInterface* _intf;
+ bool _interface_added;
};
} // namespace interface
diff --git a/sdbusplus/server/object.hpp b/sdbusplus/server/object.hpp
index 7aaeaa5..afb031e 100644
--- a/sdbusplus/server/object.hpp
+++ b/sdbusplus/server/object.hpp
@@ -2,6 +2,7 @@
#include <sdbusplus/bus.hpp>
#include <sdbusplus/sdbus.hpp>
+#include <type_traits>
namespace sdbusplus
{
@@ -15,6 +16,25 @@
namespace details
{
+/** Helper functions to detect if member exists in a class */
+
+/** Test if emit_added() exists in T return std::true_type */
+template <class T>
+constexpr auto has_emit_added_helper(int)
+ -> decltype(std::declval<T>().emit_added(), std::true_type{});
+
+/** If the above test fails, fall back to this to return std::false_type */
+template <class>
+constexpr std::false_type has_emit_added_helper(...);
+
+/** Invoke the test with an int so it first resolves to
+ * has_emit_added_helper(int), and when it fails, it resovles to
+ * has_emit_added_helper(...) thanks to SFINAE.
+ * So the return type is std::true_type if emit_added() exists in T and
+ * std::false_type otherwise */
+template <class T>
+using has_emit_added = decltype(has_emit_added_helper<T>(0));
+
/** Templates to allow multiple inheritance via template parameters.
*
* These allow an object to group multiple dbus interface bindings into a
@@ -87,6 +107,13 @@
object(object&&) = default;
object& operator=(object&&) = default;
+ enum class action
+ {
+ emit_object_added,
+ emit_interface_added,
+ defer_emit,
+ };
+
/** Construct an 'object' on a bus with a path.
*
* @param[in] bus - The bus to place the object on.
@@ -96,17 +123,23 @@
* object needs custom property init before the
* signal can be sent.
*/
- object(bus::bus& bus, const char* path, bool deferSignal = false) :
+ object(bus::bus& bus, const char* path,
+ action act = action::emit_object_added) :
details::compose<Args...>(bus, path),
__sdbusplus_server_object_bus(bus.get(), bus.getInterface()),
__sdbusplus_server_object_path(path),
__sdbusplus_server_object_emitremoved(false),
__sdbusplus_server_object_intf(bus.getInterface())
{
- if (!deferSignal)
- {
- emit_object_added();
- }
+ // Default ctor
+ check_action(act);
+ }
+
+ object(bus::bus& bus, const char* path, bool deferSignal) :
+ object(bus, path,
+ deferSignal ? action::defer_emit : action::emit_object_added)
+ {
+ // Delegate to default ctor
}
~object()
@@ -140,6 +173,30 @@
std::string __sdbusplus_server_object_path;
bool __sdbusplus_server_object_emitremoved;
SdBusInterface* __sdbusplus_server_object_intf;
+
+ /** Detect if the interface has emit_added() and invoke it */
+ template <class T>
+ void maybe_emit()
+ {
+ if constexpr (details::has_emit_added<T>())
+ {
+ T::emit_added();
+ }
+ }
+
+ /** Check and run the action */
+ void check_action(action act)
+ {
+ if (act == action::emit_object_added)
+ {
+ emit_object_added();
+ }
+ else if (act == action::emit_interface_added)
+ {
+ (maybe_emit<Args>(), ...);
+ }
+ // Otherwise, do nothing
+ }
};
} // namespace object
diff --git a/test/Makefile.am b/test/Makefile.am
index ab52758..445569a 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -60,4 +60,25 @@
timer_SOURCES = timer.cpp
timer_LDADD = $(gtest_ldadd)
+server/Test/server.hpp:
+ @mkdir -p $(@D)
+ @top_srcdir@/tools/sdbus++ \
+ -r $(srcdir) -t $(top_builddir)/tools/sdbusplus/templates \
+ interface server-header server.Test > $@
+
+server/Test/server.cpp:
+ @mkdir -p $(@D)
+ @top_srcdir@/tools/sdbus++ \
+ -r $(srcdir) -t $(top_builddir)/tools/sdbusplus/templates \
+ interface server-cpp server.Test > $@
+
+server_test_generated_sources = server/Test/server.cpp server/Test/server.hpp
+
+BUILT_SOURCES = $(server_test_generated_sources)
+CLEANFILES = $(server_test_generated_sources)
+
+check_PROGRAMS += object
+object_SOURCES = server/object.cpp $(server_test_generated_sources)
+object_LDADD = $(gtest_ldadd)
+
endif
diff --git a/test/server/Test.interface.yaml b/test/server/Test.interface.yaml
new file mode 100644
index 0000000..d015c99
--- /dev/null
+++ b/test/server/Test.interface.yaml
@@ -0,0 +1,5 @@
+description: >
+ A test interface
+properties:
+ - name: SomeValue
+ type: int64
diff --git a/test/server/object.cpp b/test/server/object.cpp
new file mode 100644
index 0000000..8faa668
--- /dev/null
+++ b/test/server/object.cpp
@@ -0,0 +1,105 @@
+#include "Test/server.hpp"
+
+#include <sdbusplus/bus.hpp>
+#include <sdbusplus/server/manager.hpp>
+#include <sdbusplus/test/sdbus_mock.hpp>
+
+#include <gtest/gtest.h>
+
+using ::testing::_;
+using ::testing::StrEq;
+
+using TestInherit =
+ sdbusplus::server::object::object<sdbusplus::server::server::Test>;
+
+class Object : public ::testing::Test
+{
+ protected:
+ sdbusplus::SdBusMock sdbusMock;
+ sdbusplus::bus::bus bus = sdbusplus::get_mocked_new(&sdbusMock);
+
+ static constexpr auto busName = "xyz.openbmc_project.sdbusplus.test.Object";
+ static constexpr auto objPath = "/xyz/openbmc_project/sdbusplus/test";
+};
+
+TEST_F(Object, InterfaceAddedOnly)
+{
+ // Simulate the typical usage of a service
+ sdbusplus::server::manager::manager objManager(bus, objPath);
+ bus.request_name(busName);
+
+ EXPECT_CALL(sdbusMock, sd_bus_emit_object_added(_, StrEq(objPath)))
+ .Times(0);
+ EXPECT_CALL(sdbusMock,
+ sd_bus_emit_interfaces_added_strv(_, StrEq(objPath), _))
+ .Times(1);
+
+ // This only add interface to the existing object
+ auto test = std::make_unique<TestInherit>(
+ bus, objPath, TestInherit::action::emit_interface_added);
+
+ // After destruction, the interface shall be removed
+ EXPECT_CALL(sdbusMock, sd_bus_emit_object_removed(_, StrEq(objPath)))
+ .Times(0);
+ EXPECT_CALL(sdbusMock,
+ sd_bus_emit_interfaces_removed_strv(_, StrEq(objPath), _))
+ .Times(1);
+}
+
+TEST_F(Object, DeferAddInterface)
+{
+ // Simulate the typical usage of a service
+ sdbusplus::server::manager::manager objManager(bus, objPath);
+ bus.request_name(busName);
+
+ EXPECT_CALL(sdbusMock, sd_bus_emit_object_added(_, StrEq(objPath)))
+ .Times(0);
+ EXPECT_CALL(sdbusMock,
+ sd_bus_emit_interfaces_added_strv(_, StrEq(objPath), _))
+ .Times(0);
+
+ // It defers emit_object_added()
+ auto test = std::make_unique<TestInherit>(bus, objPath,
+ TestInherit::action::defer_emit);
+
+ EXPECT_CALL(sdbusMock, sd_bus_emit_object_added(_, StrEq(objPath)))
+ .Times(1);
+ EXPECT_CALL(sdbusMock,
+ sd_bus_emit_interfaces_added_strv(_, StrEq(objPath), _))
+ .Times(0);
+
+ // Now invoke emit_object_added()
+ test->emit_object_added();
+
+ // After destruction, the object will be removed
+ EXPECT_CALL(sdbusMock, sd_bus_emit_object_removed(_, StrEq(objPath)))
+ .Times(1);
+ EXPECT_CALL(sdbusMock,
+ sd_bus_emit_interfaces_removed_strv(_, StrEq(objPath), _))
+ .Times(0);
+}
+
+TEST_F(Object, ObjectAdded)
+{
+ // Simulate the typical usage of a service
+ sdbusplus::server::manager::manager objManager(bus, objPath);
+ bus.request_name(busName);
+
+ EXPECT_CALL(sdbusMock, sd_bus_emit_object_added(_, StrEq(objPath)))
+ .Times(1);
+ EXPECT_CALL(sdbusMock,
+ sd_bus_emit_interfaces_added_strv(_, StrEq(objPath), _))
+ .Times(0);
+
+ // Note: this is the wrong usage!
+ // With the below code, the interface is added as expected, but after
+ // destruction of TestInherit, the whole object will be removed from DBus.
+ // Refer to InterfaceAddedOnly case for the correct usage.
+ auto test = std::make_unique<TestInherit>(bus, objPath);
+
+ EXPECT_CALL(sdbusMock, sd_bus_emit_object_removed(_, StrEq(objPath)))
+ .Times(1);
+ EXPECT_CALL(sdbusMock,
+ sd_bus_emit_interfaces_removed_strv(_, StrEq(objPath), _))
+ .Times(0);
+}
diff --git a/tools/sdbusplus/templates/interface.mako.server.hpp b/tools/sdbusplus/templates/interface.mako.server.hpp
index 99ba769..1825d6b 100644
--- a/tools/sdbusplus/templates/interface.mako.server.hpp
+++ b/tools/sdbusplus/templates/interface.mako.server.hpp
@@ -121,6 +121,18 @@
static ${e.name} convert${e.name}FromString(const std::string& s);
% endfor
+ /** @brief Emit interface added */
+ void emit_added()
+ {
+ _${"_".join(interface.name.split('.'))}_interface.emit_added();
+ }
+
+ /** @brief Emit interface removed */
+ void emit_removed()
+ {
+ _${"_".join(interface.name.split('.'))}_interface.emit_removed();
+ }
+
static constexpr auto interface = "${interface.name}";
private: