- Jul 16, 2022
-
-
Per the coding standard, we should be using the most specific version of the boost header we rely on. Tested: Code compiles Signed-off-by:Ed Tanous <edtanous@google.com> Change-Id: I76dd5a93c10c17d950b0dbac69183dd622363bce
-
Per the coding standard, now that C++ supports std::string::starts_with and std::string::ends_with, we should be using them over the boost alternatives. This commit goes through and updates all usages. Arguably some of these are incorrect, and instances of common error 13, but because this is mostly a mechanical it intentionally doesn't try to handle it. Tested: Unit tests pass. Signed-off-by:
-
- Jul 15, 2022
-
-
This is a similar change that was recently made for other entry types, so also applying for dump entries. I don't have a system that uses this resource, so I'm not able to test this change. Change-Id: I74ba2f05658c6c53d7e397e03aad31a44c7c3280 Signed-off-by:Jason M. Bills <jason.m.bills@intel.com>
-
boost/urls/urls.hpp pulls in all of boost url, when in this context, we only want url_view.hpp. Per the coding standard (specifically in regards to boost) pull in the more specific header Tested: Code compiles. Signed-off-by: -
If a destination is not reachable, then the connection will timeout in doConnect(). This causes issues later when the client attempts to resend the message. The error check in doClose() should not exit early since that will result in the connection's status not being marked as closed and thus it will never get reused. Similarly, doCloseAndRetry() should not exit early since that will cause the retry flow to hang and the connection's callback function will not get deleted. Tested: Used Redfish Aggregation patches in the chain through https://gerrit.openbmc.org/c/openbmc/bmcweb/+/54896 to verify that requests to collections such as /redfish/v1/Chassis no longer hang when the specified Satellite BMC does not exist Signed-off-by:
Carson Labrado <clabrado@google.com> Change-Id: Ic8369b0de8efb00ff168bc1ed43f1d7fd6c7366a
-
mtu = mtu has no effect. Fix it. Tested: Syntax only change. Code compiles. Signed-off-by: -
This change adds a link in the Manager for all BMCs to an empty ManagerDiagnosticData resource and a minimum ManagerDiagnosticData handler. This service is backed by phosphor-health-monitor (PHM), which is enabled by default through the "obmc-apps" package group. If PHM is disabled, the resource will be empty. $ curl http://${bmc}:10080/redfish/v1/Managers/bmc/ManagerDiagnosticData { "@odata.id": "/redfish/v1/Managers/bmc/ManagerDiagnosticData", "@odata.type": "#ManagerDiagnosticData.v1_0_0.ManagerDiagnosticData", "Id": "ManagerDiagnosticData", "Name": "Manager Diagnostic Data" } Also ran the Redfish Service Validator to make sure no new errors are introduced with the introduction of ManagerDiagnosticData. Signed-off-by:Sui Chen <suichen@google.com> Change-Id: Iba242bc3b6ebec851dbd26e149d5c92c19a7992e
-
Tested: unit test passed. Signed-off-by:Nan Zhou <nanzhoumails@gmail.com> Change-Id: I6f9c6b4648d98f4f035830d8a5da094ba76e7458
-
- Jul 14, 2022
-
-
Log statement was after the return, so therefore didn't do anything. cppcheck found. Tested: No way to test without a bug that causes an uncaught exception. Code review only. Signed-off-by: -
cppcheck takes a little issue with this logic having some redundancies in it. Regardless of that, it's kind of hard to read; Rearrange the logic so it's easier to read and add comments. Tested: Redfish service validator passes. Signed-off-by:
-
- Jul 12, 2022
-
-
Nan Zhou authored
This commit corrects the usage of ASSERT, EXPECT, and all the matchers. It also fixes cases where a cleaner matcher can be used. This commit increases readability (with correct and cleaner matcher) and corrects bugs (access iterator before checking validity). Typical incorrect usage is that when a function returns a boolean value to indicated whether the function succeeds or not, unless the function has clear behavior when it fails, we shouldn't continue the test that inspects the output parameters. A typical test codes look like this, ``` ASSERT_TRUE(fooBar(output)); EXPECT_EQ(output, 123); ``` Reference: https://testing.googleblog.com/2008/07/tott-expect-vs-assert.html https://github.com/google/googletest/blob/main/docs/reference/matchers.md Tested: unit test passed. Signed-off-by:
-
Nan Zhou authored
1. test suite names are usually related to the function under tests, not the whole source or header file 2. test case names should be verbose and describe the high level behavior This commits also splits test cases when they are for different functions, and merges test cases when they are very similar. Reference: 1. https://github.com/google/googletest/tree/main/googletest/samples 2. https://testing.googleblog.com/2007/02/tott-naming-unit-tests-responsibly.html Signed-off-by:
-
Nan Zhou authored
1. put sources into the namespace under test 2. put all test codes into anonymous namespace It can be proved that this can increase readability and save duplicate codes. Signed-off-by: -
cppcheck correctly notes that a lot of variables in the new code can be const. Make most of them const. Tested: WIP Signed-off-by: -
A number of methods in http::Response were not marked const when they should've been. This is generally not an issue, as most usages of Response are in a non-const context, but as we start using const Response objects more, we need to be more careful about const correctness. Tested: Unit tests pass. Signed-off-by:
-
- Jul 11, 2022
-
-
Nan Zhou authored
Use structured binding declaration to avoid verbose typing of subtree response. Tested: 1. code compiles 2. tested on hardware and RoT resources worked as expected. Signed-off-by: -
Nan Zhou authored
Tested: unit test worked. Signed-off-by: -
Nan Zhou authored
These variables are replaced by local string literals, which has better readability. Tested: tested on real hardware. GET: /google/v1/RootOfTrustCollection { "@odata.id": "/google/v1/RootOfTrustCollection", "@odata.type": "#RootOfTrustCollection.RootOfTrustCollection", "Members": [ { "@odata.id": "/google/v1/RootOfTrustCollection/Hoth" } ], "Members@odata.count": 1 } GET /google/v1/RootOfTrustCollection/Hoth { "@odata.id": "/google/v1/RootOfTrustCollection/Hoth", "@odata.type": "#RootOfTrust.v1_0_0.RootOfTrust", "Actions": { "#RootOfTrust.SendCommand": { "target": "/google/v1/RootOfTrustCollection/Hoth/Actions/RootOfTrust.SendCommand" } }, "Description": "Google Root Of Trust", "Id": "Hoth", "Location": { "PartLocation": { "LocationType": "Embedded", "ServiceLabel": "Hoth" } }, "Name": "Hoth", "Status": { "State": "Enabled" } } Signed-off-by: -
Nan Zhou authored
The current convention: 1. route handler should be named as "handleAbcResouceMethod" 2. prefer inline functions instead of local lambdas Tested: 1. compiles 2. on Google hardware, the commands in https://gerrit.openbmc.org/c/openbmc/bmcweb/+/52222/32 worked GET: /google/v1/RootOfTrustCollection { "@odata.id": "/google/v1/RootOfTrustCollection", "@odata.type": "#RootOfTrustCollection.RootOfTrustCollection", "Members": [ { "@odata.id": "/google/v1/RootOfTrustCollection/Hoth" } ], "Members@odata.count": 1 } GET /google/v1/RootOfTrustCollection/Hoth { "@odata.id": "/google/v1/RootOfTrustCollection/Hoth", "@odata.type": "#RootOfTrust.v1_0_0.RootOfTrust", "Actions": { "#RootOfTrust.SendCommand": { "target": "/google/v1/RootOfTrustCollection/Hoth/Actions/RootOfTrust.SendCommand" } }, "Description": "Google Root Of Trust", "Id": "Hoth", "Location": { "PartLocation": { "LocationType": "Embedded", "ServiceLabel": "Hoth" } }, "Name": "Hoth", "Status": { "State": "Enabled" } } Signed-off-by:
-
- Jul 09, 2022
-
-
clang-tidy is now integrated into CI; Documenting the "manual" process for running tidy doesn't provide value these days. Point to openbmc-build-scripts as the "recommended" mechanism to run static analysis. Tested: Documentation only Signed-off-by: -
urlFromPieces is the new way to construct "correct" urls. It also cleans up quite a bit of this code. Use it. Tested: Code compiles. No backend in upstream yet. Signed-off-by: -
If this algorithm ever processed a string where the + came before the ., then dot - plus would render a negative, and overflow, causing a crash. In practice, this doesn't happen. Tested: Inspection only at this time. Difficult to trigger error. Signed-off-by: -
Similar changes were recently made for other log entry types to improve handling of JSON objects. This applies the same fixes for host logger entries. I don't have a system that uses the host logger, so I'm not able to test this change. Change-Id: If5af976885a321e1077d89d92129f4b87a8e2943 Signed-off-by:
-
- Jul 08, 2022
-
-
There's lots of magic numbers in this file that we inherited from crow. This commit Adds a TypeCode enum class that can be used in place of the magic numbers. It keeps the same values as were present previously (0-6) in case there are places where this abstraction leaked out, but I believe this catches all of them. Tested: Redfish service validator passes. Signed-off-by:
-
- Jul 07, 2022
-
-
This normally wouldn't be so big of a deal, but the redfish-protocol validator tests this case in the REQ_PATCH_BAD_PROP test. From the specification: ''' If all properties in the update request are read-only, unknown, or unsupported, but the resource can be updated, the service shall return the HTTP 400 Bad Request status code and an error response with messages that show the non-updatable properties. ''' We wrote our code almost right for handling this case, but we put the response into the per-property responses instead of the error responses. In terms of backward compatibility, technically this is changing the behavior, but considering that it's behavior in an error case, most implementations only look at response code, and this is moving to be compliant with the specification, it doesn't seem like there would be any reason to provide both the old message and the new one, and this has a low to zero likelihood of any actual impact. To hit this condition, clients would have to be ignoring the error code response AND using a property that's unknown to the BMC. Clients that make both mistakes seems unlikely. Tested: Code now passes the REQ_PATCH_BAD_PROP test. 10 failing test cases down to 8. ''' curl -vvvv --insecure --user root:0penBmc https://192.168.7.2/redfish/v1/AccountService/ -X PATCH -d '{"foo": "bar"}' ''' Returns an object with an "error" key in it. Signed-off-by:
-
The Redfish specification for PATCH of arrays defines a number of requirements. - Setting a value to null, should remove it from the list. - Setting a value to empty object "{}" should leave the value unmodified - Values at indexes larger than whats included in the PATCH request shall be removed. This commit attempts to fix this behavior for NTPServers and make it correct. It does this by first getting the list of NTP servers, then walking the list in parallel with the list given in the patch, and either modifying or changing the list as the spec requires before setting the setting across the system. It also turns out that the current behavior of unpacking nlohmann::json objects requires an object to be an array, object, or null, which doesn't allow unpacking the strings required in this case, so that check is removed. A quick inspection shows that we don't unpack nlohmann objects very often, and this should have no impact. Tested: Redfish-protocol-validator tests for NTPServers now pass ''' curl -vvvv --insecure --user root:0penBmc https://192.168.7.2/redfish/v1/Managers/bmc/NetworkProtocol -X PATCH -d '{"NTP": {"NTPServers": []}}' ''' Used to patch values succeeds with various "good" values; ["time-a-b.nist.gov", "time-b-b.nist.gov"] [{}, {}] ["time-a-b.nist.gov", null] [] Signed-off-by: -
getEthernetIfaceData callback should ideally be returning exactly the values the system has, minus duplicates. In every case this function is used, we don't want duplicates, so move where we check for duplicates. Tested: In conjunction with https://gerrit.openbmc.org/c/openbmc/bmcweb/+/54829 test cases pass. Signed-off-by:
-
Adds the ThermalSubsystem schema to the list of the supported schemas. Tested: Code compiles. Script generated change. Signed-off-by:
Xiaochao Ma <maxiaochao@inspur.com> Signed-off-by:
-
Currently, the |systemBus| connection is a static variable declared in headers. This has a problem that every translation unit will keep its own copy. It's not a problem today because there's only one translation unit "webserver_main.cpp.o". This issue was brounght up in https://gerrit.openbmc.org/c/openbmc/bmcweb/+/54758 Actually, the |systemBus| doesn't need to be a singleton. It can just be a stack variable, which is normally more efficient than heap variables. To keep minimum changes treeside, this commits keeps the existing |systemBus| variable as an external variable. It is defined in its own translation unit. It is initialized in the main translation unit. Reference: 1. Extern https://stackoverflow.com/questions/1433204/how-do-i-use-extern-to-share-variables-between-source-files Tested: 1. Romulus QEMU robot Redfish test passed; 2. Start and restart service on real hardware, no issues; 3. No new validator failures 4. Code compies Signed-off-by:
-
The struct |ResolvedEntity| stores a pointer which might be dangling in the future when interface is not longer a string literal. Given that the interface string is small enough, this commits changes the data member to a string which is constructed (copied) from the string literal today. Tested: trivial change. Compiles. Signed-off-by: -
This commit applies the GTest test case naming convention: Camel case, use decriptive Test names. It also groups test cases according to the name. Reference: https://testing.googleblog.com/2014/10/testing-on-toilet-writing-descriptive.html http://google.github.io/googletest/primer.html#simple-tests Tested: unit test passes. Signed-off-by:
-
- Jul 06, 2022
-
-
Tested: unit test passed. Signed-off-by: -
HttpClient these days is intended to be a generic feature. It should not be relying directly on Redfish messages. In this case, we only ever rely on the return code internally, so replace messages with a simple bad_gateway. Tested: Code compiles. Unit tests pass. Signed-off-by: -
Nan Zhou authored
If the boot service isn't there, we shouldn't fail the response but omit the Boot property. Tested: 1. Service Validator passed on system without boot service 2. Service Validator passed on system with boot service Signed-off-by:
-
- Jul 05, 2022
-
-
Clang analyzer complains on this one about loading an int (CHAR_MAX) into a char, and that it might overflow. Obviously it can't, but we might as well suppress the warning. Tested: Unit tests pass. Signed-off-by: -
cppcheck finds a few variables that were unused in a few modules. Clean them up. Tested: Code compiles, unit tests pass. Signed-off-by: -
Fix the includes. Tested: Code compiles. Signed-off-by: -
cppcheck correctly notes this parameter can be const. Unfortunately we've making a copy here, but looking at the ownership, that can't be avoided simply without a shared_ptr, and that's far more invasive. Signed-off-by:
-
- Jul 01, 2022
-
-
This variable was clearly for the print statement below, but then got abandoned at some point. Clean it up. cppcheck found this as well. Tested: Unused code Signed-off-by: -
cppcheck correctly finds that these branches will always be hit, so the branch is unneeded. Let's start by getting the code cleaned up. Tested: CI only, cpp check. Signed-off-by:
-
