feat(cli)!: use serde-aco to parse cli options

Signed-off-by: Changyuan Lyu <changyuanl@google.com>

Cargo.lock

@@ -24,6 +24,7 @@ dependencies = [
  "mio",
  "parking_lot",
  "rand",
+ "serde",
  "thiserror",
  "zerocopy",
 ]
@@ -37,6 +38,8 @@ dependencies = [
  "clap",
  "flexi_logger",
  "log",
+ "serde",
+ "serde-aco",
 ]
 
 [[package]]

alioth-cli/Cargo.toml

@@ -13,6 +13,8 @@ flexi_logger = "0.28"
 clap = { version = "4", features = ["derive"] }
 anyhow = "1"
 alioth = { version = "0.1.0", path = "../alioth" }
+serde.workspace = true
+serde-aco = { version = "0.1.0", path = "../serde-aco" }
 
 [[bin]]
 path = "src/main.rs"

alioth-cli/src/main.rs

@@ -12,18 +12,16 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::fs::File;
 use std::path::PathBuf;
 
 use alioth::board::BoardConfig;
-use alioth::device::fw_cfg::{FwCfgContent, FwCfgItem};
 use alioth::hv::Kvm;
 use alioth::loader::{ExecType, Payload};
 use alioth::virtio::dev::blk::BlockParam;
 use alioth::virtio::dev::entropy::EntropyParam;
 use alioth::virtio::dev::net::NetParam;
 use alioth::vm::Machine;
-use anyhow::{bail, Result};
+use anyhow::Result;
 use clap::{Args, Parser, Subcommand};
 use flexi_logger::{FileSpec, Logger};
 
@@ -90,86 +88,6 @@ struct RunArgs {
     blk: Vec<String>,
 }
 
