RFC: add logging functionality #191
Conversation
dodsonmg
requested review from
alexandruag,
andreeaflorescu,
gsserge and
lauralt
as code owners
dodsonmg
force-pushed
the
add_logging
branch
from
c938ffd to
26ed9ad
Compare
Uses the `log` crate and implements a simple logger. Signed-off-by: Michael Dodson <[email protected]>
dodsonmg
force-pushed
the
add_logging
branch
from
26ed9ad to
dba3def
Compare
| @@ -6,7 +6,7 @@ use vm_device::bus::MmioAddress; | |||
| use vm_device::MutDeviceMmio; | |||
| use vm_superio::{rtc_pl031::NoEvents, Rtc}; | |||
|
|
|||
| use utils::debug; | |||
There was a problem hiding this comment.
The debug macro was doing something special, is this one working the same way? Specifically, we don't want to logs to be filled with data coming from the guest when we have release builds because that can be used to generate a DoS when the log file is not properly rate limited.
| pub(crate) fn init() { | ||
| // Default to Debug. | ||
| // TODO: Accept a command line argument to set the log level. | ||
| let log_level = LevelFilter::Debug; |
There was a problem hiding this comment.
We can have this as a parameter for the init function, and have a default for it (which can be LevelFilter::Debug). This would make it easier to extend it.
| // TODO: Accept a command line argument to set the log level. | ||
| let log_level = LevelFilter::Debug; | ||
|
|
||
| let log_file: Box<dyn std::io::Write + Send> = Box::new(std::io::stderr()); |
There was a problem hiding this comment.
This can also be a parameter for which we have a default.
| @@ -17,6 +17,8 @@ utils = { path = "../utils" } | |||
| vm-vcpu-ref = { path = "../vm-vcpu-ref" } | |||
| arch = { path = "../arch" } | |||
|
|
|||
| log = { version = ">=0.4", features = ["std"] } | |||
There was a problem hiding this comment.
Why is the std feature required?
|
Once we add a configuration for the logger we might find a few things worth testing. Until that point I am not sure we can do anything to test the logger that would not be a "coverage test" (i.e. a test that is not checking anything useful, and it's written just so that the coverage does not drop). |
| writeln!( | ||
| *(*(self.output.lock().unwrap())), | ||
| "vmm-reference: {:?}: <{}> {}:{}:{} -- {}", | ||
| duration, |
There was a problem hiding this comment.
Why duration instead of a wall clock time stamp?
|
Closing this PR as it is stale. @dodsonmg feel free to re-open if you want to continue working on it. |
Checking to see if this is the kind of logging implementation you're looking for to address #79.
It uses the
logcrate and implements a simple logger that I mostly copied from cloud-hypervisor:https://github.com/cloud-hypervisor/cloud-hypervisor/blob/f151a8602c1e8b2b17258aef99abd33140622839/src/main.rs#L74-L117
I left most of the
println!andeprintln!statements inmain.rsand theapicrates alone because these provide immediate feedback to the CLI user (e.g., printing a help message) and it didn't seem appropriate to redirect these tostderrwith a timestamp.I replaced remaining statements in all the crates with appropriate
info!,warn!, orerror!messages. I also replaced usage of thedebug!macro from theutilscrate with thedebug!macro provided by thelogcrate.The implementation defaults to the
Debuglevel, meaning only log entries initiated with thetrace!macro are filtered out. Alternately, I could implement a command line argument to set the log level, similar to the cloud-hypervisor implementation.Also, I would appreciate feedback on how I might go about writing unit tests for the logger module. The tests I've found elsewhere (e.g., for firecracker) support a much more complex logger implementation.