sdbusplus

* sdbusplus - const/readonly flags
@ 2020-08-25 15:00 Patrick Williams
  2020-08-25 17:50 ` Matthew Barth
  2020-08-25 20:52 ` Adriana Kobylak
  0 siblings, 2 replies; 5+ messages in thread
From: Patrick Williams @ 2020-08-25 15:00 UTC (permalink / raw)
  To: OpenBMC List

[-- Attachment #1: Type: text/plain, Size: 4174 bytes --]

Hello,

I thought I sent an email to the list on this 1.5 months ago, but I
didn't receive any feedback.  It seems like my email never made it to
the list.  So, here is another attempt.

---

TL;DR: The sdbus++ attribute flag 'const' was incorrectly documented and a
new flag 'readonly' is now available.  Some phosphor-dbus-interfaces
might be implemented incorrectly.

ALSO: I could really use the help of the domain experts for the
phosphor-dbus-interfaces listed in the gist[4] to review and determine
if 'const' or 'readonly' is more appropriate.

---

For the sdbus++ interface YAML files we have some flags that can be
added to attributes.  These flags correspond to flags available in the
underlying sdbus vtable functions[1].  Through investigation of
openbmc/sdbusplus#48[2] I came to realize that our documentation of
'const' did not match what SD_BUS_VTABLE_PROPERTY_CONST meant.

The old documentation said:

    The flag `const` makes the property read-only via D-Bus but still
    writable by the app implementing it.

The PROPERTY_CONST says:

    PROPERTY_CONST corresponds to const and means that the property never
    changes during the lifetime of the object it belongs to, so no signal
    needs to be emitted.

The words are quite different.  To deal with this I have done two
things[3]:

   a. Fix the documentation of 'const' to match PROPERTY_CONST's
      intention.
   b. Add a new flag 'readonly' to match the previously documented
      behavior.

The new documentation reads:

    Both `const` and `readonly` prevent D-Bus clients from being able to
    write to a property.  `const` is a D-Bus indication that the property
    can never change, while `readonly` properties can be changed by the D-Bus
    server itself.  As examples, the `Version` property on a software object
    might be appropriate to be `const` and the `Value` property on a sensor
    object would likely be `readonly`.

You might ask why I didn't fix 'const' to match the documentation.  I
chose to change the documentation instead for two reasons.  First, the
code using these flags has been tested, so it is a less risky change to
simply update the human-read documentation.  Second, we were always
calling a near identical-named sdbus macro, so it is more intuitive to
have matching behavior for matching names.

What does this mean for us?  A few thoughts.

   * I expect some of the 'const' flags in phosphor-dbus-interfaces are
     wrong and should be changed to 'readonly'.  I have collected a list
     of them in a gist[4].  If you really intend to mean "this property
     will never change during the life of an object" continue to use
     'const', but if you mean "this property shall not be changed by
     clients", use 'readonly' (and probably also add 'emits_change').

   * Implementations should really be careful using 'const' because the
     default behavior of sdbusplus/server/object.hpp is
     'emit_object_added' in the constructor, but the 'const' properties
     themselves are likely set later which is a violation of the
     'PROPERTY_CONST' documentation.  A process could listen for the
     ObjectAdded signal and use the results of that to cache your
     'const' properties, which haven't been fully initialized yet, and a
     later PropertyChanged signal will never come when the real property has
     been initialized.

   * We don't currently have code in sdbus++ generated servers to
     prevent changing 'const' properties after the ObjectAdded or
     InterfaceAdded signal has been sent.  This could be added at a
     later time to try to catch these cases.

Thank you for reading this far!  Like usual, reach out if I've messed
something up or what I've written above is not clear.

1. https://manpages.debian.org/experimental/libsystemd-dev/SD_BUS_WRITABLE_PROPERTY.3.en.html#Flags
2. https://github.com/openbmc/sdbusplus/issues/48
3. https://github.com/openbmc/sdbusplus/commit/e1c73d3bf8f6cabc1c58f67a233dba55b6f76d74
4. https://gist.github.com/williamspatrick/fa975c33f00640ca54a7aa246bbbfeb9

-- 
Patrick Williams

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread