Manage Request with shared_ptr

This is an attempt to solve a class of use-after-move bugs on the
Request objects which have popped up several times. This more clearly
identifies code which owns the

Manage Request with shared_ptr

This is an attempt to solve a class of use-after-move bugs on the
Request objects which have popped up several times. This more clearly
identifies code which owns the Request objects and has a need to keep it
alive. Currently it's just the `Connection` (or `HTTP2Connection`)
(which needs to access Request headers while sending the response), and
the `validatePrivilege()` function (which needs to temporarily own the
Request while doing an asynchronous D-Bus call). Route handlers are
provided a non-owning `Request&` for immediate use and required to not
hold the `Request&` for future use.

Tested: Redfish validator passes (with a few unrelated fails).
Redfish URLs are sent to a browser as HTML instead of raw JSON.

Change-Id: Id581fda90b6bceddd08a5dc7ff0a04b91e7394bf
Signed-off-by: Jonathan Doman <jonathan.doman@intel.com>
Signed-off-by: Ed Tanous <ed@tanous.net>

show more ...

Bring consistency to config options

The configuration options that exist in bmcweb are an amalgimation of
CROW options, CMAKE options using #define, pre-bmcweb ifdef mechanisms
and meson options usi

Bring consistency to config options

The configuration options that exist in bmcweb are an amalgimation of
CROW options, CMAKE options using #define, pre-bmcweb ifdef mechanisms
and meson options using a config file. This history has led to a lot of
different ways to configure code in the codebase itself, which has led
to problems, and issues in consistency.

ifdef options do no compile time checking of code not within the branch.
This is good when you have optional dependencies, but not great when
you're trying to ensure both options compile.

This commit moves all internal configuration options to:
1. A namespace called bmcweb
2. A naming scheme matching the meson option. hyphens are replaced with
underscores, and the option is uppercased. This consistent transform
allows matching up option keys with their code counterparts, without
naming changes.
3. All options are bool true = enabled, and any options with _ENABLED or
_DISABLED postfixes have those postfixes removed. (note, there are
still some options with disable in the name, those are left as-is)
4. All options are now constexpr booleans, without an explicit compare.

To accomplish this, unfortunately an option list in config/meson.build
is required, given that meson doesn't provide a way to dump all options,
as is a manual entry in bmcweb_config.h.in, in addition to the
meson_options. This obsoletes the map in the main meson.build, which
helps some of the complexity.

Now that we've done this, we have some rules that will be documented.
1. Runtime behavior changes should be added as a constexpr bool to
bmcweb_config.h
2. Options that require optionally pulling in a dependency shall use an
ifdef, defined in the primary meson.build. (note, there are no
options that currently meet this class, but it's included for
completeness.)

Note, that this consolidation means that at configure time, all options
are printed. This is a good thing and allows direct comparison of
configs in log files.

Tested: Code compiles
Server boots, and shows options configured in the default build. (HTTPS,
log level, etc)

Change-Id: I94e79a56bcdc01755036e4e7278c7e69e25809ce
Signed-off-by: Ed Tanous <ed@tanous.net>

show more ...

Break out formatters

In the change made to move to std::format, we defined some custom type
formatters in logging.hpp. This had the unintended effect of making
all compile units pull in the majorit

Break out formatters

In the change made to move to std::format, we defined some custom type
formatters in logging.hpp. This had the unintended effect of making
all compile units pull in the majority of boost::url, and nlohmann::json
as includes.

This commit breaks out boost and json formatters into their own separate
includes.

Tested: Code compiles. Logging changes only.

Change-Id: I6a788533169f10e19130a1910cd3be0cc729b020
Signed-off-by: Ed Tanous <ed@tanous.net>

show more ...

Simplify query_param

Static analysis notes that the values in these functions are never
initialized, and that a small section of the branch is no longer
possible to hit, now that a default case has

Simplify query_param

Static analysis notes that the values in these functions are never
initialized, and that a small section of the branch is no longer
possible to hit, now that a default case has been added in
4da0490bc07a458ad3fc7d586c7eabf6053c572f

Remove the dead code and initialize variables where appropriate.

Tested: Unit tests pass. Decent coverage here.

