DBus ObjectManager Interface usages within the project

[Ed Tanous]

On Tue, Jul 12, 2022 at 8:07 PM Lei Yu <yulei.sh at bytedance.com> wrote:
>
> On Wed, Jul 13, 2022 at 2:49 AM Ed Tanous <edtanous at google.com> wrote:
> >
> > We've had a couple cases in the project where patches have needed to be slowed down because of inconsistencies in our use of object manager, so IMO, the time has come to make our usage of ObjectManager consistent, documented, and drop support for the (not upstream) daemons that don't follow the rules.  As part of this, we will port forward all the upstream daemons to be consistent, and do our best to avoid bugs, but this email is intended to inform those that have non-upstream daemons about the change so they can resolve their implementations.
> >
> > The basics:
> > ObjectManager DBus interface will now be required for any daemon implementing a Sensor.Value interface or Inventory.Item interface.
> >
> > Daemons producing sensors will be required to place their ObjectManager at /xyz/openbmc_project/sensors
> > Daemons producing inventory items will be required to place their ObjectManager at /xyz/openbmc_project/inventory.
> >
> > Both of these interfaces will be required to be published before claiming a well known name on dbus, to allow for the possibility of caching implementations at some point in future.
> >
> > Functionally, this gets a little complicated because the sdbusplus::asio::object_manager bindings in their default invocation create an ObjectManager at the root object /, so we need to execute this in a few steps.
> >
> > 0. Send this email out, outlining the problem, and warn the community that if this is an assumption you make in your downstream daemons, those assumptions will need to change.  Get consensus from maintainers.
> > 1. Update the phosphor-dbus-interfaces documentation for both Sensor.Value and Inventory.Item to call out the required-ness of ObjectManager, and the explicit dbus path where it's required to be placed.
> > 2. Identify all the asio daemons that need changes, and publish changes that move the object_manager to the appropriate path.  This is mostly going to be a tree-wide grep for sdbusplus::asio::object_server, and look for daemonst that don't make use of the add_manager() API.  Anyone directly calling the Sensor or Invertory ObjectManager interfaces will need to port to the new paths, but luckily, this isn't a very common operation, and I beleive bmcweb and phosphor-ipmid-host might be the only direct users.  In bmcweb, there is actually a convoluted piece of code that uses the mapper to sort out the location that the ObjectManager exists at so Redfish sensors should remain consistent, and we don't yet have code that relies on ObjectManager for Inventory items.  I believe phosphor-ipmi-host has a similar piece of code that should protect us in this case, but I defer to those maintainers.
>
> Is it better for the asio API to provide an extra constructor to take
> a object_path?
>
> E.g.
>  object_server(const std::shared_ptr<sdbusplus::asio::connection>&
> conn, const std::string& path)
>
> Then the users could initialize its object_server at expected path directly:
>
>  sdbusplus::asio::object_server objectServer(bus, "/my/path");
>
> instead of two lines of code like:
>
>  sdbusplus::asio::object_server objectServer(bus, true);
>  objectServer.add_manager("/my/path");

I like the thinking, but considering that ObjectPath is not required
in all cases, and there might be cases where they have multiple (if a
daemon supported sensors and inventory) I don't think we can have it
in the constructor, or at the very least we'd have to support the
non-path constructor as well, which basically amounts to the same as
we have (it would just call add_manager internally).  I don't think
the one line of code savings is worth it in that case.

>
> --
> BRs,
> Lei YU