From 8551a83a2b256242db5ed05dcfdcdc2e0768f97a Mon Sep 17 00:00:00 2001 From: Jonathan Woollett-Light Date: Wed, 15 Nov 2023 10:40:52 +0000 Subject: [PATCH 1/2] fix: Support all logger level case variants https://github.com/firecracker-microvm/firecracker/pull/4047 updated parsing the logger level filter. It removed all case variants outside fully uppercase (e.g. `INFO`) and the 1st character being upper case (e.g. `Info`) this removed support for other previously supported variants e.g. `info`. This commit re-introduces this support such that all variants should again be supported. Fixes commit 332f2184ae29265fbae1648469e073fc7f987378 Signed-off-by: Jonathan Woollett-Light Signed-off-by: Sudan Landge --- src/vmm/src/logger/logging.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/vmm/src/logger/logging.rs b/src/vmm/src/logger/logging.rs index 7f45e4d2499..d111994363a 100644 --- a/src/vmm/src/logger/logging.rs +++ b/src/vmm/src/logger/logging.rs @@ -242,13 +242,13 @@ pub struct LevelFilterFromStrError(String); impl FromStr for LevelFilter { type Err = LevelFilterFromStrError; fn from_str(s: &str) -> Result { - match s { - "Off" | "OFF" => Ok(Self::Off), - "Trace" | "TRACE" => Ok(Self::Trace), - "Debug" | "DEBUG" => Ok(Self::Debug), - "Info" | "INFO" => Ok(Self::Info), - "Warn" | "WARN" | "Warning" | "WARNING" => Ok(Self::Warn), - "Error" | "ERROR" => Ok(Self::Error), + match s.to_ascii_lowercase().as_str() { + "off" => Ok(Self::Off), + "trace" => Ok(Self::Trace), + "debug" => Ok(Self::Debug), + "info" => Ok(Self::Info), + "warn" | "warning" => Ok(Self::Warn), + "error" => Ok(Self::Error), _ => Err(LevelFilterFromStrError(String::from(s))), } } @@ -301,6 +301,13 @@ mod tests { assert_eq!(LevelFilter::from_str("Warn"), Ok(LevelFilter::Warn)); assert_eq!(LevelFilter::from_str("Error"), Ok(LevelFilter::Error)); + assert_eq!(LevelFilter::from_str("off"), Ok(LevelFilter::Off)); + assert_eq!(LevelFilter::from_str("trace"), Ok(LevelFilter::Trace)); + assert_eq!(LevelFilter::from_str("debug"), Ok(LevelFilter::Debug)); + assert_eq!(LevelFilter::from_str("info"), Ok(LevelFilter::Info)); + assert_eq!(LevelFilter::from_str("warn"), Ok(LevelFilter::Warn)); + assert_eq!(LevelFilter::from_str("error"), Ok(LevelFilter::Error)); + assert_eq!(LevelFilter::from_str("OFF"), Ok(LevelFilter::Off)); assert_eq!(LevelFilter::from_str("TRACE"), Ok(LevelFilter::Trace)); assert_eq!(LevelFilter::from_str("DEBUG"), Ok(LevelFilter::Debug)); @@ -308,6 +315,7 @@ mod tests { assert_eq!(LevelFilter::from_str("WARN"), Ok(LevelFilter::Warn)); assert_eq!(LevelFilter::from_str("ERROR"), Ok(LevelFilter::Error)); + assert_eq!(LevelFilter::from_str("warning"), Ok(LevelFilter::Warn)); assert_eq!(LevelFilter::from_str("Warning"), Ok(LevelFilter::Warn)); assert_eq!(LevelFilter::from_str("WARNING"), Ok(LevelFilter::Warn)); } From 08ae0d9760adea6fd21adb525abd5449e9e06f17 Mon Sep 17 00:00:00 2001 From: Sudan Landge Date: Thu, 23 Nov 2023 23:09:49 +0000 Subject: [PATCH 2/2] test: check all logger level case variants Add unit test to cover all cases of LevelFilter. Remove redundant logic from levelfilter_from_str because levelfilter_from_str_all_variants is a better place to put the logic. Signed-off-by: Jonathan Woollett-Light Signed-off-by: Sudan Landge --- Cargo.lock | 1 + src/vmm/Cargo.toml | 1 + src/vmm/src/logger/logging.rs | 96 ++++++++++++++++++++++------------- 3 files changed, 64 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7e9ff79121e..cc99b0ff7c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1494,6 +1494,7 @@ dependencies = [ "device_tree", "displaydoc", "event-manager", + "itertools 0.12.0", "kvm-bindings", "kvm-ioctls", "lazy_static", diff --git a/src/vmm/Cargo.toml b/src/vmm/Cargo.toml index e637e18e14f..a0d4b535fa6 100644 --- a/src/vmm/Cargo.toml +++ b/src/vmm/Cargo.toml @@ -50,6 +50,7 @@ vm-fdt = "0.2.0" criterion = { version = "0.5.0", default-features = false } device_tree = "1.1.0" proptest = { version = "1.0.0", default-features = false, features = ["std"] } +itertools = "0.12.0" [features] tracing = ["log-instrument"] diff --git a/src/vmm/src/logger/logging.rs b/src/vmm/src/logger/logging.rs index d111994363a..dcdb504bd5d 100644 --- a/src/vmm/src/logger/logging.rs +++ b/src/vmm/src/logger/logging.rs @@ -10,7 +10,7 @@ use std::sync::{Mutex, OnceLock}; use std::thread; use log::{Log, Metadata, Record}; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize}; use utils::time::LocalTime; use super::metrics::{IncMetric, METRICS}; @@ -200,25 +200,19 @@ pub struct LoggerConfig { /// the log level filter. It would be a breaking change to no longer support this. In the next /// breaking release this should be removed (replaced with `log::LevelFilter` and only supporting /// its default deserialization). -#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize)] pub enum LevelFilter { /// [`log::LevelFilter:Off`] - #[serde(alias = "OFF")] Off, /// [`log::LevelFilter:Trace`] - #[serde(alias = "TRACE")] Trace, /// [`log::LevelFilter:Debug`] - #[serde(alias = "DEBUG")] Debug, /// [`log::LevelFilter:Info`] - #[serde(alias = "INFO")] Info, /// [`log::LevelFilter:Warn`] - #[serde(alias = "WARN", alias = "WARNING", alias = "Warning")] Warn, /// [`log::LevelFilter:Error`] - #[serde(alias = "ERROR")] Error, } impl From for log::LevelFilter { @@ -233,6 +227,25 @@ impl From for log::LevelFilter { } } } +impl<'de> Deserialize<'de> for LevelFilter { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + use serde::de::Error; + let key = String::deserialize(deserializer)?; + let level = match key.to_lowercase().as_str() { + "off" => Ok(LevelFilter::Off), + "trace" => Ok(LevelFilter::Trace), + "debug" => Ok(LevelFilter::Debug), + "info" => Ok(LevelFilter::Info), + "warn" | "warning" => Ok(LevelFilter::Warn), + "error" => Ok(LevelFilter::Error), + _ => Err(D::Error::custom("Invalid LevelFilter")), + }; + level + } +} /// Error type for [`::from_str`]. #[derive(Debug, PartialEq, Eq, thiserror::Error)] @@ -287,37 +300,52 @@ mod tests { log::LevelFilter::Error ); } + #[test] - fn levelfilter_from_str() { + fn levelfilter_from_str_all_variants() { + use itertools::Itertools; + + #[derive(Deserialize)] + struct Foo { + #[allow(dead_code)] + level: LevelFilter, + } + + for (level, level_enum) in [ + ("off", LevelFilter::Off), + ("trace", LevelFilter::Trace), + ("debug", LevelFilter::Debug), + ("info", LevelFilter::Info), + ("warn", LevelFilter::Warn), + ("warning", LevelFilter::Warn), + ("error", LevelFilter::Error), + ] { + let multi = level.chars().map(|_| 0..=1).multi_cartesian_product(); + for combination in multi { + let variant = level + .chars() + .zip_eq(combination) + .map(|(c, v)| match v { + 0 => c.to_ascii_lowercase(), + 1 => c.to_ascii_uppercase(), + _ => unreachable!(), + }) + .collect::(); + + let ex = format!("{{ \"level\": \"{}\" }}", variant); + assert_eq!(LevelFilter::from_str(&variant), Ok(level_enum)); + assert!(serde_json::from_str::(&ex).is_ok(), "{ex}"); + } + } + let ex = "{{ \"level\": \"blah\" }}".to_string(); + assert!( + serde_json::from_str::(&ex).is_err(), + "expected error got {ex:#?}" + ); assert_eq!( LevelFilter::from_str("bad"), Err(LevelFilterFromStrError(String::from("bad"))) ); - - assert_eq!(LevelFilter::from_str("Off"), Ok(LevelFilter::Off)); - assert_eq!(LevelFilter::from_str("Trace"), Ok(LevelFilter::Trace)); - assert_eq!(LevelFilter::from_str("Debug"), Ok(LevelFilter::Debug)); - assert_eq!(LevelFilter::from_str("Info"), Ok(LevelFilter::Info)); - assert_eq!(LevelFilter::from_str("Warn"), Ok(LevelFilter::Warn)); - assert_eq!(LevelFilter::from_str("Error"), Ok(LevelFilter::Error)); - - assert_eq!(LevelFilter::from_str("off"), Ok(LevelFilter::Off)); - assert_eq!(LevelFilter::from_str("trace"), Ok(LevelFilter::Trace)); - assert_eq!(LevelFilter::from_str("debug"), Ok(LevelFilter::Debug)); - assert_eq!(LevelFilter::from_str("info"), Ok(LevelFilter::Info)); - assert_eq!(LevelFilter::from_str("warn"), Ok(LevelFilter::Warn)); - assert_eq!(LevelFilter::from_str("error"), Ok(LevelFilter::Error)); - - assert_eq!(LevelFilter::from_str("OFF"), Ok(LevelFilter::Off)); - assert_eq!(LevelFilter::from_str("TRACE"), Ok(LevelFilter::Trace)); - assert_eq!(LevelFilter::from_str("DEBUG"), Ok(LevelFilter::Debug)); - assert_eq!(LevelFilter::from_str("INFO"), Ok(LevelFilter::Info)); - assert_eq!(LevelFilter::from_str("WARN"), Ok(LevelFilter::Warn)); - assert_eq!(LevelFilter::from_str("ERROR"), Ok(LevelFilter::Error)); - - assert_eq!(LevelFilter::from_str("warning"), Ok(LevelFilter::Warn)); - assert_eq!(LevelFilter::from_str("Warning"), Ok(LevelFilter::Warn)); - assert_eq!(LevelFilter::from_str("WARNING"), Ok(LevelFilter::Warn)); } #[test]