object: handle diamond inheritance
Commit 664922157bbbd14f0ad1692cee5547f60f6c617c added an inheritance
to `server::object` to gain access to the bus-pointer. This was
observed to cause a compile failure in some applications which had a
diamond-inheritance structure due to a nested `object` inheritance:
`object<object<iface0, iface1>, object<iface2, iface3>>`
These clients probably should not have attempted a nested/diamond
because the previous implementation would have resulted in a silent
failure to make `action::emit_interface_added` work properly (since
object itself doesn't have an `emit_added` function).
Improve the `object` so that:
- Diamond inheritance is no longer possible with nested inherits.
- The `action::emit_interface_added` action works properly with
nested inherits.
This required adding some template specialization to determine if one of
the type arguments to `object` were nested (ie.
`object<Args0..., object<Args1...>, Args2...>`) and collapse out the
nested `object` inheritance (so that the example acts exactly the same
as `object<Args0..., Args1..., Args2...>`).
Tested: Enhance the existing `object::emit` test cases to operate on a
nested `object` structure.
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Change-Id: I10635a256aafbdc1e42439a49fa61127e06c8e36
diff --git a/include/sdbusplus/server/object.hpp b/include/sdbusplus/server/object.hpp
index f258435..2e5652a 100644
--- a/include/sdbusplus/server/object.hpp
+++ b/include/sdbusplus/server/object.hpp
@@ -14,27 +14,72 @@
namespace object
{
+// Forward declaration.
+template <class... Args>
+struct object;
+
namespace details
{
-/** Helper functions to detect if member exists in a class */
+// Forward declaration.
+template <class... Args>
+struct compose;
-/** Test if emit_added() exists in T return std::true_type */
+/** Template to identify if an inheritance is a "normal" type or a nested
+ * sdbusplus::object.
+ */
template <class T>
-constexpr auto has_emit_added_helper(int)
- -> decltype(std::declval<T>().emit_added(), std::true_type{});
+struct compose_inherit
+{
+ // This is a normal type.
+ using type = T;
-/** If the above test fails, fall back to this to return std::false_type */
-template <class>
-constexpr std::false_type has_emit_added_helper(...);
+ // Normal types need emit_added called, if possible.
+ template <class S>
+ static void maybe_emit_iface_added(S* obj)
+ {
+ if constexpr (has_emit_added<S>())
+ {
+ obj->S::emit_added();
+ }
+ }
-/** 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));
+ // Test if emit_added() exists in T return std::true_type.
+ template <class S>
+ static constexpr auto has_emit_added_helper(int)
+ -> decltype(std::declval<S>().emit_added(), std::true_type{});
+
+ // If the above test fails, fall back to this to return std::false_type
+ template <class>
+ static 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 S>
+ using has_emit_added = decltype(has_emit_added_helper<S>(0));
+};
+
+/** Template specialization for the sdbusplus::object nesting. */
+template <class... Args>
+struct compose_inherit<object<Args...>>
+{
+ // Unravel the inheritance with a recursive compose.
+ using type = compose<Args...>;
+
+ // Redirect the interface added to the composed parts.
+ template <class T>
+ static void maybe_emit_iface_added(T* obj)
+ {
+ obj->compose<Args...>::maybe_emit_iface_added();
+ }
+};
+
+/** std-style _t alias for compose_inherit. */
+template <class... Args>
+using compose_inherit_t = typename compose_inherit<Args...>::type;
/** Templates to allow multiple inheritance via template parameters.
*
@@ -42,19 +87,32 @@
* single class.
*/
template <class T, class... Rest>
-struct compose_impl : T, compose_impl<Rest...>
+struct compose_impl : compose_inherit_t<T>, compose_impl<Rest...>
{
compose_impl(bus_t& bus, const char* path) :
T(bus, path), compose_impl<Rest...>(bus, path)
{}
+
+ void maybe_emit_iface_added()
+ {
+ compose_inherit<T>::maybe_emit_iface_added(
+ static_cast<compose_inherit_t<T>*>(this));
+ compose_impl<Rest...>::maybe_emit_iface_added();
+ }
};
/** Specialization for single element. */
template <class T>
-struct compose_impl<T> : T
+struct compose_impl<T> : compose_inherit_t<T>
{
- compose_impl(bus_t& bus, const char* path) : T(bus, path)
+ compose_impl(bus_t& bus, const char* path) : compose_inherit_t<T>(bus, path)
{}
+
+ void maybe_emit_iface_added()
+ {
+ compose_inherit<T>::maybe_emit_iface_added(
+ static_cast<compose_inherit_t<T>*>(this));
+ }
};
/** Default compose operation for variadic arguments. */
@@ -63,6 +121,14 @@
{
compose(bus_t& bus, const char* path) : compose_impl<Args...>(bus, path)
{}
+
+ friend struct compose_inherit<object<Args...>>;
+
+ protected:
+ void maybe_emit_iface_added()
+ {
+ compose_impl<Args...>::maybe_emit_iface_added();
+ };
};
/** Specialization for zero variadic arguments. */
@@ -71,6 +137,9 @@
{
compose(bus_t& /*bus*/, const char* /*path*/)
{}
+
+ protected:
+ void maybe_emit_iface_added(){};
};
} // namespace details
@@ -165,7 +234,7 @@
private:
// These member names are purposefully chosen as long and, hopefully,
- // unique. Since an object is 'composed' via multiple-inheritence,
+ // unique. Since an object is 'composed' via multiple-inheritance,
// all members need to have unique names to ensure there is no
// ambiguity.
bus_t __sdbusplus_server_object_bus;
@@ -173,16 +242,6 @@
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)
{
@@ -192,7 +251,7 @@
}
else if (act == action::emit_interface_added)
{
- (maybe_emit<Args>(), ...);
+ details::compose<Args...>::maybe_emit_iface_added();
}
// Otherwise, do nothing
}
diff --git a/test/server/object.cpp b/test/server/object.cpp
index b6bd9a8..c541e5b 100644
--- a/test/server/object.cpp
+++ b/test/server/object.cpp
@@ -9,8 +9,20 @@
using ::testing::_;
using ::testing::StrEq;
-using TestInherit =
- sdbusplus::server::object_t<sdbusplus::server::server::Test>;
+struct UselessInherit
+{
+ template <typename... Args>
+ explicit UselessInherit(Args&&...)
+ {}
+};
+
+// It isn't a particularly good idea to inherit from object_t twice, but some
+// clients seem to do it. Do it here to ensure that code compiles (avoiding
+// diamond-inheritance problems) and that emit happens correctly when it is
+// done.
+using TestInherit = sdbusplus::server::object_t<
+ UselessInherit,
+ sdbusplus::server::object_t<sdbusplus::server::server::Test>>;
class Object : public ::testing::Test
{