kernel, logging: Pass Logger instances to kernel objects - #30342
kernel, logging: Pass Logger instances to kernel objects#30342ryanofsky wants to merge 18 commits into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30342. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK. |
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
Updated 4542335 -> db1b9f7 ( |
Change LogInstance() function to no longer allocate (and leak) a BCLog::Logger instance. Instead allow kernel applications to initialize their own logging instances that can be returned by LogInstance(). This change is somewhat useful by itself, but more useful in combination with bitcoin#30342. By itself, it gives kernel applications control over when the Logger is created and destroyed and allows new instances to replace old ones. In combination with bitcoin#30342 it allows multiple log instances to exist at the same time, and different output to be sent to different instances. This commit is built on top of bitcoin#30141 since it simplifies the implementation somewhat.
Current kernel logging API is too complicated and too restrictive. This PR tries to improves it. I'd expect an API that supported logging to allow creating log streams with different options and providing some way to specify which library operations should be logged to which streams. By contrast, the current API allows creating multiple log streams, but all the streams receive the same messages because options can only be set globally and the stream objects can't be passed to any kernel API functions. They are not even referenced by the `btck_Context` struct which holds other shared state. If no log streams are created, log messages are generated anyway, but they are stored in a 1MB buffer and not sent anywhere, unless a log stream is created later, at which point they are sent in bulk to that stream. More log streams can be created after that, but they only receive new messages, not the buffered ones. If log output is not needed, a btck_logging_disable() call is required to prevent log messages from accumulating in the 1MB buffer. Calling this will abort the program if any log streams were created before it was called, and also abort the program if any new log streams are created later. None of these behaviors seem necessary or ideal, and it would be better to provide a simpler logging API that allows creating a log stream, setting options on it, registering it with the `btck_Context` instance and receiving log messages from it. Another advantage of this approach is that it could allow (with bitcoin#30342) different log streams to be used for different purposes, which would be not be possible with the current interface.
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Pass Logger instances to BlockManager, CCoinsViewDB, CDBWrapper, ChainstateManager, and CoinsViews instances so libbitcoinkernel applications and test code have the option to control where log output goes instead of having all output sent to the global logger. This commit just passes the logger objects without using them. The next commit updates log print statements to use the new objects.
Add option to require context objects to be passed to log macros in a file or block of code, so new logging calls can't forget to specify them and accidentally log to the global logging stream.
This is a mechanical change updating kernel code that currently uses the global log instance to log to local instances instead.
Change LogInstance() function to no longer allocate (and leak) a BCLog::Logger instance. Instead allow kernel applications to initialize their own logging instances that can be returned by LogInstance(). The LogInstance() function is not actually used for the majority of logging in kernel code. Most kernel log output goes directly to BCLog::Logger instances specified when kernel objects like ChainstateManager and CTxMemPool are constructed, which allows applications to create multiple Logger instances and control which log output is sent to them. The only kernel code that does rely on LogInstance() is certain low level code in the util library, like the RNG seeder, that is not passed a specific instance and needs to rely on the global LogInstance() function. Other code outside the kernel library uses LogInstance() heavily, and may continue to do so. bitcoind and test code now just create a log instance before the first LogInstance() call, which it returns, so all behavior is the same as before.
Needed due to base PR, can be dropped before merge
4 participants
Pass
Loggerinstances toBlockManager,CCoinsViewDB,CDBWrapper,Chainstate,ChainstateManager,CoinsViews, andCTxMemPoolclasses so libbitcoinkernel applications and tests have the option to control where log output goes instead of having all output sent to the global logger.This PR is an alternative to #30338. It is more limited because it only changes kernel code while leaving other parts of the codebase alone. It also takes the opportunity to migrate legacy LogPrint / LogPrintf calls to the new log macros introduced in #28318.
This is based on #34778 + #29256 + #33847. The non-base commits are:
775344b1f0drefactor: Pass Logger instances to kernel objects6106a57a2falogging: Add LOG_REQUIRE_CONTEXT option6c1f3aedd01refactor: Log kernel output to local log instancesdbe85f39723kernel: Drop global Logger instance