-
Notifications
You must be signed in to change notification settings - Fork 180
Description
In the HTTP router, there appears to be this async_method_call;
Line 1296 in 3fad0d5
| crow::connections::systemBus->async_method_call( |
This is causing a dbus method call for every request. To my understanding, this was added because of LDAP support needing to fill in an unknown users role. That's fine, but causing a performance impact to all requests, not just those systems relying on LDAP, or when LDAP is enabled is a problem, and needs resolved.
Other notable issues:
The routing layer/file should have nothing to do with looking up a users role. At a minimum this is the wrong place to put the code.
It forces a dependence on phosphor-user-manager and Dbus. bmcweb is used in contexts without dbus or phosphor-user-manager (ie, openrmc). While that project is a fork, I'd like to see them joined at some point, and the more openbmc-specific restrictions we impose at the framework level, the harder that becomes.
req, res, and rules are all captured by reference, which might lead to lifetime issues if the connection is closed while the UserRole lookup is occurring.
std::map is less efficient than could be used.
I have very little context on what this is needed for. Does anyone have any suggestions on how these could be resolved?
It should be noted, this was checked in as a "fix"
61dbeef
The code was arguably better before, as it didn't cause a performance impact to users not using ldap, but the commit message is not clear about why exactly we had to move to what is effectively a block call.