Clean up ASIO set-property callback code
This reverts commit ce62764d6c0a72ec88eb433e1afd05b751225971, which
introduced inconsistent semantics for the underlying ASIO set handler
functions. The "external" `call` case expected the handler to return an
integer error code, while the "internal" `set` case expected the handler
to return a boolean status.
This also does a small refactor on the two similar `call` and `set`
functions to deduplicate, and adds ASIO.md to document the expected
semantics of the ASIO handlers.
The vast majority of user code in OpenBMC is still returning integers
from the callbacks (0/1/-1), but in most cases 0 was used to mean
failure, and 1 was used to mean success, so formalizing that the
callback should return a boolean value will not break things.
Tested: With accompanying dbus-sensors change
I05c41318954d5d1549752c5ef2c227f3f22a45d3, using steps in that commit
msg.
Signed-off-by: Jonathan Doman <jonathan.doman@intel.com>
Change-Id: Ic3a36027fa95a77469439b296a1497634cfe030f
diff --git a/docs/asio.md b/docs/asio.md
new file mode 100644
index 0000000..b215d9e
--- /dev/null
+++ b/docs/asio.md
@@ -0,0 +1,54 @@
+# ASIO
+
+## Properties
+
+Properties can be dynamically registered to an interface by calling
+`register_property()`. If you need to provide a custom getter or setter for a
+property, it should follow the guidelines in this section.
+
+To return a D-Bus error reply for either GetProperty or SetProperty, throw a
+custom exception derived from `sdbusplus::exception_t`. For the custom
+exception, you can return a well-defined org.freedesktop.DBus.Error
+(https://www.freedesktop.org/software/systemd/man/sd-bus-errors.html#) from the
+`name()` or a custom/arbitrary name. The former will be automatically translated
+into a matching error code
+(https://github.com/systemd/systemd/blob/485c9e19e7ebcd912d5fbf11f40afc62951173f8/src/libsystemd/sd-bus/bus-error.c)
+that can be consumed by the caller, while the latter will always be mapped to
+`EIO`, requiring a `strcmp` to determine the exact error name.
+
+The handler may also throw any exception not derived from
+`sdbusplus::exception_t`, in which case a generic
+org.freedesktop.DBus.Error.InvalidArgs error will be returned to the caller.
+
+### Get Property callback
+
+When registering a property using ASIO methods, the get callback should meet the
+following prototype:
+
+```c++
+PropertyType getHandler(const PropertyType& value);
+```
+
+The first argument is the current value cached in the D-Bus interface (i.e. the
+previously "getted" value, or the initialized value), and the return value is
+returned to the D-Bus caller.
+
+### Set Property callback
+
+When registering a writable property using ASIO methods, the set callback should
+meet the following prototype:
+
+```c++
+bool setHandler(const PropertyType& newValue, PropertyType& value);
+```
+
+The first argument is the new value requested to be set by the D-Bus caller.
+
+The second argument is the actual underlying value contained within the object
+server. If the new value meets the expected constraints, the handler must set
+`value = newValue` to make the set operation take effect and return true. If the
+new value is invalid or cannot be applied for whatever reason, the handler must
+leave `value` unmodified and return false.
+
+If the handler returns false but doesn't throw an exception, a generic
+org.freedesktop.DBus.Error.InvalidArgs error will be returned to the caller.
diff --git a/example/asio-example.cpp b/example/asio-example.cpp
index f113dd3..a83ffea 100644
--- a/example/asio-example.cpp
+++ b/example/asio-example.cpp
@@ -219,17 +219,17 @@
[](const int& req, int& propertyValue) {
if (req >= 50)
{
- return -EINVAL;
+ return false;
}
propertyValue = req;
- return 1; // success
+ return true;
});
iface->register_property(
"TrailTime", std::string("foo"),
// custom set
[](const std::string& req, std::string& propertyValue) {
propertyValue = req;
- return 1; // success
+ return true;
},
// custom get
[](const std::string& property) {
diff --git a/example/get-all-properties.cpp b/example/get-all-properties.cpp
index 8b01258..2f1e908 100644
--- a/example/get-all-properties.cpp
+++ b/example/get-all-properties.cpp
@@ -33,7 +33,7 @@
propertyGoodbyesName, sdbusplus::vtable::property_::emits_change,
[this](const auto& newPropertyValue, const auto&) {
goodbyes_ = newPropertyValue;
- return 1;
+ return true;
},
[this](const auto&) { return goodbyes_; });
diff --git a/example/register-property.cpp b/example/register-property.cpp
index 339c0ce..2c3e362 100644
--- a/example/register-property.cpp
+++ b/example/register-property.cpp
@@ -32,7 +32,7 @@
sdbusplus::vtable::property_::emits_change,
[this](const auto& newPropertyValue, const auto&) {
goodbyes_ = newPropertyValue;
- return 1;
+ return true;
},
[this](const auto&) { return goodbyes_; });
});
diff --git a/include/sdbusplus/asio/object_server.hpp b/include/sdbusplus/asio/object_server.hpp
index f6db20a..9aed002 100644
--- a/include/sdbusplus/asio/object_server.hpp
+++ b/include/sdbusplus/asio/object_server.hpp
@@ -19,7 +19,6 @@
#include <list>
#include <optional>
#include <set>
-#include <tuple>
#include <unordered_map>
namespace sdbusplus
@@ -45,7 +44,7 @@
{
public:
virtual ~callback_set() = default;
- virtual std::tuple<SetPropertyReturnValue, int> call(message_t& m) = 0;
+ virtual SetPropertyReturnValue call(message_t& m) = 0;
virtual SetPropertyReturnValue set(const std::any& value) = 0;
};
@@ -293,28 +292,22 @@
value_(value),
func_(std::move(func))
{}
- std::tuple<SetPropertyReturnValue, int> call(message_t& m) override
+ SetPropertyReturnValue call(message_t& m) override
{
PropertyType input;
m.read(input);
-
- auto oldValue = *value_;
- auto ret = func_(input, *value_);
-
- if (ret < 0)
- {
- return {SetPropertyReturnValue::fail, ret};
- }
- if (oldValue == *value_)
- {
- return {SetPropertyReturnValue::sameValueUpdated, ret};
- }
- return {SetPropertyReturnValue::valueUpdated, ret};
+ return set_(input);
}
SetPropertyReturnValue set(const std::any& value) override
{
- auto oldValue = *value_;
- if (func_(std::any_cast<PropertyType>(value), *value_))
+ return set_(std::any_cast<PropertyType>(value));
+ }
+
+ private:
+ SetPropertyReturnValue set_(const PropertyType& newValue)
+ {
+ PropertyType oldValue = *value_;
+ if (func_(newValue, *value_))
{
if (oldValue == *value_)
{
@@ -465,7 +458,7 @@
name, property, vtable::property_::emits_change,
[](const PropertyType& req, PropertyType& old) {
old = req;
- return 1;
+ return true;
},
[](const PropertyType& value) { return value; });
}
@@ -631,26 +624,18 @@
try
{
#endif
- auto [status, rc] = func->second->call(mesg);
-
- switch (status)
+ SetPropertyReturnValue status = func->second->call(mesg);
+ if ((status == SetPropertyReturnValue::valueUpdated) ||
+ (status == SetPropertyReturnValue::sameValueUpdated))
{
- case SetPropertyReturnValue::sameValueUpdated:
- {
- return true;
- }
- case SetPropertyReturnValue::valueUpdated:
+ if (status != SetPropertyReturnValue::sameValueUpdated)
{
data->signal_property(property);
- return true;
}
- case SetPropertyReturnValue::fail:
- {
- sd_bus_error_set_errno(error, rc);
- return false;
- }
+ // There shouldn't be any other callbacks that want to
+ // handle the message so just return a positive integer.
+ return 1;
}
-
#ifdef __EXCEPTIONS
}