sdbusplus - const/readonly flags

[Matthew Barth]

On 8/25/20 10:00 AM, Patrick Williams wrote:
> 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.

 From the info you provided, sounds like the ThermalMode interface's 
Supported property needs to be updated to "readonly" as there may be a 
reason where the supported thermal modes are changed by the server of 
the interface due to machine configuration differences.

>
> ---
>
> 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.

Are there any code update implications after these interfaces are 
changed from const to readonly that would require code changes by the 
server code implementing them?

>     * 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
>