| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
Args captured by logging functions should be captured by rvalue, and use
std::forward to get perfect forwarding. In addition, separate out the
various std::out lines.
While we're here, also try to optimize a little. We should ideally be
writing each log line to the output once, and ideally not use iostreams,
which induce a lot of overhead.
Similar to spdlog[1] (which at one point this codebase used), construct
the string, then call fwrite and fflush once, rather than calling
std::cout repeatedly.
Now that we don't have a dependency on iostreams anymore, we can remove
it from the places where it has snuck in.
Tested:
Logging still functions as before. Logs present.
[1] https://github.com/gabime/spdlog/blob/27cb4c76708608465c413f6d0e6b8d99a4d84302/include/spdlog/sinks/stdout_sinks-inl.h#L70C7-L70C13
Change-Id: I1dd4739e06eb506d68989a066d122109b71b92cd
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
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>
|
|
Clang has new checks for std::move/std::forward correctness, which
catches quite a few "wrong" things where we were making copies of
callback handlers.
Unfortunately, the lambda syntax of
callback{std::forward<Callback>(callback)}
in a capture confuses it, so change usages to
callback = std::forward<Callback>(callback)
to be consistent.
Tested: Redfish service validator passes.
Change-Id: I7a111ec00cf78ecb7d5f5b102c786c1c14d74384
Signed-off-by: Ed Tanous <ed@tanous.net>
|
|
clang-format-17 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: I2f9540cf0d545a2da4d6289fc87b754f684bc9a7
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
|
|
Type safety is a good thing. In:
https://gerrit.openbmc.org/c/openbmc/bmcweb/+/65606
It was found that splitting out the URI into encoded pieces in the early
phase removed some information we needed, namely whether or not a URI
was ipv6. This commit changes http client such that it passes all the
information through, with the correct type, rather than passing in
hostname, port, path, and ssl separately.
Opportunistically, because a number of log lines are changing, this uses
the opportunity to remove a number of calls to std::to_string, and rely
on std::format instead.
Now that we no longer use custom URI splitting code, the
ValidateAndSplitUrl() method can be removed, given that our validation
now happens in the URI class.
Tested: Aggregation works properly when satellite URIs are queried.
Change-Id: I9f605863179af54c5af2719bc5ce9d29cbfffab7
Signed-off-by: Ed Tanous <edtanous@google.com>
|
|
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
|
|
The Async DBus resolver really has nothing to do with crow, which is our
core http library namespace and has some opportunistic cleanups that can
be done.
This commit moves it into the bmcweb namespace (unimportantly) and
breaks out one of the larger functions such that it can be unit tested,
and unit tests it.
Tested: Unit tests pass.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ie3cfbb0ef81a027a1ad42358c04967a517471117
|
|
This commit adds a meson option to allow selecting which dns resolver
bmcweb uses. There are use cases, like Open Compute Project Inband
Management Agent, that would require not using dbus, which would require
us to fall back to the asio resolver. This commit makes the existing
asio resolver constructor, and async_resolve methods match the
equivalents in asio (which we intended to do anyway), then adds a macro
and configure option for being able to select which resolver backend to
rely on.
Tested: Code can now compile without sdbusplus.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I3220214367179f131a60082bdfaf7e725d35c125
|
|
By convention, we should be following boost here, and passing error_code
by reference, not by value. This makes our code consistent, and removes
the need for a copy in some cases.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Id42ea4a90b6685a84818b87d1506c11256b3b9ae
|
|
Most of these missing includes were found by running clang-tidy on all
files, including headers. The existing scripts just run clang-tidy on
source files, which doesn't catch most of these.
Tested: Code compiles
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ic741fbb2cc9e5e92955fd5a1b778a482830e80e8
|
|
There are a number of properties of Type "string (uri)" for which we
do not currently support adding prefixes. This patch adds support
for all existing URI properties which are missed by the existing
implementation.
This change will be needed by future patches which will expand
aggregation support to all top level collections defined by the
schema. Those collections that are not currently supported include
properties whose URIs should be fixed, but would be missed by the
existing implementation.
Tested:
New unit test passes.
URI properties are still handled correctly.
```shell
curl localhost/redfish/v1/Chassis/5B247A_<chassisID>
{
"@odata.id": "/redfish/v1/Chassis/5B247A_<chassisID>",
"@odata.type": "#Chassis.v1_16_0.Chassis",
"Actions": {
"#Chassis.Reset": {
"@Redfish.ActionInfo": "/redfish/v1/Chassis/5B247A_<chassisID>/ResetActionInfo",
"target": "/redfish/v1/Chassis/5B247A_<chassisID>/Actions/Chassis.Reset"
}
},
...
}
```
Signed-off-by: Carson Labrado <clabrado@google.com>
Change-Id: I3b3e06ee3191564d266598f7bc9f1641e6fcb333
|
|
clang-tidy has a setting, LambdaBodyIndentation, which it says:
"For callback-heavy code, it may improve readability to have the
signature indented two levels and to use OuterScope."
bmcweb is very callback heavy code. Try to enable it and see if that
improves things. There are many cases where the length of a lambda call
will change, and reindent the entire lambda function. This is really
bad for code reviews, as it's difficult to see the lines changed. This
commit should resolve it. This does have the downside of reindenting a
lot of functions, which is unfortunate, but probably worth it in the
long run.
All changes except for the .clang-format file were made by the robot.
Tested: Code compiles, whitespace changes only.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ib4aa2f1391fada981febd25b67dcdb9143827f43
|
|
As the patch at
https://gerrit.openbmc-project.xyz/c/openbmc/bmcweb/+/50994 can attest,
parsing urls with a regex is error prone. We should avoid it where
possible, and we have boost::urls that implements a full, correct, and
unit tested parser.
Ideally, eventually this helper function would devolve into just the
parse_uri, and setting defaults portion, and we could rely on the
boost::urls::url class to pass into things like http_client.
As a side note, because boost url implements port as a proper type-safe
uint16, some interfaces that previously accepted port by std::string&
needed to be modified, and is included in this patch.
Also, once moved, the branch on the ifdef for HTTP push support was
failing a clang-tidy validation. This is a known limitation of using
ifdefs for our code, and something we've solved with the header file, so
move the http push enabler to the header file.
Also note that given this reorganization, two EXPECT statements are
added to the unit tests for user input behaviors that the old code
previously did not handle properly.
Tested: Unit tests passing
Ran Redfish-Event-Listener, saw subscription create properly:
Subcription is successful for https://192.168.7.2, /redfish/v1/EventService/Subscriptions/2197426973
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ia4127c6cbcde6002fe8a50348792024d1d615e8f
|
|
In the upcoming fmt patch, we remove the use of streams, and a number of
our logging statements are relying on them. This commit changes them to
no longer rely on operator>> or operator+ to build their strings. This
alone isn't very useful, but in the context of the next patch makes the
automation able to do a complete conversion of all log statements
automatically.
Tested: enabled logging on local and saw log statements print to console
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I0e5dc2cf015c6924037e38d547535eda8175a6a1
|
|
Quite a few places we've disobeyed this rule, so simply ignore them for
now to avoid new issues popping up.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I3e518a8e8742279afb3ad1a9dad54006ed109fb1
|
|
Part of enforcing cpp core guidelines involves explicitly including all
constructors required on a non-trivial class. We were missing quite a
few. In all cases, the copy/move/and operator= methods are simply
deleted.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: Ie8d6e8bf2bc311fa21a9ae48b0d61ee5c1940999
|
|
clang-tidy added cppcoreguidelines-init-variables as a check, which is
something we already enforce to some extent, but getting CI to enforce
it will help reviews move faster.
Tested: Code compiles. Noop changes.
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I7e10950de617b1d3262265572b1703f2e60b69d0
|
|
Clang-13 adds new checks we can turn on, which find quite a few errors.
Tested: Code compiles
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I74b780760014c898cc440b37aea640b33e91c439
|
|
The http client at bmcweb does not resolve the client's
hostname asynchronously
This commit implements the async_resolve by using systemd resolved.
The async dbus message to resolvd.service is sent when a subscriber
successfully subscribes for events. The method ResolveHostname is
used to resolve the subscriber's hostname
Tested by:
Subscribe for the events at BMC using DMTF event listener
Generate an event and see the same is received at the listener's console
Signed-off-by: Sunitha Harish <sunithaharish04@gmail.com>
Change-Id: I3ab8206ac4764cfa025e94c06407524d6ba220e0
|
