Introduce more granularity to setting log levels #292
Conversation
floss/main.py
Outdated
| # WARNING:plugins.arithmetic_plugin.XORPlugin:identify: Invalid instruction encountered in basic block, skipping: 0x4a0637 | ||
| logging.getLogger("floss.plugins.arithmetic_plugin.XORPlugin").setLevel(logging.ERROR) | ||
| logging.getLogger("floss.plugins.arithmetic_plugin.ShiftPlugin").setLevel(logging.ERROR) | ||
| set_log_config(should_debug, should_verbose) |
There was a problem hiding this comment.
This should not be indented so deeply, correct? I think we'd want to call this routine for all cases?
There was a problem hiding this comment.
The only reason I didn't call set_log_config() from every case is because I figured logging.BasicConfig() would do the same thing, and it'd be redundant. set_log_config() is called only if logging.BasicConfig() sets the loggers to the WARNING level, because set_log_config() sets most of the loggers to ERROR level, and what's not explicitly set is already set by logging.BasicConfig(). Calling set_log_config() when logging.BasicConfig() has already set the loggers to DEBUG or INFO level would just accomplish the same thing twice it'd think.
There was a problem hiding this comment.
I think that this routine should be called from all cases (or de-dented) for symmetry. Maybe there is some duplication, but I believe this is ok, since otherwise, its confusing why the final case is special.
FYI, this type of reasoning is the perfect type of explanation to go into code comments.
There was a problem hiding this comment.
Ok I'll change that. Sorry yeah I should have documented more.
floss/main.py
Outdated
| @@ -207,6 +207,98 @@ def make_parser(): | |||
|
|
|||
| return parser | |||
|
|
|||
| def set_log_config(should_debug=False, should_verbose=False): | |||
There was a problem hiding this comment.
This function sets logger levels, but doesn't do any other configuration. The other function (currently: set_logging_level) both sets logger levels and does configuration. This naming is confusing to me, and I think it should be swapped:
set_log_levels: this functionset_log_config: the other function, callslogging.basicConfigand alsoset_log_levels
There was a problem hiding this comment.
You're entirely correct. My bad for not naming the functions better.
floss/main.py
Outdated
| # WARNING:plugins.arithmetic_plugin.XORPlugin:identify: Invalid instruction encountered in basic block, skipping: 0x4a0637 | ||
| logging.getLogger("floss.plugins.arithmetic_plugin.XORPlugin").setLevel(logging.ERROR) | ||
| logging.getLogger("floss.plugins.arithmetic_plugin.ShiftPlugin").setLevel(logging.ERROR) | ||
| set_log_config(should_debug, should_verbose) |
There was a problem hiding this comment.
I think that this routine should be called from all cases (or de-dented) for symmetry. Maybe there is some duplication, but I believe this is ok, since otherwise, its confusing why the final case is special.
FYI, this type of reasoning is the perfect type of explanation to go into code comments.
floss/main.py
Outdated
| # ignore messages like: | ||
| # DEBUG: identify: suspicious MOV instruction at 0x00401017 in function 0x00401000: mov byte [edx],al | ||
| logging.getLogger("floss.plugins.mov_plugin.MovPlugin").setLevel(log_level) | ||
|
|
||
|
|
||
| def set_logging_level(should_debug=False, should_verbose=False): |
There was a problem hiding this comment.
let's change the name of this routine, as described above.
|
LGTM! thanks @capnspacehook! |
As discussed in #291, the current method of setting Floss's logging levels when used as a library is inadequate. Either the user has to manually set the 20 or so loggers Floss uses to the level they desire, or use the floss.main.set_logging_level() function, which has the unpleasant side effect of removing all of the root logger's handlers.
This creates a new function, floss.main.set_log_config(), which has the exact same interface as floss.main.set_logging_level(), but sets ALL of Floss's loggers to the specified level. set_logging_level() is changed to call set_log_config() internally to remove code duplication.