-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Logging updates prior to supporting configuration through the API #246
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
Conversation
typo in commit message: s/confiuring/configuring |
logger/src/error.rs
Outdated
@@ -24,6 +26,7 @@ impl fmt::Display for LoggerError { | |||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | |||
let printable = match *self { | |||
LoggerError::NeverInitialized(ref e) => e, | |||
LoggerError::AlreadyInitialized => "Attempt at reinitialization", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This error message is a bit nondescript, maybe change it to something like "Reinitialization of logger is not allowed" or "Logger already initialized" ?
logger/src/lib.rs
Outdated
@@ -259,7 +258,10 @@ impl Logger { | |||
"{}", | |||
INIT_RES.as_ref().err().unwrap() | |||
))); | |||
} else if INIT_RES.is_ok() && INIT_RES.as_ref().ok().unwrap() == &INITIALIZED { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- no need for .ok() before .unwrap()
- while comparing references is fine in Rust because auto DeRef() is being done ( https://www.reddit.com/r/rust/comments/2dmzf6/why_do_equality_tests_of_references_seem_to/ ), it looks a bit weird; I suggest removing the '&' from '&INITIALIZED' if the compiler doesn't complain about it.
- nit: instead of error on "state == INITIALIZED", it's better practice to error on "state != UNINITIALIZED", that way if for whatever reason a new state is added, this check is still valid and correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I did try it without references. If I call unwrap on INIT_RES it would try to move the content and given that INIT_RES is static the compiler complains about it. It does complain if I remove the &.
assert_eq!(l.show_line_numbers, false); | ||
|
||
let l = Logger::new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this change needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to be able to do the configuration of the logger (i.e. call the set functions) progressively. In the first version where the functions would return self I would only be able to do it from one call (Logger::new.set_bla.set_bla2 ...). But in the case of the API I need to be able to parse the config that I receive and call the set functions separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@acatangiu The typo is in the pr description. I updated it. The commit message looks ok to me. |
configuration of the logger from outside the crate. Signed-off-by: Diana Popa <[email protected]>
Changes
Testing
Build Time
Prerequisite
## add the necessary musl target to the active toolchain rustup target add x86_64-unknown-linux-musl
Build tests
Integration Testing
rm -f /tmp/firecracker.socket && \ target/debug/firecracker --api-sock=/tmp/firecracker.socket