server: object: default emit destruct signals
In the mailing list discussion [1] it was observed that there were cases
where daemons like `phosphor-logging` do not end up emitting
InterfacesRemoved signals even when the holding sdbusplus::object_t was
destructed.
After investigation, we determined that there are valid use cases for not
calling `object->emit_added()` but still wanting the removal signals to be
emitted. Specifically, the case of a daemon creating initial object prior
to claiming a bus-name it is good to omit the emission.
I examined all cases where callers were using `action::defer_emit` and
it was acceptable for all of them to later send the InterfacesRemoved
signals on destruction. To enable the old behavior, if desired, I added
a new enumeration which is `action::emit_no_signals`.
1. https://lore.kernel.org/openbmc/CAGm54UHMED4Np0MThLfp4H-i8R24o8pCns2-6MEzy1Me-9XJmA@mail.gmail.com/
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Change-Id: I70bd5d5f2ecc676bdc9475e8a2a27e02c52d9142
diff --git a/include/sdbusplus/server/object.hpp b/include/sdbusplus/server/object.hpp
index 7672e2e..bef4010 100644
--- a/include/sdbusplus/server/object.hpp
+++ b/include/sdbusplus/server/object.hpp
@@ -177,9 +177,14 @@
enum class action
{
+ /** sd_bus_emit_object_{added, removed} */
emit_object_added,
+ /** sd_bus_emit_interfaces_{added, removed} */
emit_interface_added,
+ /** no automatic added signal, but sd_bus_emit_object_removed on
+ * destruct */
defer_emit,
+ /** no interface signals */
emit_no_signals,
};
@@ -187,17 +192,13 @@
*
* @param[in] bus - The bus to place the object on.
* @param[in] path - The path the object resides at.
- * @param[in] deferSignal - Set to true if emit_object_added should be
- * deferred. This would likely be true if the
- * object needs custom property init before the
- * signal can be sent.
+ * @param[in] act - Set to the desired InterfacesAdded signal behavior.
*/
object(bus_t& bus, const char* path,
action act = action::emit_object_added) :
details::compose<Args...>(bus, path),
__sdbusplus_server_object_bus(get_busp(bus), bus.getInterface()),
__sdbusplus_server_object_path(path),
- __sdbusplus_server_object_emitremoved(false),
__sdbusplus_server_object_intf(bus.getInterface())
{
// Default ctor
@@ -213,7 +214,7 @@
~object()
{
- if (__sdbusplus_server_object_emitremoved)
+ if (__sdbusplus_server_object_signalstate != action::emit_no_signals)
{
__sdbusplus_server_object_intf->sd_bus_emit_object_removed(
get_busp(__sdbusplus_server_object_bus),
@@ -224,12 +225,12 @@
/** Emit the 'object-added' signal, if not already sent. */
void emit_object_added()
{
- if (!__sdbusplus_server_object_emitremoved)
+ if (__sdbusplus_server_object_signalstate == action::defer_emit)
{
__sdbusplus_server_object_intf->sd_bus_emit_object_added(
get_busp(__sdbusplus_server_object_bus),
__sdbusplus_server_object_path.c_str());
- __sdbusplus_server_object_emitremoved = true;
+ __sdbusplus_server_object_signalstate = action::emit_object_added;
}
}
@@ -240,21 +241,36 @@
// ambiguity.
bus_t __sdbusplus_server_object_bus;
std::string __sdbusplus_server_object_path;
- bool __sdbusplus_server_object_emitremoved;
+ action __sdbusplus_server_object_signalstate = action::defer_emit;
SdBusInterface* __sdbusplus_server_object_intf;
/** Check and run the action */
void check_action(action act)
{
- if (act == action::emit_object_added)
+ switch (act)
{
- emit_object_added();
+ case action::emit_object_added:
+ // We are wanting to call emit_object_added to set up in
+ // deferred state temporarily and then emit the signal.
+ __sdbusplus_server_object_signalstate = action::defer_emit;
+ emit_object_added();
+ break;
+
+ case action::emit_interface_added:
+ details::compose<Args...>::maybe_emit_iface_added();
+ // If we are emitting at an interface level, we should never
+ // also emit at the object level.
+ __sdbusplus_server_object_signalstate = action::emit_no_signals;
+ break;
+
+ case action::defer_emit:
+ __sdbusplus_server_object_signalstate = action::defer_emit;
+ break;
+
+ case action::emit_no_signals:
+ __sdbusplus_server_object_signalstate = action::emit_no_signals;
+ break;
}
- else if (act == action::emit_interface_added)
- {
- details::compose<Args...>::maybe_emit_iface_added();
- }
- // Otherwise, do nothing
}
};
diff --git a/test/server/object.cpp b/test/server/object.cpp
index c541e5b..afb7ad0 100644
--- a/test/server/object.cpp
+++ b/test/server/object.cpp
@@ -83,7 +83,6 @@
// 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,
@@ -91,6 +90,36 @@
.Times(0);
}
+TEST_F(Object, NeverAddInterface)
+{
+ // Simulate the typical usage of a service
+ sdbusplus::server::manager_t 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::emit_no_signals);
+
+ 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);
+
+ // After destruction, the object will 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(0);
+}
+
TEST_F(Object, ObjectAdded)
{
// Simulate the typical usage of a service