Change-Id: I42ec4678672fea5b21f98aaae05dfca0629652e7
Signed-off-by: Ed Tanous <ed@tanous.net>

show more ...

Fix redundant inline operators

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: I3115b7c0

Fix redundant inline operators

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>

show more ...

Use no-switch-default on clang

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

Use no-switch-default on clang

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>

show more ...

Fix redundant init issues

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: I1de7686c597

Fix redundant init issues

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>

show more ...

Simplify body

Now that we have a custom boost http body class, we can use it in more
cases. There's some significant overhead and code when switching to a
file body, namely removing all the headers

Simplify body

Now that we have a custom boost http body class, we can use it in more
cases. There's some significant overhead and code when switching to a
file body, namely removing all the headers. Making the body class
support strings would allow us to completely avoid that inefficiency.
At the same time, it would mean that we can now use that class for all
cases, including HttpClient, and http::Request. This leads to some code
reduction overall, and means we're reliant on fewer beast structures.

As an added benefit, we no longer have to take a dependency on
boost::variant2.

Tested: Redfish service validator passes, with the exception of
badNamespaceInclude, which is showing warnings prior to this commit.

Change-Id: I061883a73230d6085d951c15891465c2c8445969
Signed-off-by: Ed Tanous <ed@tanous.net>

show more ...

Remove some boost includes

The less we rely on boost, and more on std algorithms, the less people
have to look up, and the more likely that our code will deduplicate.

Replace all uses of boost::alg

Remove some boost includes

The less we rely on boost, and more on std algorithms, the less people
have to look up, and the more likely that our code will deduplicate.

Replace all uses of boost::algorithms with std alternatives.

Tested: Redfish Service Validator passes.

Change-Id: I8a26f39b5709adc444b4178e92f5f3c7b988b05b
Signed-off-by: Ed Tanous <edtanous@google.com>

show more ...

Fix spelling mistakes

These were found with:
codespell -w $(git ls-files | grep "\.[hc]\(pp\)\?$")

At some point in the future, we might want to get this enabled in CI.

Change-Id: Iccb57b2adfd06a2

Fix spelling mistakes

These were found with:
codespell -w $(git ls-files | grep "\.[hc]\(pp\)\?$")

At some point in the future, we might want to get this enabled in CI.

Change-Id: Iccb57b2adfd06a2e177e99db2923fe4e8e329118
Signed-off-by: Ed Tanous <ed@tanous.net>

show more ...

Use ranges

C++20 brought us std::ranges for a lot of algorithms. Most of these
conversions were done using comby, similar to:

