Refactor logging code into a global object
AbandonedPublic
Actions

Authored by jimpo on Apr 11 2018, 07:27.

Details

Reviewers

jasonbcox
deadalnix

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Summary

Refactor logging-related code out of util.{h,cpp} to a Logger class. A global logger object encapsulates the state. This will make it easier to add functionality to the logger such as syslog levels.

Commits:

MOVEONLY: Move logging code from util.{h,cpp} to new files.

util: Establish global logger object.

The object encapsulates logging configuration, and in a later commit,
set up routines will also be moved into the class.

util: Move debug file management functions into Logger.

util: Encapsulate logCategories within BCLog::Logger.

util: Refactor GetLogCategory.

Changing parameter types from pointers to references and uint32_t to
BCLog::LogFlags simplies calling code.

Test Plan

Run bitcoind with different permutations of:
-logtimestamps, -debug, -printtoconsole, -datadir, -shrinkdebugfile

Diff Detail

Repository

rABC Bitcoin ABC

Branch

logging

Lint

Lint Passed

Severity	Location	Code	Message
Auto-Fix	src/init.cpp:1	CFMT	Code style violation

Unit

No Test Coverage

Build Status

Buildable 2290
Build 2718: Bitcoin ABC Buildbot (legacy)
Build 2717: arc lint + arc unit

Event Timeline

jimpo created this revision.Apr 11 2018, 07:27

Owners added a reviewer: Restricted Owners Package.Apr 11 2018, 07:27

Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 11 2018, 07:27

jimpo retitled this revision from Summary: Refactor logging-related code out of util.{h,cpp} to a Logger class. A global logger object encapsulates the state. This will make it easier to add functionality to the logger such as syslog levels. to Refactor logging code into a global object.Apr 11 2018, 07:30

jimpo edited the summary of this revision. (Show Details)

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2290: Diff 3471.Apr 11 2018, 07:46

Nice! ConceptACK. Initial code review looks fine.

itshappening

Makes sense to me. Will review more closely on April 16th, after testnet testing is concluded.

Please make clang-format version 4.0 and use braces around conditionals.

This moves all kind fo code around, while defining a new interface. It is really hard to get things right that way both when writing the code and reviewing. For instance, this broke thread safety and started exposing in its API various element that where previously implementation details.

Please define a new interface while forwarding to the old one as its implementation as a first step. Then code can be migrated to use the new interfaces and implementation moved over as the use of various pieces go through the new API.

src/init.cpp
1235	g_logger vs gArgs ?
src/logging.cpp
14	This isn't thread safe.
src/logging.h
81	You just exposed that guy to the world.

This revision now requires changes to proceed.Apr 12 2018, 16:24

Yes, this diff does a lot at once, however each commit is fairly well scoped. It is much easier to review commit by commit, which I now realize is not that easy with Phabricator. The commits actually do take the approach you are describing of moving the same implementation behind a new interface, and only afterwards changing behavior. Is the recommended approach to break each commit into a separate diff?

src/init.cpp
1235	I don't understand the question. The idea is to read the command line argument and set it on the logger object, which stores configuration as public fields.
src/logging.cpp
14	No, but I'm hoping that it is in the way it is used. If the pointer is only assigned during initialization while there is only one thread and only released on program exit after all threads have been joined, it should only ever have one value when accessed concurrently. Do you think that is OK? I'd really rather not guard access to the logger with a mutex.
src/logging.h
81	You are right, this should be private.