From 46533006a1578df5f1e3589b9f4eb8d50856a9d7 Mon Sep 17 00:00:00 2001 From: Abhijit Gadgil Date: Mon, 18 Jan 2021 17:47:13 +0530 Subject: [PATCH] Implementation of Builder pattern for Config Implemented `Builder` for `VMMConfig`. (Note: This is an early implementation, more like prototype. But almost all functionality is present.) Removed `derive(Default)` from `MemoryConfig`, `KernelConfig` and `VcpuConfig` and added sensible defaults (memory = 256M, vcpu=1) (derive default will trivially initialize this to 0 and hence we are required to take a memory coniguration from CLI launcher. Brought these changes to `api` (`CLI`) to demonstrate a use. Removed `required` from 'memory' and 'vcpu' config. Changed `TryFrom` to `TryFrom<&str>`. Believe this is better because it's quite possible to get `&str` from `String` than other way round and in fact often we'll get `&str` from CLI parsing etc. But the other way round is painful. Right now passes all 'existing' tests (`cargo test --all`, except one which likely fails because the right kernel image is not available to test.) TODO: Documentation with examples for using the `Builder` and Test cases for the `Builder`. Signed-off-by: Abhijit Gadgil --- src/api/src/lib.rs | 62 +++------------- src/vmm/src/config/arg_parser.rs | 6 +- src/vmm/src/config/builder.rs | 110 ++++++++++++++++++++++++++++ src/vmm/src/config/mod.rs | 121 ++++++++++++++++++++----------- src/vmm/src/lib.rs | 4 +- 5 files changed, 205 insertions(+), 98 deletions(-) create mode 100644 src/vmm/src/config/builder.rs diff --git a/src/api/src/lib.rs b/src/api/src/lib.rs index 3ba5bb8a..d70cea23 100644 --- a/src/api/src/lib.rs +++ b/src/api/src/lib.rs @@ -6,11 +6,10 @@ //! CLI for the Reference VMM. #![deny(missing_docs)] -use std::convert::TryFrom; use std::result; use clap::{App, Arg}; -use vmm::{BlockConfig, KernelConfig, MemoryConfig, NetConfig, VMMConfig, VcpuConfig}; +use vmm::VMMConfig; /// Command line parser. pub struct CLI; @@ -26,17 +25,13 @@ impl CLI { .arg( Arg::with_name("memory") .long("memory") - .required(true) .takes_value(true) - .default_value("size_mib=256") .help("Guest memory configuration.\n\tFormat: \"size_mib=\""), ) .arg( Arg::with_name("vcpu") .long("vcpu") - .required(true) .takes_value(true) - .default_value("num=1") .help("vCPU configuration.\n\tFormat: \"num=\""), ) .arg( @@ -71,52 +66,14 @@ impl CLI { format!("Invalid command line arguments: {}", e) })?; - Ok(VMMConfig { - memory_config: MemoryConfig::try_from( - matches - .value_of("memory") - .expect("Missing memory configuration") - .to_string(), - ) - .map_err(|e| { - eprintln!("{}", help_msg); - format!("{}", e) - })?, - kernel_config: KernelConfig::try_from( - matches - .value_of("kernel") - .expect("Missing kernel configuration") - .to_string(), - ) - .map_err(|e| { - eprintln!("{}", help_msg); - format!("{}", e) - })?, - vcpu_config: VcpuConfig::try_from( - matches - .value_of("vcpu") - .expect("Missing vCPU configuration") - .to_string(), - ) - .map_err(|e| { - eprintln!("{}", help_msg); - format!("{}", e) - })?, - block_config: match matches.value_of("block") { - Some(value) => Some(BlockConfig::try_from(value.to_string()).map_err(|e| { - eprintln!("{}", help_msg); - format!("{}", e) - })?), - None => None, - }, - network_config: match matches.value_of("net") { - Some(value) => Some(NetConfig::try_from(value.to_string()).map_err(|e| { - eprintln!("{}", help_msg); - format!("{}", e) - })?), - None => None, - }, - }) + Ok(VMMConfig::builder() + .memory_config(matches.value_of("memory")) + .kernel_config(matches.value_of("kernel")) + .vcpu_config(matches.value_of("vcpu")) + .network_config(matches.value_of("network")) + .block_config(matches.value_of("block")) + .build() + .map_err(|e| format!("{:?}", e))?) } } @@ -126,6 +83,7 @@ mod tests { use std::path::PathBuf; use vmm::DEFAULT_KERNEL_CMDLINE; + use vmm::{KernelConfig, MemoryConfig, VcpuConfig}; #[test] fn test_launch() { diff --git a/src/vmm/src/config/arg_parser.rs b/src/vmm/src/config/arg_parser.rs index 44beb3cd..a00afa3e 100644 --- a/src/vmm/src/config/arg_parser.rs +++ b/src/vmm/src/config/arg_parser.rs @@ -27,7 +27,7 @@ pub(super) struct CfgArgParser { } impl CfgArgParser { - pub(super) fn new(input: String) -> Self { + pub(super) fn new(input: &str) -> Self { let args = input .split(',') .filter(|tok| !tok.is_empty()) @@ -89,7 +89,7 @@ mod tests { #[test] fn test_cfg_arg_parse() -> Result<(), CfgArgParseError> { - let input_params = "path=/path,string=HelloWorld,int=123,u8=1".to_string(); + let input_params = "path=/path,string=HelloWorld,int=123,u8=1"; let mut arg_parser = CfgArgParser::new(input_params); // No parameter was consumed yet @@ -119,7 +119,7 @@ mod tests { // All params were consumed: assert!(arg_parser.all_consumed().is_ok()); - let input_params = "path=".to_string(); + let input_params = "path="; assert!(CfgArgParser::new(input_params) .value_of::("path")? .is_none()); diff --git a/src/vmm/src/config/builder.rs b/src/vmm/src/config/builder.rs new file mode 100644 index 00000000..7d99ed4d --- /dev/null +++ b/src/vmm/src/config/builder.rs @@ -0,0 +1,110 @@ +//! Config builder +use std::convert::TryFrom; + +use super::{ + BlockConfig, ConfigResult, ConversionError, KernelConfig, MemoryConfig, NetConfig, VMMConfig, + VcpuConfig, +}; + +/// Builder structure for VMMConfig +#[derive(Debug)] +pub struct Builder { + inner: ConfigResult, +} + +impl Default for Builder { + fn default() -> Self { + Self::new() + } +} + +impl Builder { + pub fn new() -> Self { + Builder { + inner: Ok(VMMConfig::default()), + } + } + + pub fn build(&self) -> ConfigResult { + self.inner.clone() + } + + pub fn memory_config(self, memory: Option) -> Self + where + MemoryConfig: TryFrom, + >::Error: Into, + { + match memory { + Some(m) => self.and_then(move |mut config| { + config.memory_config = TryFrom::try_from(m).map_err(Into::into)?; + Ok(config) + }), + None => self, + } + } + + pub fn vcpu_config(self, vcpu: Option) -> Self + where + VcpuConfig: TryFrom, + >::Error: Into, + { + match vcpu { + Some(v) => self.and_then(move |mut config| { + config.vcpu_config = TryFrom::try_from(v).map_err(Into::into)?; + Ok(config) + }), + None => self, + } + } + + pub fn kernel_config(self, kernel: Option) -> Self + where + KernelConfig: TryFrom, + >::Error: Into, + { + match kernel { + Some(k) => self.and_then(move |mut config| { + config.kernel_config = TryFrom::try_from(k).map_err(Into::into)?; + Ok(config) + }), + None => self, + } + } + + pub fn network_config(self, network: Option) -> Self + where + NetConfig: TryFrom, + >::Error: Into, + { + match network { + Some(n) => self.and_then(move |mut config| { + config.network_config = Some(TryFrom::try_from(n).map_err(Into::into)?); + Ok(config) + }), + None => self, + } + } + + pub fn block_config(self, block: Option) -> Self + where + BlockConfig: TryFrom, + >::Error: Into, + { + match block { + Some(b) => self.and_then(move |mut config| { + config.block_config = Some(TryFrom::try_from(b).map_err(Into::into)?); + Ok(config) + }), + None => self, + } + } + + fn and_then(self, func: F) -> Self + where + F: FnOnce(VMMConfig) -> ConfigResult, + { + Builder { + inner: self.inner.and_then(func), + } + } +} diff --git a/src/vmm/src/config/mod.rs b/src/vmm/src/config/mod.rs index 8586dc10..057922a7 100644 --- a/src/vmm/src/config/mod.rs +++ b/src/vmm/src/config/mod.rs @@ -11,10 +11,14 @@ use arg_parser::CfgArgParser; use super::{DEFAULT_HIGH_RAM_START, DEFAULT_KERNEL_CMDLINE}; +use self::builder::Builder; + mod arg_parser; +mod builder; + /// Errors encountered converting the `*Config` objects. -#[derive(Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub enum ConversionError { /// Failed to parse the string representation for the kernel. ParseKernel(String), @@ -46,6 +50,13 @@ impl ConversionError { } } +impl VMMConfig { + /// Builds a builder + pub fn builder() -> Builder { + Builder::new() + } +} + impl fmt::Display for ConversionError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { use ConversionError::*; @@ -60,16 +71,24 @@ impl fmt::Display for ConversionError { } /// Guest memory configurations. -#[derive(Clone, Debug, Default, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub struct MemoryConfig { /// Guest memory size in MiB. pub size_mib: u32, } -impl TryFrom for MemoryConfig { +impl Default for MemoryConfig { + fn default() -> Self { + MemoryConfig { + size_mib: 256 as u32, + } + } +} + +impl TryFrom<&str> for MemoryConfig { type Error = ConversionError; - fn try_from(mem_cfg_str: String) -> result::Result { + fn try_from(mem_cfg_str: &str) -> result::Result { // Supported options: `size=` let mut arg_parser = CfgArgParser::new(mem_cfg_str); @@ -85,16 +104,22 @@ impl TryFrom for MemoryConfig { } /// vCPU configurations. -#[derive(Clone, Debug, Default, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub struct VcpuConfig { /// Number of vCPUs. pub num: u8, } -impl TryFrom for VcpuConfig { +impl Default for VcpuConfig { + fn default() -> Self { + VcpuConfig { num: 1 as u8 } + } +} + +impl TryFrom<&str> for VcpuConfig { type Error = ConversionError; - fn try_from(vcpu_cfg_str: String) -> result::Result { + fn try_from(vcpu_cfg_str: &str) -> result::Result { // Supported options: `num=` let mut arg_parser = CfgArgParser::new(vcpu_cfg_str); let num = arg_parser @@ -109,7 +134,7 @@ impl TryFrom for VcpuConfig { } /// Guest kernel configurations. -#[derive(Clone, Debug, Default, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub struct KernelConfig { /// Kernel command line. pub cmdline: String, @@ -119,10 +144,20 @@ pub struct KernelConfig { pub himem_start: u64, } -impl TryFrom for KernelConfig { +impl Default for KernelConfig { + fn default() -> Self { + KernelConfig { + cmdline: String::from(DEFAULT_KERNEL_CMDLINE), + path: PathBuf::from(""), // FIXME: make it a const. + himem_start: DEFAULT_HIGH_RAM_START, + } + } +} + +impl TryFrom<&str> for KernelConfig { type Error = ConversionError; - fn try_from(kernel_cfg_str: String) -> result::Result { + fn try_from(kernel_cfg_str: &str) -> result::Result { // Supported options: // `cmdline=<"string">,path=/path/to/kernel,himem_start=` // Required: path @@ -153,16 +188,16 @@ impl TryFrom for KernelConfig { } } /// Network device configuration. -#[derive(Clone, Debug, Default, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub struct NetConfig { /// Name of tap device. pub tap_name: String, } -impl TryFrom for NetConfig { +impl TryFrom<&str> for NetConfig { type Error = ConversionError; - fn try_from(net_config_str: String) -> Result { + fn try_from(net_config_str: &str) -> Result { // Supported options: `tap=String` let mut arg_parser = CfgArgParser::new(net_config_str); @@ -179,16 +214,16 @@ impl TryFrom for NetConfig { } /// Block device configuration -#[derive(Clone, Debug, Default, PartialEq)] +#[derive(Clone, Debug, PartialEq)] pub struct BlockConfig { /// Path to the block device backend. pub path: PathBuf, } -impl TryFrom for BlockConfig { +impl TryFrom<&str> for BlockConfig { type Error = ConversionError; - fn try_from(block_cfg_str: String) -> Result { + fn try_from(block_cfg_str: &str) -> Result { // Supported options: `path=PathBuf` let mut arg_parser = CfgArgParser::new(block_cfg_str); @@ -219,6 +254,8 @@ pub struct VMMConfig { pub block_config: Option, } +pub(super) type ConfigResult = Result; + #[cfg(test)] mod tests { use super::*; @@ -233,18 +270,18 @@ mod tests { path: PathBuf::from("/foo/bar"), }; assert_eq!( - KernelConfig::try_from(kernel_str.to_string()).unwrap(), + KernelConfig::try_from(kernel_str).unwrap(), expected_kernel_config ); // Check that an empty path returns a conversion error. let kernel_str = r#"path=,cmdline="foo=bar",himem_start=42,"#; assert_eq!( - KernelConfig::try_from(kernel_str.to_string()).unwrap_err(), + KernelConfig::try_from(kernel_str).unwrap_err(), ConversionError::ParseKernel("Missing required argument: path".to_string()) ); - assert!(KernelConfig::try_from("path=/something,not=valid".to_string()).is_err()); - assert!(KernelConfig::try_from("path=/something,himem_start=invalid".to_string()).is_err()); + assert!(KernelConfig::try_from("path=/something,not=valid").is_err()); + assert!(KernelConfig::try_from("path=/something,himem_start=invalid").is_err()); } #[test] @@ -252,7 +289,7 @@ mod tests { // Invalid vCPU numbers: 0, 256 (exceeds the u8 limit). let vcpu_str = "num=0"; assert_eq!( - VcpuConfig::try_from(vcpu_str.to_string()).unwrap_err(), + VcpuConfig::try_from(vcpu_str).unwrap_err(), ConversionError::ParseVcpus( "Param \'num\', parsing failed: number would be zero for non-zero type".to_string() ) @@ -260,7 +297,7 @@ mod tests { let vcpu_str = "num=256"; assert_eq!( - VcpuConfig::try_from(vcpu_str.to_string()).unwrap_err(), + VcpuConfig::try_from(vcpu_str).unwrap_err(), ConversionError::ParseVcpus( "Param 'num', parsing failed: number too large to fit in target type".to_string() ) @@ -268,20 +305,20 @@ mod tests { // Missing vCPU number in config string, use default let vcpu_str = "num="; - assert!(VcpuConfig::try_from(vcpu_str.to_string()).is_ok()); + assert!(VcpuConfig::try_from(vcpu_str).is_ok()); // vCPU number parsing error let vcpu_str = "num=abc"; - assert!(VcpuConfig::try_from(vcpu_str.to_string()).is_err()); + assert!(VcpuConfig::try_from(vcpu_str).is_err()); // Extra argument let vcpu_str = "num=1,foo=bar"; - assert!(VcpuConfig::try_from(vcpu_str.to_string()).is_err()); + assert!(VcpuConfig::try_from(vcpu_str).is_err()); } #[test] fn test_network_config() { - let network_str = "tap=vmtap".to_string(); + let network_str = "tap=vmtap"; let network_cfg = NetConfig::try_from(network_str).unwrap(); let expected_cfg = NetConfig { tap_name: "vmtap".to_string(), @@ -289,24 +326,24 @@ mod tests { assert_eq!(network_cfg, expected_cfg); // Test case: empty string error. - assert!(NetConfig::try_from(String::new()).is_err()); + assert!(NetConfig::try_from("").is_err()); // Test case: empty tap name error. - let network_str = "tap=".to_string(); + let network_str = "tap="; assert!(NetConfig::try_from(network_str).is_err()); // Test case: invalid string. - let network_str = "blah=blah".to_string(); + let network_str = "blah=blah"; assert!(NetConfig::try_from(network_str).is_err()); // Test case: unused parameters - let network_str = "tap=something,blah=blah".to_string(); + let network_str = "tap=something,blah=blah"; assert!(NetConfig::try_from(network_str).is_err()); } #[test] fn test_block_config() { - let block_str = "path=/foo/bar".to_string(); + let block_str = "path=/foo/bar"; let block_cfg = BlockConfig::try_from(block_str).unwrap(); let expected_cfg = BlockConfig { path: PathBuf::from("/foo/bar"), @@ -314,49 +351,49 @@ mod tests { assert_eq!(block_cfg, expected_cfg); // Test case: empty string error. - assert!(BlockConfig::try_from(String::new()).is_err()); + assert!(BlockConfig::try_from("").is_err()); - // Test case: empty path error. - let block_str = "path=".to_string(); + // Test case: empty tap name error. + let block_str = "path="; assert!(BlockConfig::try_from(block_str).is_err()); // Test case: invalid string. - let block_str = "blah=blah".to_string(); + let block_str = "blah=blah"; assert!(BlockConfig::try_from(block_str).is_err()); // Test case: unused parameters - let block_str = "path=/foo/bar,blah=blah".to_string(); + let block_str = "path=/foo/bar,blah=blah"; assert!(BlockConfig::try_from(block_str).is_err()); } #[test] fn test_memory_config() { let default = MemoryConfig { size_mib: 256 }; - let size_str = "size_mib=42".to_string(); + let size_str = "size_mib=42"; let memory_cfg = MemoryConfig::try_from(size_str).unwrap(); let expected_cfg = MemoryConfig { size_mib: 42 }; assert_eq!(memory_cfg, expected_cfg); // Test case: empty string should use default - assert_eq!(MemoryConfig::try_from(String::new()).unwrap(), default); + assert_eq!(MemoryConfig::try_from("").unwrap(), default); // Test case: empty size_mib, use default - let memory_str = "size_mib=".to_string(); + let memory_str = "size_mib="; assert!(MemoryConfig::try_from(memory_str).is_ok()); // Test case: size_mib invalid input - let memory_str = "size_mib=ciao".to_string(); + let memory_str = "size_mib=ciao"; assert!(MemoryConfig::try_from(memory_str).is_err()); // Test case: invalid string. - let memory_str = "blah=blah".to_string(); + let memory_str = "blah=blah"; assert_eq!( MemoryConfig::try_from(memory_str).unwrap_err(), ConversionError::ParseMemory("Unknown arguments found: \'blah\'".to_string()) ); // Test case: unused parameters - let memory_str = "size_mib=12,blah=blah".to_string(); + let memory_str = "size_mib=12,blah=blah"; assert!(MemoryConfig::try_from(memory_str).is_err()); } } diff --git a/src/vmm/src/lib.rs b/src/vmm/src/lib.rs index f922ba76..4227b088 100644 --- a/src/vmm/src/lib.rs +++ b/src/vmm/src/lib.rs @@ -48,6 +48,8 @@ use vm_vcpu::vm::{self, KvmVm, VmState}; mod boot; mod config; +pub use config::*; + mod serial; /// First address past 32 bits is where the MMIO gap ends. @@ -184,7 +186,7 @@ impl TryFrom for VMM { } if !cmdline.as_str().is_empty() { - vmm.kernel_cfg.cmdline.push_str(" "); + vmm.kernel_cfg.cmdline.push(' '); vmm.kernel_cfg.cmdline.push_str(cmdline.as_str()); }