Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unused variable fc::logging_config cfg in cli_wallet/main.cpp #1149

Closed
ihla opened this issue Jul 18, 2018 · 6 comments
Closed

unused variable fc::logging_config cfg in cli_wallet/main.cpp #1149

ihla opened this issue Jul 18, 2018 · 6 comments
Assignees
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 CLI Impact flag identifying the command line interface (CLI) wallet application code cleanup

Comments

@ihla
Copy link

ihla commented Jul 18, 2018

The logging config is created but never used. I see in the "Initial commit" the commented line //fc::configure_logging( cfg ); probably deleted later on. We should either delete the dead code or put the fc::configure_logging( cfg ); back - what is desired? (Now there is a print on cosole saying "Logging RPC to file: logs/rpc/rpc.log" but logging is not happening!)

@abitmore
Copy link
Member

abitmore commented Jul 18, 2018

CLI logging makes some sense when running in daemon mode.

But please be careful don't log sensitive info, e.g. wallet passwords or private keys.

@ryanRfox ryanRfox added 1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 CLI Impact flag identifying the command line interface (CLI) wallet application labels Jul 18, 2018
@jmjatlanta
Copy link
Contributor

I'm sorry, but I am missing something. I find no configure_logging in cli_wallet/main.cpp. Can you please clarify where the dead code is? I've checked the develop and master branches.

@ihla
Copy link
Author

ihla commented Jul 20, 2018

I consider these code lines as the dead code because the cfg is created but never passed to fc::configure_logging():

fc::logging_config cfg;

cfg.appenders.push_back(fc::appender_config( "default", "console", fc::variant(fc::console_appender::config(), 20)));
cfg.appenders.push_back(fc::appender_config( "rpc", "file", fc::variant(ac, 5)));
cfg.loggers = { fc::logger_config("default"), fc::logger_config( "rpc") };
cfg.loggers.front().level = fc::log_level::info;
cfg.loggers.front().appenders = {"default"};
cfg.loggers.back().level = fc::log_level::debug;
cfg.loggers.back().appenders = {"rpc"};

@ihla
Copy link
Author

ihla commented Jul 20, 2018

line fc::configure_logging(cfg); is missing in order to make the logging to work

@jmjatlanta
Copy link
Contributor

Thank you for setting me straight. I now understand your OP. It sure would be nice to configure logging via a configuration file, but that is certainly a separate issue.

I'm with abitmore on this one. I believe we should call configure_logging( cfg ) for those using the wallet in daemon mode.

@oxarbitrage
Copy link
Member

closed by #1528

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 CLI Impact flag identifying the command line interface (CLI) wallet application code cleanup
Projects
None yet
Development

No branches or pull requests

5 participants