Create Redfish specific setProperty call
There are currently 78 sdbusplus::asio::setProperty calls in
redfish-core. The error handler for nearly all of them looks something
like:
```
if (ec)
{
const sd_bus_error* dbusError = msg.get_error();
if ((dbusError != nullptr) &&
(dbusError->name ==
std::string_view(
"xyz.openbmc_project.Common.Error.InvalidArgument")))
{
BMCWEB_LOG_WARNING("DBUS response error: {}", ec);
messages::propertyValueIncorrect(asyncResp->res, "<PropertyName>", <PropertyValue>);
return;
}
messages::internalError(asyncResp->res);
return;
}
messages::success(asyncResp->res);
```
In some cases there are more errors handled that translate to more error
messages, but the vast majority only handle InvalidArgument. Many of
these, like the ones in account_service.hpp, do the error handling in a
lambda, which causes readability problems. This commit starts to make
things more consistent, and easier for trivial property sets.
This commit invents a setDbusProperty method in the redfish namespace
that tries to handle all DBus errors in a consistent manner. Looking
for input on whether this will work before changing over the other 73
calls. Overall this is less code, fewer inline lambdas, and defaults
that should work for MOST use cases of calling an OpenBMC daemon, and
fall back to more generic errors when calling a "normal" dbus daemon.
As part of this, I've ported over several examples. Some things that
might be up in the air:
1. Do we always return 204 no_content on property sets? Today there's a
mix of 200, with a Base::Success message, and 204, with an empty body.
2. Do all DBus response codes map to the same error? A majority are
covered by xyz.openbmc_project.Common.Error.InvalidArgument, but there
are likely differences. If we allow any daemon to return any return
code, does that cause compatibility problems later?
Tested:
```
curl -k --user "root:0penBmc" -H "Content-Type: application/json" -X PATCH -d '{"HostName":"openbmc@#"}' https://192.168.7.2/redfish/v1/Managers/bmc/EthernetInterfaces/eth0
```
Returns the appropriate error in the response
Base.1.16.0.PropertyValueIncorrect
Change-Id: If033a1112ba516792c9386c997d090c8f9094f3a
Signed-off-by: Ed Tanous <ed@tanous.net>
diff --git a/redfish-core/lib/account_service.hpp b/redfish-core/lib/account_service.hpp
index 23f1616..edf3cf7 100644
--- a/redfish-core/lib/account_service.hpp
+++ b/redfish-core/lib/account_service.hpp
@@ -258,19 +258,9 @@
// logged.
return;
}
-
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, "xyz.openbmc_project.User.Manager",
- dbusObjectPath, "xyz.openbmc_project.User.Attributes", "UserGroups",
- updatedUserGroups, [asyncResp](const boost::system::error_code& ec) {
- if (ec)
- {
- BMCWEB_LOG_ERROR("D-Bus responses error: {}", ec);
- messages::internalError(asyncResp->res);
- return;
- }
- messages::success(asyncResp->res);
- });
+ setDbusProperty(asyncResp, "xyz.openbmc_project.User.Manager",
+ dbusObjectPath, "xyz.openbmc_project.User.Attributes",
+ "UserGroups", "AccountTypes", updatedUserGroups);
}
inline void userErrorMessageHandler(
@@ -433,70 +423,23 @@
// If "RemoteGroup" info is provided
if (remoteGroup)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, ldapDbusService,
- roleMapObjData[index].first,
+ setDbusProperty(
+ asyncResp, ldapDbusService, roleMapObjData[index].first,
"xyz.openbmc_project.User.PrivilegeMapperEntry",
- "GroupName", *remoteGroup,
- [asyncResp, roleMapObjData, serverType, index,
- remoteGroup](const boost::system::error_code& ec,
- const sdbusplus::message_t& msg) {
- if (ec)
- {
- const sd_bus_error* dbusError = msg.get_error();
- if ((dbusError != nullptr) &&
- (dbusError->name ==
- std::string_view(
- "xyz.openbmc_project.Common.Error.InvalidArgument")))
- {
- BMCWEB_LOG_WARNING("DBUS response error: {}",
- ec);
- messages::propertyValueIncorrect(asyncResp->res,
- "RemoteGroup",
- *remoteGroup);
- return;
- }
- messages::internalError(asyncResp->res);
- return;
- }
- asyncResp->res
- .jsonValue[serverType]["RemoteRoleMapping"][index]
- ["RemoteGroup"] = *remoteGroup;
- });
+ "GroupName",
+ std::format("RemoteRoleMapping/{}/RemoteGroup", index),
+ *remoteGroup);
}
// If "LocalRole" info is provided
if (localRole)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, ldapDbusService,
- roleMapObjData[index].first,
+ setDbusProperty(
+ asyncResp, ldapDbusService, roleMapObjData[index].first,
"xyz.openbmc_project.User.PrivilegeMapperEntry",
- "Privilege", *localRole,
- [asyncResp, roleMapObjData, serverType, index,
- localRole](const boost::system::error_code& ec,
- const sdbusplus::message_t& msg) {
- if (ec)
- {
- const sd_bus_error* dbusError = msg.get_error();
- if ((dbusError != nullptr) &&
- (dbusError->name ==
- std::string_view(
- "xyz.openbmc_project.Common.Error.InvalidArgument")))
- {
- BMCWEB_LOG_WARNING("DBUS response error: {}",
- ec);
- messages::propertyValueIncorrect(
- asyncResp->res, "LocalRole", *localRole);
- return;
- }
- messages::internalError(asyncResp->res);
- return;
- }
- asyncResp->res
- .jsonValue[serverType]["RemoteRoleMapping"][index]
- ["LocalRole"] = *localRole;
- });
+ "Privilege",
+ std::format("RemoteRoleMapping/{}/LocalRole", index),
+ *localRole);
}
}
// Create a new RoleMapping Object.
@@ -805,40 +748,10 @@
const std::string& ldapServerElementName,
const std::string& ldapConfigObject)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, ldapDbusService, ldapConfigObject,
- ldapConfigInterface, "LDAPServerURI", serviceAddressList.front(),
- [asyncResp, ldapServerElementName, serviceAddressList](
- const boost::system::error_code& ec, sdbusplus::message_t& msg) {
- if (ec)
- {
- const sd_bus_error* dbusError = msg.get_error();
- if ((dbusError != nullptr) &&
- (dbusError->name ==
- std::string_view(
- "xyz.openbmc_project.Common.Error.InvalidArgument")))
- {
- BMCWEB_LOG_WARNING(
- "Error Occurred in updating the service address");
- messages::propertyValueIncorrect(asyncResp->res,
- "ServiceAddresses",
- serviceAddressList.front());
- return;
- }
- messages::internalError(asyncResp->res);
- return;
- }
- std::vector<std::string> modifiedserviceAddressList = {
- serviceAddressList.front()};
- asyncResp->res.jsonValue[ldapServerElementName]["ServiceAddresses"] =
- modifiedserviceAddressList;
- if ((serviceAddressList).size() > 1)
- {
- messages::propertyValueModified(asyncResp->res, "ServiceAddresses",
- serviceAddressList.front());
- }
- BMCWEB_LOG_DEBUG("Updated the service address");
- });
+ setDbusProperty(asyncResp, ldapDbusService, ldapConfigObject,
+ ldapConfigInterface, "LDAPServerURI",
+ ldapServerElementName + "/ServiceAddress",
+ serviceAddressList.front());
}
/**
* @brief updates the LDAP Bind DN and updates the
@@ -855,21 +768,10 @@
const std::string& ldapServerElementName,
const std::string& ldapConfigObject)
{
- sdbusplus::asio::setProperty(*crow::connections::systemBus, ldapDbusService,
- ldapConfigObject, ldapConfigInterface,
- "LDAPBindDN", username,
- [asyncResp, username, ldapServerElementName](
- const boost::system::error_code& ec) {
- if (ec)
- {
- BMCWEB_LOG_DEBUG("Error occurred in updating the username");
- messages::internalError(asyncResp->res);
- return;
- }
- asyncResp->res.jsonValue[ldapServerElementName]["Authentication"]
- ["Username"] = username;
- BMCWEB_LOG_DEBUG("Updated the username");
- });
+ setDbusProperty(asyncResp, ldapDbusService, ldapConfigObject,
+ ldapConfigInterface, "LDAPBindDN",
+ ldapServerElementName + "/Authentication/Username",
+ username);
}
/**
@@ -886,21 +788,10 @@
const std::string& ldapServerElementName,
const std::string& ldapConfigObject)
{
- sdbusplus::asio::setProperty(*crow::connections::systemBus, ldapDbusService,
- ldapConfigObject, ldapConfigInterface,
- "LDAPBindDNPassword", password,
- [asyncResp, password, ldapServerElementName](
- const boost::system::error_code& ec) {
- if (ec)
- {
- BMCWEB_LOG_DEBUG("Error occurred in updating the password");
- messages::internalError(asyncResp->res);
- return;
- }
- asyncResp->res.jsonValue[ldapServerElementName]["Authentication"]
- ["Password"] = "";
- BMCWEB_LOG_DEBUG("Updated the password");
- });
+ setDbusProperty(asyncResp, ldapDbusService, ldapConfigObject,
+ ldapConfigInterface, "LDAPBindDNPassword",
+ ldapServerElementName + "/Authentication/Password",
+ password);
}
/**
@@ -918,41 +809,11 @@
const std::string& ldapServerElementName,
const std::string& ldapConfigObject)
{
- sdbusplus::asio::setProperty(*crow::connections::systemBus, ldapDbusService,
- ldapConfigObject, ldapConfigInterface,
- "LDAPBaseDN", baseDNList.front(),
- [asyncResp, baseDNList, ldapServerElementName](
- const boost::system::error_code& ec,
- const sdbusplus::message_t& msg) {
- if (ec)
- {
- BMCWEB_LOG_DEBUG("Error Occurred in Updating the base DN");
- const sd_bus_error* dbusError = msg.get_error();
- if ((dbusError != nullptr) &&
- (dbusError->name ==
- std::string_view(
- "xyz.openbmc_project.Common.Error.InvalidArgument")))
- {
- messages::propertyValueIncorrect(asyncResp->res,
- "BaseDistinguishedNames",
- baseDNList.front());
- return;
- }
- messages::internalError(asyncResp->res);
- return;
- }
- auto& serverTypeJson = asyncResp->res.jsonValue[ldapServerElementName];
- auto& searchSettingsJson =
- serverTypeJson["LDAPService"]["SearchSettings"];
- std::vector<std::string> modifiedBaseDNList = {baseDNList.front()};
- searchSettingsJson["BaseDistinguishedNames"] = modifiedBaseDNList;
- if (baseDNList.size() > 1)
- {
- messages::propertyValueModified(
- asyncResp->res, "BaseDistinguishedNames", baseDNList.front());
- }
- BMCWEB_LOG_DEBUG("Updated the base DN");
- });
+ setDbusProperty(asyncResp, ldapDbusService, ldapConfigObject,
+ ldapConfigInterface, "LDAPBaseDN",
+ ldapServerElementName +
+ "/LDAPService/SearchSettings/BaseDistinguishedNames",
+ baseDNList.front());
}
/**
* @brief updates the LDAP user name attribute and updates the
@@ -969,24 +830,11 @@
const std::string& ldapServerElementName,
const std::string& ldapConfigObject)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, ldapDbusService, ldapConfigObject,
- ldapConfigInterface, "UserNameAttribute", userNameAttribute,
- [asyncResp, userNameAttribute,
- ldapServerElementName](const boost::system::error_code& ec) {
- if (ec)
- {
- BMCWEB_LOG_DEBUG("Error Occurred in Updating the "
- "username attribute");
- messages::internalError(asyncResp->res);
- return;
- }
- auto& serverTypeJson = asyncResp->res.jsonValue[ldapServerElementName];
- auto& searchSettingsJson =
- serverTypeJson["LDAPService"]["SearchSettings"];
- searchSettingsJson["UsernameAttribute"] = userNameAttribute;
- BMCWEB_LOG_DEBUG("Updated the user name attr.");
- });
+ setDbusProperty(asyncResp, ldapDbusService, ldapConfigObject,
+ ldapConfigInterface, "UserNameAttribute",
+ ldapServerElementName +
+ "LDAPService/SearchSettings/UsernameAttribute",
+ userNameAttribute);
}
/**
* @brief updates the LDAP group attribute and updates the
@@ -1003,24 +851,11 @@
const std::string& ldapServerElementName,
const std::string& ldapConfigObject)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, ldapDbusService, ldapConfigObject,
- ldapConfigInterface, "GroupNameAttribute", groupsAttribute,
- [asyncResp, groupsAttribute,
- ldapServerElementName](const boost::system::error_code& ec) {
- if (ec)
- {
- BMCWEB_LOG_DEBUG("Error Occurred in Updating the "
- "groupname attribute");
- messages::internalError(asyncResp->res);
- return;
- }
- auto& serverTypeJson = asyncResp->res.jsonValue[ldapServerElementName];
- auto& searchSettingsJson =
- serverTypeJson["LDAPService"]["SearchSettings"];
- searchSettingsJson["GroupsAttribute"] = groupsAttribute;
- BMCWEB_LOG_DEBUG("Updated the groupname attr");
- });
+ setDbusProperty(asyncResp, ldapDbusService, ldapConfigObject,
+ ldapConfigInterface, "GroupNameAttribute",
+ ldapServerElementName +
+ "/LDAPService/SearchSettings/GroupsAttribute",
+ groupsAttribute);
}
/**
* @brief updates the LDAP service enable and updates the
@@ -1036,21 +871,9 @@
const std::string& ldapServerElementName,
const std::string& ldapConfigObject)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, ldapDbusService, ldapConfigObject,
- ldapEnableInterface, "Enabled", serviceEnabled,
- [asyncResp, serviceEnabled,
- ldapServerElementName](const boost::system::error_code& ec) {
- if (ec)
- {
- BMCWEB_LOG_DEBUG("Error Occurred in Updating the service enable");
- messages::internalError(asyncResp->res);
- return;
- }
- asyncResp->res.jsonValue[ldapServerElementName]["ServiceEnabled"] =
- serviceEnabled;
- BMCWEB_LOG_DEBUG("Updated Service enable = {}", serviceEnabled);
- });
+ setDbusProperty(asyncResp, ldapDbusService, ldapConfigObject,
+ ldapEnableInterface, "Enabled",
+ ldapServerElementName + "/ServiceEnabled", serviceEnabled);
}
inline void
@@ -1359,19 +1182,10 @@
if (enabled)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus,
- "xyz.openbmc_project.User.Manager", dbusObjectPath,
- "xyz.openbmc_project.User.Attributes", "UserEnabled", *enabled,
- [asyncResp](const boost::system::error_code& ec) {
- if (ec)
- {
- BMCWEB_LOG_ERROR("D-Bus responses error: {}", ec);
- messages::internalError(asyncResp->res);
- return;
- }
- messages::success(asyncResp->res);
- });
+ setDbusProperty(asyncResp, "xyz.openbmc_project.User.Manager",
+ dbusObjectPath,
+ "xyz.openbmc_project.User.Attributes",
+ "UserEnabled", "Enabled", *enabled);
}
if (roleId)
@@ -1383,20 +1197,10 @@
"Locked");
return;
}
-
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus,
- "xyz.openbmc_project.User.Manager", dbusObjectPath,
- "xyz.openbmc_project.User.Attributes", "UserPrivilege", priv,
- [asyncResp](const boost::system::error_code& ec) {
- if (ec)
- {
- BMCWEB_LOG_ERROR("D-Bus responses error: {}", ec);
- messages::internalError(asyncResp->res);
- return;
- }
- messages::success(asyncResp->res);
- });
+ setDbusProperty(asyncResp, "xyz.openbmc_project.User.Manager",
+ dbusObjectPath,
+ "xyz.openbmc_project.User.Attributes",
+ "UserPrivilege", "RoleId", priv);
}
if (locked)
@@ -1410,21 +1214,10 @@
"Locked");
return;
}
-
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus,
- "xyz.openbmc_project.User.Manager", dbusObjectPath,
- "xyz.openbmc_project.User.Attributes",
- "UserLockedForFailedAttempt", *locked,
- [asyncResp](const boost::system::error_code& ec) {
- if (ec)
- {
- BMCWEB_LOG_ERROR("D-Bus responses error: {}", ec);
- messages::internalError(asyncResp->res);
- return;
- }
- messages::success(asyncResp->res);
- });
+ setDbusProperty(asyncResp, "xyz.openbmc_project.User.Manager",
+ dbusObjectPath,
+ "xyz.openbmc_project.User.Attributes",
+ "UserLockedForFailedAttempt", "Locked", *locked);
}
if (accountTypes)
@@ -1594,19 +1387,11 @@
if (minPasswordLength)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, "xyz.openbmc_project.User.Manager",
- "/xyz/openbmc_project/user",
+ setDbusProperty(
+ asyncResp, "xyz.openbmc_project.User.Manager",
+ sdbusplus::message::object_path("/xyz/openbmc_project/user"),
"xyz.openbmc_project.User.AccountPolicy", "MinPasswordLength",
- *minPasswordLength,
- [asyncResp](const boost::system::error_code& ec) {
- if (ec)
- {
- messages::internalError(asyncResp->res);
- return;
- }
- messages::success(asyncResp->res);
- });
+ "MinPasswordLength", *minPasswordLength);
}
if (maxPasswordLength)
@@ -1642,34 +1427,20 @@
if (unlockTimeout)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, "xyz.openbmc_project.User.Manager",
- "/xyz/openbmc_project/user",
+ setDbusProperty(
+ asyncResp, "xyz.openbmc_project.User.Manager",
+ sdbusplus::message::object_path("/xyz/openbmc_project/user"),
"xyz.openbmc_project.User.AccountPolicy", "AccountUnlockTimeout",
- *unlockTimeout, [asyncResp](const boost::system::error_code& ec) {
- if (ec)
- {
- messages::internalError(asyncResp->res);
- return;
- }
- messages::success(asyncResp->res);
- });
+ "AccountLockoutDuration", *unlockTimeout);
}
if (lockoutThreshold)
{
- sdbusplus::asio::setProperty(
- *crow::connections::systemBus, "xyz.openbmc_project.User.Manager",
- "/xyz/openbmc_project/user",
+ setDbusProperty(
+ asyncResp, "xyz.openbmc_project.User.Manager",
+ sdbusplus::message::object_path("/xyz/openbmc_project/user"),
"xyz.openbmc_project.User.AccountPolicy",
- "MaxLoginAttemptBeforeLockout", *lockoutThreshold,
- [asyncResp](const boost::system::error_code& ec) {
- if (ec)
- {
- messages::internalError(asyncResp->res);
- return;
- }
- messages::success(asyncResp->res);
- });
+ "MaxLoginAttemptBeforeLockout", "AccountLockoutThreshold",
+ *lockoutThreshold);
}
}