-fn parse_fw_cfgs(s: &str) -> Result<FwCfgItem> {
-    let Some((name, ty_content)) = s.split_once(',') else {
-        bail!("invalid config: {s}")
-    };
-    let Some((ty, content)) = ty_content.split_once('=') else {
-        bail!("invalid content: {ty_content}")
-    };
-    match ty {
-        "file" => {
-            let f = File::open(content)?;
-            Ok(FwCfgItem {
-                name: name.to_owned(),
-                content: FwCfgContent::File(f),
-            })
-        }
-        "string" => Ok(FwCfgItem {
-            name: name.to_owned(),
-            content: FwCfgContent::Bytes(content.into()),
-        }),
-        _ => bail!("invalid content: {ty_content}"),
-    }
-}
-
-fn parse_mem(s: &str) -> Result<usize> {
-    if let Some((num, "")) = s.split_once(['g', 'G']) {
-        let n = num.parse::<usize>()?;
-        Ok(n << 30)
-    } else if let Some((num, "")) = s.split_once(['m', 'M']) {
-        let n = num.parse::<usize>()?;
-        Ok(n << 20)
-    } else if let Some((num, "")) = s.split_once(['k', 'K']) {
-        let n = num.parse::<usize>()?;
-        Ok(n << 10)
-    } else {
-        let n = s.parse::<usize>()?;
-        Ok(n)
-    }
-}
-
-fn parse_net<'a>(s: &'a str) -> Result<NetParam> {
-    let mut parts = s.trim().splitn(3, ',');
-    let splitter = |s: &'a str| s.split_once::<'a, _>('=');
-    let (tap_path, if_name) = match parts.next().and_then(splitter) {
-        Some(("tap", p)) => (p, None),
-        Some(("if", name)) => ("/dev/net/tun", Some(name.to_owned())),
-        _ => bail!("invalid net opt: {s}"),
-    };
-    let Some(("mac", mac_str)) = parts.next().and_then(splitter) else {
-        bail!("invalid net opt: {s}");
-    };
-    let mut mac = [0u8; 6];
-    let mut fail = false;
-    for (index, p) in mac_str.trim().split(':').enumerate() {
-        if index < 6 {
-            let Ok(b) = u8::from_str_radix(p, 16) else {
-                fail = true;
-                break;
-            };
-            mac[index] = b;
-        } else {
-            fail = true;
-            break;
-        }
-    }
-    if fail {
-        bail!("cannot parse {mac_str}")
-    }
-    let Some(("mtu", mtu_str)) = parts.next().and_then(splitter) else {
-        bail!("invalid net opt: {s}")
-    };
-
-    Ok(NetParam {
-        mac,
-        mtu: mtu_str.parse()?,
-        queue_pairs: 1,
-        tap: tap_path.into(),
-        if_name,
-    })
-}
-
 fn main_run(args: RunArgs) -> Result<()> {
     let hypervisor = Kvm::new()?;
     let payload = if let Some(fw) = args.firmware {
@@ -197,7 +115,7 @@ fn main_run(args: RunArgs) -> Result<()> {
         None
     };
     let board_config = BoardConfig {
-        mem_size: parse_mem(&args.mem_size)?,
+        mem_size: serde_aco::from_arg(&args.mem_size)?,
         num_cpu: args.num_cpu,
     };
     let mut vm = Machine::new(hypervisor, board_config)?;
@@ -209,19 +127,19 @@ fn main_run(args: RunArgs) -> Result<()> {
     if args.pvpanic {
         vm.add_pvpanic()?;
     }
-    let fw_cfg_items = args
-        .fw_cfgs
-        .iter()
-        .map(|s| parse_fw_cfgs(s))
-        .collect::<Result<Vec<_>>>()?;
-    if !fw_cfg_items.is_empty() {
-        vm.add_fw_cfg(fw_cfg_items)?;
+    if !args.fw_cfgs.is_empty() {
+        let params = args
+            .fw_cfgs
+            .iter()
+            .map(|s| serde_aco::from_arg(s))
+            .collect::<Result<Vec<_>, _>>()?;
+        vm.add_fw_cfg(params.into_iter())?;
     }
     if args.entropy {
         vm.add_virtio_dev("virtio-entropy".to_owned(), EntropyParam)?;
     }
     for (index, net_opt) in args.net.into_iter().enumerate() {
-        let net_param = parse_net(&net_opt)?;
+        let net_param: NetParam = serde_aco::from_arg(&net_opt)?;
         vm.add_virtio_dev(format!("virtio-net-{index}"), net_param)?;
     }
     for (index, blk) in args.blk.into_iter().enumerate() {

alioth/Cargo.toml

@@ -18,6 +18,7 @@ rand = "0.8.5"
 libc = "0.2.150"
 parking_lot.workspace = true
 macros = { version = "0.1.0", path = "../macros", package = "alioth-macros" }
+serde.workspace = true
 
 [dev-dependencies]
 assert_matches = "1"

alioth/src/device/fw_cfg/fw_cfg.rs

@@ -12,14 +12,18 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use core::fmt;
 use std::fs::File;
 use std::io::{ErrorKind, Read, Result, Seek, SeekFrom};
 use std::os::unix::fs::FileExt;
+use std::path::PathBuf;
 use std::sync::Arc;
 
 use bitfield::bitfield;
 use macros::Layout;
 use parking_lot::Mutex;
+use serde::de::{self, MapAccess, Visitor};
+use serde::{Deserialize, Deserializer};
 use zerocopy::{AsBytes, FromBytes, FromZeroes};
 
 use crate::firmware::acpi::AcpiTable;
@@ -457,3 +461,94 @@ impl Mmio for Mutex<FwCfg> {
         Ok(())
     }
 }
+
+#[derive(Debug)]
+pub enum FwCfgContentParam {
+    File(PathBuf),
+    String(String),
+}
+
+#[derive(Debug)]
+pub struct FwCfgItemParam {
+    pub name: String,
+    pub content: FwCfgContentParam,
+}
+
+impl<'de> Deserialize<'de> for FwCfgItemParam {
+    fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        #[derive(Deserialize)]
+        #[serde(field_identifier, rename_all = "lowercase")]
+        enum Field {
+            Name,
+            File,
+            String,
+        }
+
+        struct ParamVisitor;
+
+        impl<'de> Visitor<'de> for ParamVisitor {
+            type Value = FwCfgItemParam;
+
+            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+                formatter.write_str("struct FwCfgItemParam")
+            }
+
+            fn visit_map<V>(self, mut map: V) -> std::result::Result<Self::Value, V::Error>
+            where
+                V: MapAccess<'de>,
+            {
+                let mut name = None;
+                let mut content = None;
+                while let Some(key) = map.next_key()? {
+                    match key {
+                        Field::Name => {
+                            if name.is_some() {
+                                return Err(de::Error::duplicate_field("file"));
+                            }
+                            name = Some(map.next_value()?);
+                        }
+                        Field::String => {
+                            if content.is_some() {
+                                return Err(de::Error::duplicate_field("string,file"));
+                            }
+                            content = Some(FwCfgContentParam::String(map.next_value()?));
+                        }
+                        Field::File => {
+                            if content.is_some() {
+                                return Err(de::Error::duplicate_field("string,file"));
+                            }
+                            content = Some(FwCfgContentParam::File(map.next_value()?));
+                        }
+                    }
+                }
+                let name = name.ok_or_else(|| de::Error::missing_field("name"))?;
+                let content = content.ok_or_else(|| de::Error::missing_field("file,string"))?;
+                Ok(FwCfgItemParam { name, content })
+            }
+        }
+
+        const FIELDS: &[&str] = &["name", "file", "string"];
+        deserializer.deserialize_struct("FwCfgItemParam", FIELDS, ParamVisitor)
+    }
+}
+
+impl FwCfgItemParam {
+    pub fn build(self) -> Result<FwCfgItem> {
+        match self.content {
+            FwCfgContentParam::File(file) => {
+                let f = File::open(file)?;
+                Ok(FwCfgItem {
+                    name: self.name,
+                    content: FwCfgContent::File(f),
+                })
+            }
+            FwCfgContentParam::String(string) => Ok(FwCfgItem {
+                name: self.name,
+                content: FwCfgContent::Bytes(string.into()),
+            }),
+        }
+    }
+}

alioth/src/lib.rs

@@ -26,6 +26,8 @@ pub mod hv;
 pub mod loader;
 #[path = "mem/mem.rs"]
 pub mod mem;
+#[path = "net/net.rs"]
+pub mod net;
 #[path = "pci/pci.rs"]
 pub mod pci;
 #[path = "utils/utils.rs"]

alioth/src/net/net.rs

@@ -0,0 +1,90 @@
+// Copyright 2024 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use serde::de::{self, Visitor};
+use serde::Deserialize;
+use zerocopy::{AsBytes, FromBytes, FromZeroes};
+
+#[derive(Debug, Default, FromBytes, FromZeroes, AsBytes, PartialEq, Eq)]
+#[repr(transparent)]
+pub struct MacAddr([u8; 6]);
+
+struct MacAddrVisitor;
+
+impl<'de> Visitor<'de> for MacAddrVisitor {
+    type Value = MacAddr;
+
+    fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+        formatter.write_str("a MAC address like ea:d7:a8:e8:c6:2f")
+    }
+
+    fn visit_borrowed_str<E>(self, v: &'de str) -> Result<Self::Value, E>
+    where
+        E: de::Error,
+    {
+        let mut addr = [0u8; 6];
+        let iter = v.split(':');
+        let mut index = 0;
+        for b_s in iter {
+            let Some(b) = addr.get_mut(index) else {
+                return Err(E::custom("expect 6 bytes"));
+            };
+            let Ok(v) = u8::from_str_radix(b_s, 16) else {
+                return Err(E::custom("expect bytes"));
+            };
+            *b = v;
+            index += 1;
+        }
+        if index != 6 {
+            return Err(E::custom("expect 6 bytes"));
+        }
+        Ok(MacAddr(addr))
+    }
+}
+
+impl<'de> Deserialize<'de> for MacAddr {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: de::Deserializer<'de>,
+    {
+        deserializer.deserialize_str(MacAddrVisitor)
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use serde::de::value::Error;
+    use serde::de::Visitor;
+
+    use crate::net::MacAddr;
+
+    use super::MacAddrVisitor;
+
+    #[test]
+    fn test_mac_addr_visitor() {
+        assert_eq!(
+            MacAddrVisitor.visit_borrowed_str::<Error>("ea:d7:a8:e8:c6:2f"),
+            Ok(MacAddr([0xea, 0xd7, 0xa8, 0xe8, 0xc6, 0x2f]))
+        );
+        assert!(MacAddrVisitor
+            .visit_borrowed_str::<Error>("ea:d7:a8:e8:c6")
+            .is_err());
+        assert!(MacAddrVisitor
+            .visit_borrowed_str::<Error>("ea:d7:a8:e8:c6:ac:ac")
+            .is_err());
+        assert!(MacAddrVisitor
+            .visit_borrowed_str::<Error>("ea:d7:a8:e8:c6:2g")
+            .is_err());
+    }
+}

alioth/src/virtio/dev/net/net.rs

@@ -17,6 +17,7 @@ use std::fs::{self, File};
 use std::io;
 use std::iter::zip;
 use std::mem::MaybeUninit;
+use std::num::NonZeroU16;
 use std::os::fd::AsRawFd;
 use std::os::unix::prelude::OpenOptionsExt;
 use std::path::PathBuf;
@@ -27,10 +28,12 @@ use libc::{IFF_NO_PI, IFF_TAP, IFF_VNET_HDR, O_NONBLOCK};
 use mio::event::Event;
 use mio::unix::SourceFd;
 use mio::{Interest, Registry, Token};
+use serde::Deserialize;
 use zerocopy::{AsBytes, FromBytes, FromZeroes};
 
 use crate::impl_mmio_for_zerocopy;
 use crate::mem::mapped::RamBus;
+use crate::net::MacAddr;
 use crate::virtio::dev::{DevParam, DeviceId, Result, Virtio};
 use crate::virtio::queue::handlers::{queue_to_writer, reader_to_queue};
 use crate::virtio::queue::VirtQueue;
@@ -46,7 +49,7 @@ const QUEUE_TX: u16 = 1;
 #[repr(C, align(8))]
 #[derive(Debug, Default, FromBytes, FromZeroes, AsBytes)]
 pub struct NetConfig {
-    mac: [u8; 6],
+    mac: MacAddr,
     status: u16,
     max_queue_pairs: u16,
     mtu: u16,
@@ -104,10 +107,11 @@ pub struct Net {
     feature: NetFeature,
 }
 
+#[derive(Deserialize)]
 pub struct NetParam {
-    pub mac: [u8; 6],
+    pub mac: MacAddr,
     pub mtu: u16,
-    pub queue_pairs: u16,
+    pub queue_pairs: Option<NonZeroU16>,
     pub tap: PathBuf,
     pub if_name: Option<String>,
 }
@@ -133,7 +137,7 @@ impl Net {
             name,
             config: Arc::new(NetConfig {
                 mac: param.mac,
-                max_queue_pairs: param.queue_pairs,
+                max_queue_pairs: param.queue_pairs.map(|p| p.into()).unwrap_or(1),
                 mtu: param.mtu,
                 ..Default::default()
             }),

alioth/src/vm.rs

@@ -22,7 +22,7 @@ use parking_lot::{Condvar, Mutex, RwLock};
 use thiserror::Error;
 
 use crate::board::{self, ArchBoard, Board, BoardConfig, STATE_CREATED, STATE_RUNNING};
-use crate::device::fw_cfg::{FwCfg, FwCfgItem, PORT_SELECTOR};
+use crate::device::fw_cfg::{FwCfg, FwCfgItemParam, PORT_SELECTOR};
 use crate::device::pvpanic::PvPanic;
 use crate::device::serial::Serial;
 use crate::hv::{self, Hypervisor, Vm};
@@ -133,7 +133,11 @@ where
         self.add_pci_dev(pci_dev)
     }
 
-    pub fn add_fw_cfg(&mut self, items: Vec<FwCfgItem>) -> Result<(), Error> {
+    pub fn add_fw_cfg(
+        &mut self,
+        params: impl Iterator<Item = FwCfgItemParam>,
+    ) -> Result<(), Error> {
+        let items = params.map(|p| p.build()).collect::<Result<Vec<_>, _>>()?;
         let fw_cfg = Arc::new(Mutex::new(FwCfg::new(self.board.memory.ram_bus(), items)?));
         let mut io_devs = self.board.io_devs.write();
         io_devs.push((PORT_SELECTOR, fw_cfg.clone()));