sdbusplus - const/readonly flags
msbarth at linux.ibm.com
Wed Aug 26 03:50:06 AEST 2020
On 8/25/20 10:00 AM, Patrick Williams wrote:
> 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 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. Through investigation of
> openbmc/sdbusplus#48 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
> a. Fix the documentation of 'const' to match PROPERTY_CONST's
> b. Add a new flag 'readonly' to match the previously documented
> 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. 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
More information about the openbmc