| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
TemporaryFileHandle class is used to create temp files in the
filesystem, and hold a handle to them until the class goes out of scope,
at which time they can be removed. It replaces makeFile(), which was
not RAII safe if an exception gets thrown, and could potentially leave
files in the filesystem if the tests fail.
Tested: Unit tests pass
Change-Id: I03eb0d342a6cd7b78115a8c42be9175f30c4ccd0
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
The redfish-enable-proccessor-memory-status option was scheduled to be
removed in 1Q 2024. It is now 2Q, so remove the option. No upstream
layers enabled it and I could not find a downstream layer that did
either.
Redfish deprecated the Processor/Memory Summary Status (state, health,
healthrollup) attributes.
Discussion on discord, when disabling:
[1]: https://discord.com/channels/775381525260664832/855566794994221117/1093939076710793296
Commit disabling this (merged 10 months ago):
[2]: https://github.com/openbmc/bmcweb/commit/5fd0aafb0f14fb3011970e8575647bb608688c7c
Tested: Code builds.
Change-Id: I539cd5f384633afa7badf1cecfc6c7a87062f672
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
Several places access the members of `req` indirectly like
`req.req.method()`. This can be simplified as `req.method()` .
This would also make the code clearer.
Tested:
- Compiles
- Redfish service validator passes
Change-Id: Ie129564ff907cdea7ac224b1e3d80cc0dedfbd7b
Signed-off-by: Myung Bae <myungbae@us.ibm.com>
|
|
Change-Id: I147664c3d181ba8ec535c7cddcb5c714e05616ea
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Have seen defects where hitting the max connection limit with multiple
server managers attached. Although not common to exceed 100, can hit
this when using 2 or 3 webui-vue GUIs and a server manager attached.
webui-vue can use ~30 of these on its own; this isn't that hard to hit.
Nginx by default sets 512 connections[1] , so 200 for an embedded
target doesn't seem that unreasonable:
Apache sets 256 by default [2]
lighttpd sets 1024 [3]
We're in line for the defaults for other webservers.
Tested: Sent 180 basic auth requests seen bmcweb memory at
2189 2178 root R 29080 4% 49% ./bmcweb
This was on a AST2600 (p10bmc)
The connections open got to:
[DEBUG "http_connection.hpp":79] 0x19bb5c8 Connection open, total 161
Came back down as expected:
[DEBUG "http_connection.hpp":89] 0x1a41440 Connection closed, total 1
Didn't see this with multiple webui-vues / server managers.
[1] https://nginx.org/en/docs/ngx_core_module.html#worker_connections
[2] https://httpd.apache.org/docs/2.4/mod/mpm_common.html#maxrequestworkers
[3] https://redmine.lighttpd.net/projects/1/wiki/Server_max-connectionsDetails
Change-Id: I807302e32e61e31212850a480d721d89d484593f
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
This is the same version Yocto uses.
[1] https://github.com/openbmc/openbmc/blob/8c1713b6664523ff7a5bc300ca00c9cde8e5b2c1/poky/meta/recipes-support/nghttp2/nghttp2_1.60.0.bb
Change-Id: I71485c559f37dc21cabeab6b95e38c8a30073af5
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
These tests are commented out, and have been for a very long time.
Clearly they don't matter.
Change-Id: I084378ee9bc43bb64bd6e134398bbf2173d263ff
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Clang-tidy-18 has new checks that can find more cases where we've
missed an opportunity to std::move.
Fix them.
Tested: Logging works, unit tests pass.
Change-Id: I0cf58204ce7265828693b787a7b3a16484c3d5e5
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
readability-avoid-nested-conditional-operator
With one exception, we already pass this check. Update the log services
code to make it pass, and update it to use the generated enums.
Tested: Code inspection only.
Change-Id: Ic80a7382beb0f541de4916d7b51e42ed5d5dc542
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
This commit changes sdbusplus setProperty calls in
hypervisor_system.hpp file to "setDbusProperty" method in Redfish
namespace that handles all DBus errors in a consistent manner.
Change-Id: Iebca5eb4e28159d61cd4b13c0343b78efd0f1f39
Signed-off-by: Asmitha Karunanithi <asmitk01@in.ibm.com>
|
|
This include exists above.
Tested: code compiles.
Change-Id: I8e5d7bce292486d2f534da1b539212113c1e8d56
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
When running compiling from local dependencies, it's nice to be able to
move binaries around wherever they're needed.
When we moved to the cmake version of boost, we started building shared
libraries on accident. nghttp2 has always pulled shared libraries since
introduction.
Go back to building static libraries.
Tested: Binary build from desktop launches directly without a
"missing boost_url.so" error.
Change-Id: I9858f27f21841097bd03a6bbcac953b08caa4b14
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
inline is not required on member methods. Clang-tidy has a check for
this. Enable the check and fix the two bad usages.
Tested: Code compiles.
Change-Id: I3115b7c0c4005e1082e0005b818fbe6569511f49
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
clang-18 improves this check so that we can actually use it. Enable it
and fix all violations.
Change-Id: Ibe4ce19c423d447a4cbe593d1abba948362426af
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Redfish protocol validatator has SSE tests that expose some bad coding
practies in SSE handlers, namely, that there are several cases where we
don't check for nullptr.
Fix them.
This appears to have been introduced in:
https://gerrit.openbmc.org/c/openbmc/bmcweb/+/41319
Tested: Redfish service validator passes more tests.
Change-Id: Id980725f007d044b7d120dbe0f4b625865cab6ba
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
The commit:
b5f288d Make use of filebody for dump offload
Caused a minor failure in clearing responses, where open file handles
wouldn't be closed in between queries, resulting in the next read to
return empty content. This caused redfish protocol validator to fail.
boost::beast::http::response::clear() documentation shows that it only
clears the headers, not the file body. Now normally, this doesn't
matter, because bmcweb completely replaces the response body when a new
response is driven, but not in the case of files.
Add response.body().clear() during the clear to ensure the response is
cleared.
In addition, add encodingType to the clear() call, to ensure that it is
reset as well. This is a bug, but I don't know the reproduction steps.
Tested: Redfish protocol validator now completes (with SSE failures)
Change-Id: Ice6d5085003034a1bed48397ddc6316e9cd0536f
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
When we have an internal error, having the D-Bus response is really
helpful. Follow our guide and have these be a Logging Level Error.
Tested: None. Inspection only.
Change-Id: Ie1d9f364c3af7f2a8839d878d68c82c10ddc0429
Signed-off-by: Gunnar Mills <gmills@us.ibm.com>
|
|
Most of this code was written before bmcweb had a logger, and
therefore used cout/cerr.
This commit greps the codebase and finds all places where we still
use cout/cerr, and moves them to logging.
Tested: Inspection only. No functional changes.
Change-Id: I5ce1883c9941e80203ec29decb3a0206fd118506
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
This commit changes sdbusplus setProperty calls in ethernet.hpp file
to "setDbusProperty" method in Redfish namespace that handles all DBus
errors in a consistent manner.
Tested By:
Tested a few PATCH operations on the redfish endpoints defined in
this file and verified that bmcweb returns appropriate Redfish
errors.
Change-Id: Ie456db75d59dc247cdce5dd5cc0b2f6894f5265f
Signed-off-by: Asmitha Karunanithi <asmitk01@in.ibm.com>
|
|
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>
|
|
And fix the includes that are wrong.
Note, there is a very large ignore list included in the .clang-tidy
configcfile. These are things that clang-tidy doesn't yet handle
well, like knowing about a details include.
Change-Id: Ie3744f2c8cba68a8700b406449d6c2018a736952
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Current code has
//clang-format on
When it should have
// clang-format on
The difference is subtle, but disables formatting for this whole file.
Re-enable and fix the couple of problems.
Tested: Whitespace only.
Change-Id: Ia155226327d4d611eb2c0f5232274459866e81cc
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
These classes accidentally overlapped in naming with the nghttp2
classes. This is because this class, unlike most nghttp2 classes
doesn't end in _ptr for a type. This changes the class name to add a
_ex to differentiate the two classes, and avoid a warning in clang.
Tested: Unit tests pass. Code only used in unit test.
Change-Id: I91a6982264df69bc65166ab38feddc21f72cd223
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Enable the checks we pass already.
This also removes the commented out misc-no-recursion, considering we
don't pass it.
Tested: Clang-tidy passes.
Change-Id: Ibaed95677aed85188bff483d2cd53605faaf7cc6
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Another clang version, another set of checks we can enable.
bmcweb passes all these checks today, so enable them to help
folks write better code.
Change-Id: Ied6a364ee92d8d634edea717cfa2fb5245d534f9
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
clang-tidy-18 makes this feature stable enough for us to use in general.
Enable the check, and fix the couple of regressions that have snuck in
since we last ran the check.
Tidy seems to not be able to understand that ASSERT will not continue,
so if we ASSERT a std::optional, it's not a bug. Add explicit checks to
keep tidy happy.
Tested: clang-tidy passes.
Change-Id: I0986453851da5471056a7b47b8ad57a9801df259
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
It's not clear how this came to be the way it is, but tidy now warns
that this variable is unused (which it is).
Refactor the LDAP code to not use the variable, and to use concrete
object_t and array_t
Tested: Redfish service validator passes.
Change-Id: I0c106d10594a396d506bf9865cb29d4a10a372a1
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Extend the hasIndicatorLed array and add
xyz.openbmc_project.Inventory.Item.Chassis interface.
Tested:
```
curl -k https://$bmc/redfish/v1/Chassis/chassis
{
"@odata.id": "/redfish/v1/Chassis/chassis",
"@odata.type": "#Chassis.v1_22_0.Chassis",
"Actions": {
"#Chassis.Reset": {
"@Redfish.ActionInfo": "/redfish/v1/Chassis/chassis/ResetActionInfo",
"target": "/redfish/v1/Chassis/chassis/Actions/Chassis.Reset"
}
},
"ChassisType": "RackMount",
"Id": "chassis",
"Links": {
"ComputerSystems": [
{
"@odata.id": "/redfish/v1/Systems/system"
}
],
"ManagedBy": [
{
"@odata.id": "/redfish/v1/Managers/bmc"
}
]
},
"Location": {
"PartLocation": {
"ServiceLabel": "U78DA.ND0.WZS004K"
}
},
"IndicatorLED": "Off",
"LocationIndicatorActive": false,
"Manufacturer": "",
"Model": "23",
"Name": "chassis",
"PCIeDevices": {
"@odata.id": "/redfish/v1/Systems/system/PCIeDevices"
},
"PartNumber": "",
"Power": {
"@odata.id": "/redfish/v1/Chassis/chassis/Power"
},
"PowerState": "Off",
"Sensors": {
"@odata.id": "/redfish/v1/Chassis/chassis/Sensors"
},
"SerialNumber": "",
"Status": {
"Health": "OK",
"HealthRollup": "OK",
"State": "StandbyOffline"
},
"Thermal": {
"@odata.id": "/redfish/v1/Chassis/chassis/Thermal"
}
}
```
Signed-off-by: George Liu <liuxiwei@ieisystem.com>
Change-Id: I02e77d56e555f9aee3f76015baeebbf1f4a292ab
|
|
These variables aren't used, and clang-tidy-18 flags it. Remove
Tested: Code compiles.
Change-Id: I414c4614a5f789aecab7700a4ec805e98c09cade
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
bugprone-multi-level-implicit-pointer-conversion is something that we
pass currently, with one exception in the deprecated rest API. Ignore
the one exception, as it's not clear how to fix it, and enable the
check.
Tested: Clang tidy passes.
Change-Id: Idc10e0bb7b876e1c70afa28f9c27cc7bef1db0d7
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
clang-tidy-18 must've fixed their checking for these in headers.
Resolve as the robot commands.
Tested: Noop changes made by tidy. Code compiles.
Change-Id: I1de7686c597deffb0df91c30dae1a29f9ba7900e
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Change-Id: I0133bbd0a7573bd3d1e3c3c99382442b286696f6
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Quite often do I compile this project, and see an error message that
makes no sense. Multiple times I've seen posted about compiler errors
that amount to using an old version of clang or gcc.
Explicitly require clang-17 and gcc-13 in the meson config, and give
better errors if they're not present.
This also allows simplifying our warning flags (which probably need
a review soon) by making two sets of flags, one for each compiler.
Note, clang has the -Weverything flag, which we use, so explicitly
enabling warnings isn't really required, only disabling the ones
that we don't use.
Tested: Code compiles.
Change-Id: I09fa74e6d36feaf05710a4bb7d266f80ff1cc592
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
This code was copied from one of the nghttp2 examples, and makes things
more complicated than they should be. We only handle one case here, so
a pattern of returning early is easier.
Also, this resolves a possible clang-tidy bugprone warning (that we
don't yet enable).
Tested: Http2 unit tests pass (good coverage for this case).
Change-Id: Ie8606872f3a96f1bb0329bf22a4f7429f431bbef
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
If one sets the XSS policy disable, and tries to load the webui, they're
met with the following error message:
```
chunk-vendors.6cfb4b74.js:36 Refused to load the image
'data:image/svg+xml;charset=utf-8,%3Csvg
xmlns='http://www.w3.org/2000/svg' width='4' height='5'%3E%3Cpath
fill='%233f3f3f' d='M2 0L0 2h4zm0 5L0 3h4z'/%3E%3C/svg%3E' because it
violates the following Content Security Policy directive: "img-src *".
Note that '*' matches only URLs with network schemes ('http', 'https',
'ws', 'wss'), or URLs whose scheme matches `self`'s scheme. The scheme
'data:' must be added explicitly.
```
Do as it asks, and add data: to the content security policy.
Tested: Browser console no longer shows error when XSS is enabled.
Change-Id: I17f70d7c87a284b33ef6eb5a01a01c23a14898c9
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
This reverts commit cd40b060ee2df5469077a70d15590f86158f2c60.
Cookie based login is no longer functional with this patch. It looks
like we got a merge conflict that I resolved incorrectly.
Tested: Webui can now log in.
Change-Id: I60b8aeae173b1838d8745a2c499fbcb410813ef3
|
|
This allows http2 connections to now host authenticated endpoints.
Note, this work exposed that the http2 path was not calling
preparePayload() and responses were therefore missing the
Content-Length header. preparePayload is now called, and Content-Length
is added to the unit tests.
This commit also allows a full Redfish Service Validator test to pass
entirely using HTTP2.
Tested: Unit tests pass.
Curl /redfish/v1/Managers/bmc/LogServices/Journal/Entries
(which returns a payload larger than 16kB) succeeds and returns the
data.
Manually logging in with both basic and session authentication succeeds
over http2.
A modified Redfish-Service-Validator, changed to use httpx as its
backend, (thus using http2) succeeds.
Change-Id: I956f3ff8f442e9826312c6147d7599ab136a8e7c
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
We don't have any routes that use 5 wildcards, but clearly someone uses
it because of the bug #270. There's no reason not to fix this.
Ideally we would support an arbitrary number of wildcards, but that's a
template problem for another day.
Tested: No way to test, inspection only.
Change-Id: I5de75f5288124e84c153518966d658e1c899f6d5
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
For the content type header
application/json;charset=utf-8
The Redfish specification DSP0266 shows no space between the ; and
charset. Sites like mozilla show the space included [1]
Considering the discrepancy, we should just accept both.
Resolves #271
[1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type
Tested: Submitter reports issue fixed.
Change-Id: I77b7db91d65acc84f2221ec50985d4b942fbe77f
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Use multiple level direct read.
Tested: Visual only. Need help if anyone wants to test.
Change-Id: I8655e74d39edcbab43fcd2a8379b085e91ed00eb
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Use multiple level direct read
Tested: Visual only. Need help if anyone wants to test.
Change-Id: I2595a7024f1d02e02874310d1911cd4855b867be
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Change-Id: Idc37e3e98296cf59aa6fab499a27d7ed899b71dd
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Change-Id: If4237a21aa0c5f414e20cd9e7eee2f1188097e14
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Change-Id: I29a9ecbdc9011b6513dc6bfccd28e7e7158fed9b
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Change-Id: I2e72f01821e931a8d6eeb812c314de9d1c52df78
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Change-Id: I69ed29472b209e8782be63c3f0f2e8ca63dc14a4
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Use multiple level direct read to simplify code.
Tested: Visual only. Need help if anyone wants to test.
Change-Id: Ib7c34daefbe2bb835cbe420b40861f27442d05b1
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Similar to other places where we've ported the depth-based readJson
support forward, this commit ports the UpdateService handler to simplify
the code.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ia9841a10b4414f81205d3f9b49ec8aab8f9d491d
|
|
Historically, logging has been disabled in bmcweb for two reasons.
First, the crow logging handler used iostreams, which can bloat binary
sizes if there are lots of logging call sites.
Second, the amount of logging and the levels at which is was performed
were not very carefully selected by either crow, or the follow on
bmcweb. A number of log calls logged at Error or Critical level that
are fully expected to occur in a normally operating service.
The first was corrected with commit 62598e3, which replaced the
iostreams logger with c++20 compliant std::format.
The second was corrected by Gunnar, documenting when and where to log
different levels in commit 0e88cb3, and a series of commits after
making the levels usage more consistent.
With those two changes in place and showing the appearance of being
functional, this patchset is recommending that we change the default
error level to log Critical and Error levels by default.
A number of organizations have already made this change to their local
systems [1], opting for varying levels. Given that we're now internally
consistent, this is going to request that we modify those systems to
accept defaults once again.
There are two negatives to this being the default.
1. We take a 2.7% increase (about 27KB) in binary size. Given the last
couple years of reductions in the default binary size, this shouldn't
cause any platforms to go over their flash limit, and we're still well
within the documented size targets.
2. Error paths now log, which slows down the result, and increases the
potential that a DOS attack or test will slow down real results. This
concern is hypothetical at best, but we will have to watch for repeated
patterns emerging and evaluate if this is a potential problem.
Please comment.
[1] https://github.com/search?q=repo%3Aopenbmc%2Fopenbmc%20bmcweb-logging&type=code
Change-Id: Ib32654c3bcbcbee567f3bd7abd003411dd9e489a
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
Redfish supports several type systems for json. This makes parsing into
proper types a challenge. Nlohmann supports 3 core data types,
nlohmann::json, which supports all json types (float, int, array,
object). Nlohmann::json::object_t, which is a specific typedef of
std::map, and nlohmann::json::array_t, which is a specific typedef of
std::map.
Redfish allows reading our arrays of complex objects, similar to
NtpServers: [null, {}, "string"]
Which makes it a challenge to support. This commit allows parsing out
objects as a nlohmann::object_t, which gives the ability to later use it
in a type safe manner, without having to call
get_ptr<nlohmann::json::object_t later>.
Tested:
Unit tests pass.
Change-Id: I4134338951ce27c2f56841a45b56bc64ad1753db
Signed-off-by: Ed Tanous <ed@tanous.net>
|
