Missed interfacesRemoved signal callbacks in object-mapper results in invalidated DBus objects

[Patrick Williams]

On Sat, Apr 02, 2022 at 11:33:57AM +0800, Lei Yu wrote:

> Thanks! It's a nice article and clarifies the different cases.

Thank you.

> But I still have a question about `Permanent Sub-Objects`:

> > We never want the sub-object to deal with its own lifetime signals! If we
> > did they’d also be the lifetime signals for the parent object because they 
> > are all residing at the same dbus object-path
> 
> For example the `Value` and `Critical` objects in phosphor-hwmon:
> 1. when it's destructed, it will emit the interfacesRemoved signal for
> the `Value` interface, but no such signal for the `Critical`
> interface, is it? I do not think it's expected behavior.

No, this is not the case.  Value _will_ emit the interfacesRemoved for
Threshold::Critical.  This is because Value (as the primary object) is
using emit_object_added() to signal the InterfacesAdded and it will use
emit_object_removed() in the destructor to signal the InterfacesRemoved.

I also wrote this:

    The systemd sd-bus API provides the following functions:

       * sd_bus_emit_interfaces_added
       * sd_bus_emit_interfaces_removed
       * sd_bus_emit_object_added
       * sd_bus_emit_object_removed

    The interface-level functions take a list of interfaces and create the
    corresponding Interfaces{Added,Removed} signal. The object-level functions
    are simply helper functions which create the signals for all interfaces the
    sd-bus ObjectManager is aware of having resided at that object-path.

The object-level functions have nothing to do with the interface types inside
the `object_t<...>`.  They send signals for *all* interfaces on the same
object path.  From the manpage:

       sd_bus_emit_object_added() and sd_bus_emit_object_removed() are convenience functions for emitting the
       InterfacesAdded or InterfacesRemoved signals for all interfaces registered on a specific object path,
       respectively. This includes any parent fallback vtables if they are not overridden by a more applicable
       child vtable. It also includes all the standard D-Bus interfaces implemented by sd-bus itself on any
       registered object.

Notice the "all interfaces registered on a specific object path" and
"includes all the standard D-Bus interfaces implemented by sd-bus
itself".  That means interfaces like `org.freedesktop.DBus.Properties`
even though those aren't part of the `object_t<...>` directly.

> 2. It has the assumption that the service needs to know which is the
> permanent `main` object, and which are the `sub` objects. What if the
> service could not tell which is the main and which is the sub? I think
> this makes it harder for sdbusplus' users to design the object
> hierarchy, is it?

I've been doing the conversions and I've done half a dozen so far and I
haven't really seen a case where it is a challenge to identify.  We
almost always have a `std::optional` or `std::unique_ptr` holding the
sub-object, which isn't constructed in 100% of the code paths.  If the
sub-object _were_ constructed 100% of the time, it would have just been
included in the original `object_t<...>`.

If you really have a case where you create interface X 50% of the time
and interface Y 50% of the time (and 25% of the time both), then neither
of them are the primary object and you'll probably need to use
`emit_interfaces_added` instead.  I haven't run across code that does
this yet and I can't really come up with a scenario either, so I didn't
document it in the article.

-- 
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20220402/ccb5f3b8/attachment.sig>