```
comby -verbose 'std::lower_bound(:[a].begin(),:[b].end(),:[c])' 's

Use ranges

C++20 brought us std::ranges for a lot of algorithms. Most of these
conversions were done using comby, similar to:

```
comby -verbose 'std::lower_bound(:[a].begin(),:[b].end(),:[c])' 'std::ranges::lower_bound(:[a], :[c])' $(git ls-files | grep "\.[hc]\(pp\)\?$") -in-place
```

Change-Id: I0c99c04e9368312555c08147d474ca93a5959e8d
Signed-off-by: Ed Tanous <edtanous@google.com>

show more ...

Replace logging with std::format

std::format is a much more modern logging solution, and gives us a lot
more flexibility, and better compile times when doing logging.

Unfortunately, given its level

Replace logging with std::format

std::format is a much more modern logging solution, and gives us a lot
more flexibility, and better compile times when doing logging.

Unfortunately, given its level of compile time checks, it needs to be a
method, instead of the stream style logging we had before. This
requires a pretty substantial change. Fortunately, this change can be
largely automated, via the script included in this commit under
scripts/replace_logs.py. This is to aid people in moving their
patchsets over to the new form in the short period where old patches
will be based on the old logging. The intention is that this script
eventually goes away.

The old style logging (stream based) looked like.

BMCWEB_LOG_DEBUG << "Foo " << foo;

The new equivalent of the above would be:
BMCWEB_LOG_DEBUG("Foo {}", foo);

In the course of doing this, this also cleans up several ignored linter
errors, including macro usage, and array to pointer deconstruction.

Note, This patchset does remove the timestamp from the log message. In
practice, this was duplicated between journald and bmcweb, and there's
no need for both to exist.

One design decision of note is the addition of logPtr. Because the
compiler can't disambiguate between const char* and const MyThing*, it's
necessary to add an explicit cast to void*. This is identical to how
fmt handled it.

Tested: compiled with logging meson_option enabled, and launched bmcweb

Saw the usual logging, similar to what was present before:
```
[Error include/webassets.hpp:60] Unable to find or open /usr/share/www/ static file hosting disabled
[Debug include/persistent_data.hpp:133] Restored Session Timeout: 1800
[Debug redfish-core/include/event_service_manager.hpp:671] Old eventService config not exist
[Info src/webserver_main.cpp:59] Starting webserver on port 18080
[Error redfish-core/include/event_service_manager.hpp:1301] inotify_add_watch failed for redfish log file.
[Info src/webserver_main.cpp:137] Start Hostname Monitor Service...
```
Signed-off-by: Ed Tanous <ed@tanous.net>

Change-Id: I86a46aa2454be7fe80df608cb7e5573ca4029ec8

show more ...

FindNavigationReferences rename p->jsonPtr

It was pointed out in a prior review that single character variable
names were not very descriptive. Rename p to jsonPtr to be more
descriptive.

Tested:

FindNavigationReferences rename p->jsonPtr

It was pointed out in a prior review that single character variable
names were not very descriptive. Rename p to jsonPtr to be more
descriptive.

Tested: Pretty good unit test coverage here.

Change-Id: I9583d52dd6cc7f5e6a8eabe42bbe8da9efa44ee0
Signed-off-by: Ed Tanous <edtanous@google.com>

show more ...

Make findNavigationReference* use references

Using pointers here would require to check for null in every handler,
which is a bit wasteful. Use a reference instead.

Tested: Unit tests pass. Prett

Make findNavigationReference* use references

Using pointers here would require to check for null in every handler,
which is a bit wasteful. Use a reference instead.

Tested: Unit tests pass. Pretty good coverage for this section of code.

Change-Id: If47e893affb96eb848262cbbcd718f676191664a
Signed-off-by: Ed Tanous <edtanous@google.com>

show more ...

Split up findNavigationReference

findNavigationReference handles a couple different types. Split it up
into the relevant types.

Tested: Unit tests pass. Pretty good coverage for this section of c

Split up findNavigationReference

findNavigationReference handles a couple different types. Split it up
into the relevant types.

Tested: Unit tests pass. Pretty good coverage for this section of code.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I0c601a0779c23050b7803e4691334243485c8d29

show more ...

query: Fix default expand level with delegated

With delegate expand, the default expand level is -=
`queryCapabilities.canDelegateExpandLevel`. This creates an overlap of
expand process between dele

query: Fix default expand level with delegated

With delegate expand, the default expand level is -=
`queryCapabilities.canDelegateExpandLevel`. This creates an overlap of
expand process between delegate expand vs. default expand.

With
query.expandLevel = 2 ->
query.expandLevel = 1 and delegated.expandLevel = 1.

Both delegated and default expand will try to only expand of level one
instead of level 2 for the default.

The code in
https://github.com/openbmc/bmcweb/blob/479e899d5f57a67647f83b7f615d2c8565290bcf/redfish-core/include/utils/query_param.hpp#L583-L597
stated that the level with "@odata.id" + other property is treated as a
seperate level. So with `query.expandLevel = 1` it just loop through the
id that was already expanded and is noop.

Tested:

Before:
/redfish/v1/Chassis/BMC/Sensors?$expand=.($levels=2) returns
the same result as level=1. Needs level=3 to expand to the next level.

The RelatedItem in here doesn't get expanded with level=2.
```
wget -qO- 'http://localhost:80/redfish/v1/Chassis/BMC/Sensors?$expand=.($levels=1)'
...
{
"@odata.id": "/redfish/v1/Chassis/BMC/Sensors/temperature_DIMMXX",
"@odata.type": "#Sensor.v1_2_0.Sensor",
"Id": "temperature_DIMMXX",
"Name": "DIMMXX",
"Reading": 30.0,
"ReadingRangeMax": 127.0,
"ReadingRangeMin": -128.0,
"ReadingType": "Temperature",
"ReadingUnits": "Cel",
"RelatedItem": [
{
"@odata.id": "/redfish/v1/Systems/system/Memory/dimmXX"
}
],
"Status": {
"Health": "OK",
"State": "Enabled"
},
"Thresholds": {
"LowerCaution": {
"Reading": null
},
"LowerCritical": {
"Reading": null
},
"UpperCaution": {
"Reading": 93.0
},
"UpperCritical": {
"Reading": 95.0
}
}
}
],
"Members@odata.count": 242,
"Name": "Sensors"
}
```

After:
level=2 was able to expand to the next level.

Change-Id: I542177a94a33f8df7afbb68837f3a53b86140c86
Signed-off-by: Willy Tu <wltu@google.com>

show more ...

fix clang-tidy warnings with unreachable returns

```
/data0/jenkins/workspace/ci-repository/openbmc/bmcweb/http/verb.hpp:51:12: error: 'return' will never be executed [clang-diagnostic-unreachable-c

fix clang-tidy warnings with unreachable returns

```
/data0/jenkins/workspace/ci-repository/openbmc/bmcweb/http/verb.hpp:51:12: error: 'return' will never be executed [clang-diagnostic-unreachable-code-return,-warnings-as-errors]
/data0/jenkins/workspace/ci-repository/openbmc/bmcweb/http/utility.hpp:99:12: error: 'return' will never be executed [clang-diagnostic-unreachable-code-return,-warnings-as-errors]
/data0/jenkins/workspace/ci-repository/openbmc/bmcweb/redfish-core/include/utils/query_param.hpp:272:13: error: 'break' will never be executed [clang-diagnostic-unreachable-code-break,-warnings-as-errors]
```

Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Change-Id: Ia74f4fb4f34875097d1ef04b26e40908cc175088

show more ...

query-param: fix clang-tidy warnings

```
../redfish-core/include/utils/query_param.hpp:287:51: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
auto it = std::from_chars(value.da

query-param: fix clang-tidy warnings

```
../redfish-core/include/utils/query_param.hpp:287:51: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
auto it = std::from_chars(value.data(), value.data() + value.size(),
~~~~~~^~~~~~
../redfish-core/include/utils/query_param.hpp:307:45: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
std::from_chars(value.data(), value.data() + value.size(), param);
```

Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Change-Id: Ic93d3e98e0539f5c7068b93ff7e4505fdd5bbfe1

show more ...

clang-format: copy latest and re-format

clang-format-16 has some backwards incompatible changes that require
additional settings for best compatibility and re-running the formatter.
Copy the latest

clang-format: copy latest and re-format

clang-format-16 has some backwards incompatible changes that require
additional settings for best compatibility and re-running the formatter.
Copy the latest .clang-format from the docs repository and reformat the
repository.

Change-Id: I75f89d2959b0f1338c20d72ad669fbdc1d720835
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>

show more ...

Only parse to the depth requested

There are cases in aggregation where an expand parameter might get
forwarded to a client. Because our previous expand algorithm assumed
that any endpoint within bm

Only parse to the depth requested

There are cases in aggregation where an expand parameter might get
forwarded to a client. Because our previous expand algorithm assumed
that any endpoint within bmcweb would only produce "depth=1" responses,
it was reasonable to assume that the pre-response could not contain
expanded content. Aggregated resources can't make that assumption.

This commit attempts to pass through depth through the request, to
ensure that we only expand the level that the user requested, and not
any level returned by the request. This is done by using the existence
of the resource identifer "@odata.id" to indicate each level in an
expanded response. This should be fine since the Redfish spec requires
that property to exist.

Added unit tests to cover aggregation scenarios. Modified existing
$expand tests to comply with the resource identifier dependency.

Tested:
New unit tests pass

Queried '/redfish/v1/Systems?$expand=.($levels=2)' on an aggregated
system whose satellite BMC supported $expand. The overall response was
correctly expanded for both resources on the aggregating BMC as well as
on the satellite BMC. Expanding the satellite resources did not require
sending multiple queries to the satellite.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I20ba60ee39bac11ffb3fe1768cec6299cf9ee13e
Signed-off-by: Carson Labrado <clabrado@google.com>

show more ...

Implement alternative to on boost::split

boost::split has a documented false-positive in clang-tidy. While
normally we'd handle this with NOLINTNEXTLINE, this doesn't appear to
work in all cases.

Implement alternative to on boost::split

boost::split has a documented false-positive in clang-tidy. While
normally we'd handle this with NOLINTNEXTLINE, this doesn't appear to
work in all cases. Unclear why, but seems to be due to some of our
lambda callback complexity.

Each of these uses is a case where we should be using a more specific
check, rather than split, but for the moment, this is the best we have.

Tested: clang-tidy passes.

[1] https://github.com/llvm/llvm-project/issues/40486

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I144c6610cb740287b7225e2be03b4142a64f9563

show more ...

Prepare for boost::url upgrade

The new boost URL now interops properly with std::string_view, which is
great, and cleans up a bunch of mediocre code to convert one to another.
It has also been pulle

Prepare for boost::url upgrade

The new boost URL now interops properly with std::string_view, which is
great, and cleans up a bunch of mediocre code to convert one to another.
It has also been pulled into boost-proper, so we no longer need a
boost-url dependency that's separate.

Unfortunately, boost url makes these improvements by changing
boost::string_view for boost::urls::const_string, which causes us to
have some compile errors on the missing type.

The bulk of these changes fall into a couple categories, and have to be
executed in one commit.
string() is replaced with buffer() on the url and url_view types
boost::string_view is replaced by std::string_view for many times, in
many cases removing a temporary that we had in the code previously.

Tested: Code compiles with boost 1.81.0 beta.
Redfish service validator passes.
Pretty good unit test coverage for URL-specific use cases.

Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I8d3dc89b53d1cc390887fe53605d4867f75f76fd

show more ...

query: propogate errors for expand

The existing code doesn't propogate errors of subqueries correctly.
This commit corrects the behavior, so that the final response gets all
error message of subquer

query: propogate errors for expand

The existing code doesn't propogate errors of subqueries correctly.
This commit corrects the behavior, so that the final response gets all
error message of subqueries and the "highest priority" HTTP code.

DMTF doesn't specify how expand queries handle error codes, since using
subqueries is an implementation choice that we made in this project.

What we did here follows existing behavior of this project, and follows
the error message section of the Redfish spec;
[1] https://redfish.dmtf.org/schemas/DSP0266_1.15.1.html#error-responses

As for now, this commit uses the worst HTTP code among all the error
code. See query_param.hpp, function |propogateErrorCode| for detailed
order of the errror codes.

Tested:
1. this is difficult to test, but I hijacked the code so it returns
errors in TaskServices, then I verified that "/redfish/v1?$expand=."
correctly returns 500 and the gets the error message set.
2. unit test so that when there are multiple errors, the final response
gets a generate error message.

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I0c1ebdd9015f389801db9150d687027485f1203c

show more ...

query: make $select true by default

The most outstanding concerns for $select query have been resolved. We
added a set of restrictions: character set, property length, # of
properties, which makes t

query: make $select true by default

The most outstanding concerns for $select query have been resolved. We
added a set of restrictions: character set, property length, # of
properties, which makes this feature safe to use.

This commit takes $select out of the insecure flag, so every system can
start to use it. This decision has been made in Discord, available at

[1] https://discord.com/channels/775381525260664832/994314752102760559/1006650821569675355

Tested:
1. unit test passed
2. no new service validator failure on hardware

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I1f669cd35afcc1a65473a3ed665768e172a423bc

show more ...

query_param: Move maxEntriesPerPage to Query struct

Putting the maxEntriesPerPage next to the top parameter makes it more
clear about its intention as Ed suggested. Here it is also renamed to
maxTop

query_param: Move maxEntriesPerPage to Query struct

Putting the maxEntriesPerPage next to the top parameter makes it more
clear about its intention as Ed suggested. Here it is also renamed to
maxTop to illustrate its relationship with top.

Tested:
Build, unit test and Redfish Service Validator passed.

Change-Id: I035eea7e33d78439685a81092a4dd9332cfc501a
Signed-off-by: Jiaqing Zhao <jiaqing.zhao@intel.com>

show more ...