More kargs improvements and hoisting parsing into clap

crates/kernel_cmdline/Cargo.toml

@@ -9,6 +9,7 @@ repository = "https://github.com/bootc-dev/bootc"
 [dependencies]
 # Workspace dependencies
 anyhow = { workspace = true }
+serde = { workspace = true, features = ["derive"] }
 
 [dev-dependencies]
 similar-asserts = { workspace = true }

crates/kernel_cmdline/src/bytes.rs

@@ -4,18 +4,20 @@
 //! arguments, supporting both key-only switches and key-value pairs with proper quote handling.
 
 use std::borrow::Cow;
+use std::cmp::Ordering;
 use std::ops::Deref;
 
 use crate::{utf8, Action};
 
 use anyhow::Result;
+use serde::{Deserialize, Serialize};
 
 /// A parsed kernel command line.
 ///
 /// Wraps the raw command line bytes and provides methods for parsing and iterating
 /// over individual parameters. Uses copy-on-write semantics to avoid unnecessary
 /// allocations when working with borrowed data.
-#[derive(Clone, Debug, Default)]
+#[derive(Clone, Debug, Default, Serialize, Deserialize)]
 pub struct Cmdline<'a>(Cow<'a, [u8]>);
 
 /// An owned Cmdline.  Alias for `Cmdline<'static>`.
@@ -38,6 +40,16 @@ impl Deref for Cmdline<'_> {
     }
 }
 
+impl<'a, T> AsRef<T> for Cmdline<'a>
+where
+    T: ?Sized,
+    <Cmdline<'a> as Deref>::Target: AsRef<T>,
+{
+    fn as_ref(&self) -> &T {
+        self.deref().as_ref()
+    }
+}
+
 impl From<Vec<u8>> for CmdlineOwned {
     /// Creates a new `Cmdline` from an owned `Vec<u8>`.
     fn from(input: Vec<u8>) -> Self {
@@ -339,12 +351,6 @@ impl<'a> Cmdline<'a> {
     }
 }
 
-impl<'a> AsRef<[u8]> for Cmdline<'a> {
-    fn as_ref(&self) -> &[u8] {
-        &self.0
-    }
-}
-
 impl<'a> IntoIterator for &'a Cmdline<'a> {
     type Item = Parameter<'a>;
     type IntoIter = CmdlineIter<'a>;
@@ -369,16 +375,29 @@ impl<'a, 'other> Extend<Parameter<'other>> for Cmdline<'a> {
     }
 }
 
+impl PartialEq for Cmdline<'_> {
+    fn eq(&self, other: &Self) -> bool {
+        let mut our_params = self.iter().collect::<Vec<_>>();
+        our_params.sort();
+        let mut their_params = other.iter().collect::<Vec<_>>();
+        their_params.sort();
+
+        our_params == their_params
+    }
+}
+
+impl Eq for Cmdline<'_> {}
+
 /// A single kernel command line parameter key
 ///
 /// Handles quoted values and treats dashes and underscores in keys as equivalent.
-#[derive(Clone, Debug, Eq)]
+#[derive(Clone, Debug)]
 pub struct ParameterKey<'a>(pub(crate) &'a [u8]);
 
-impl<'a> Deref for ParameterKey<'a> {
+impl Deref for ParameterKey<'_> {
     type Target = [u8];
 
-    fn deref(&self) -> &'a Self::Target {
+    fn deref(&self) -> &Self::Target {
         self.0
     }
 }
@@ -399,32 +418,42 @@ impl<'a, T: AsRef<[u8]> + ?Sized> From<&'a T> for ParameterKey<'a> {
     }
 }
 
+impl ParameterKey<'_> {
+    /// Returns an iterator over the canonicalized bytes of the
+    /// parameter, with dashes turned into underscores.
+    fn iter(&self) -> impl Iterator<Item = u8> + use<'_> {
+        self.0
+            .iter()
+            .map(|&c: &u8| if c == b'-' { b'_' } else { c })
+    }
+}
+
 impl PartialEq for ParameterKey<'_> {
     /// Compares two parameter keys for equality.
     ///
     /// Keys are compared with dashes and underscores treated as equivalent.
     /// This comparison is case-sensitive.
     fn eq(&self, other: &Self) -> bool {
-        let dedashed = |&c: &u8| {
-            if c == b'-' {
-                b'_'
-            } else {
-                c
-            }
-        };
+        self.iter().eq(other.iter())
+    }
+}
 
-        // We can't just zip() because leading substrings will match
-        //
-        // For example, "foo" == "foobar" since the zipped iterator
-        // only compares the first three chars.
-        let our_iter = self.0.iter().map(dedashed);
-        let other_iter = other.0.iter().map(dedashed);
-        our_iter.eq(other_iter)
+impl Eq for ParameterKey<'_> {}
+
+impl Ord for ParameterKey<'_> {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.iter().cmp(other.iter())
+    }
+}
+
+impl PartialOrd for ParameterKey<'_> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
     }
 }
 
 /// A single kernel command line parameter.
-#[derive(Clone, Debug, Eq)]
+#[derive(Clone, Debug)]
 pub struct Parameter<'a> {
     /// The full original value
     parameter: &'a [u8],
@@ -501,21 +530,45 @@ impl<'a> Parameter<'a> {
     }
 }
 
-impl<'a> PartialEq for Parameter<'a> {
+impl PartialEq for Parameter<'_> {
     fn eq(&self, other: &Self) -> bool {
         // Note we don't compare parameter because we want hyphen-dash insensitivity for the key
         self.key == other.key && self.value == other.value
     }
 }
 
-impl<'a> std::ops::Deref for Parameter<'a> {
+impl Eq for Parameter<'_> {}
+
+impl Ord for Parameter<'_> {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.key.cmp(&other.key).then(self.value.cmp(&other.value))
+    }
+}
+
+impl PartialOrd for Parameter<'_> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+impl Deref for Parameter<'_> {
     type Target = [u8];
 
-    fn deref(&self) -> &'a Self::Target {
+    fn deref(&self) -> &Self::Target {
         self.parameter
     }
 }
 
+impl<'a, T> AsRef<T> for Parameter<'a>
+where
+    T: ?Sized,
+    <Parameter<'a> as Deref>::Target: AsRef<T>,
+{
+    fn as_ref(&self) -> &T {
+        self.deref().as_ref()
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -1051,4 +1104,24 @@ mod tests {
 
         assert_eq!(params.len(), 0);
     }
+
+    #[test]
+    fn test_cmdline_eq() {
+        // Ordering, quoting, and the whole dash-underscore
+        // equivalence thing shouldn't affect whether these are
+        // semantically equal
+        assert_eq!(
+            Cmdline::from("foo bar-with-delim=\"with spaces\""),
+            Cmdline::from("\"bar_with_delim=with spaces\" foo")
+        );
+
+        // Uneven lengths are not equal even if the parameters are. Or
+        // to put it another way, duplicate parameters break equality.
+        // Check with both orderings.
+        assert_ne!(Cmdline::from("foo"), Cmdline::from("foo foo"));
+        assert_ne!(Cmdline::from("foo foo"), Cmdline::from("foo"));
+
+        // Equal lengths but differing duplicates are also not equal
+        assert_ne!(Cmdline::from("a a b"), Cmdline::from("a b b"));
+    }
 }

crates/kernel_cmdline/src/utf8.rs

@@ -8,13 +8,14 @@ use std::ops::Deref;
 use crate::{bytes, Action};
 
 use anyhow::Result;
+use serde::{Deserialize, Serialize};
 
 /// A parsed UTF-8 kernel command line.
 ///
 /// Wraps the raw command line bytes and provides methods for parsing and iterating
 /// over individual parameters. Uses copy-on-write semantics to avoid unnecessary
 /// allocations when working with borrowed data.
-#[derive(Clone, Debug, Default)]
+#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)]
 pub struct Cmdline<'a>(bytes::Cmdline<'a>);
 
 /// An owned `Cmdline`.  Alias for `Cmdline<'static>`.
@@ -223,10 +224,13 @@ impl Deref for Cmdline<'_> {
     }
 }
 
-impl<'a> AsRef<str> for Cmdline<'a> {
-    fn as_ref(&self) -> &str {
-        str::from_utf8(self.0.as_ref())
-            .expect("We only construct the underlying bytes from valid UTF-8")
+impl<'a, T> AsRef<T> for Cmdline<'a>
+where
+    T: ?Sized,
+    <Cmdline<'a> as Deref>::Target: AsRef<T>,
+{
+    fn as_ref(&self) -> &T {
+        self.deref().as_ref()
     }
 }
 
@@ -266,7 +270,7 @@ impl<'a, 'other> Extend<Parameter<'other>> for Cmdline<'a> {
 #[derive(Clone, Debug, Eq)]
 pub struct ParameterKey<'a>(bytes::ParameterKey<'a>);
 
-impl<'a> Deref for ParameterKey<'a> {
+impl Deref for ParameterKey<'_> {
     type Target = str;
 
     fn deref(&self) -> &Self::Target {
@@ -380,7 +384,7 @@ impl<'a> std::fmt::Display for Parameter<'a> {
     }
 }
 
-impl<'a> Deref for Parameter<'a> {
+impl Deref for Parameter<'_> {
     type Target = str;
 
     fn deref(&self) -> &Self::Target {
@@ -390,6 +394,16 @@ impl<'a> Deref for Parameter<'a> {
     }
 }
 
+impl<'a, T> AsRef<T> for Parameter<'a>
+where
+    T: ?Sized,
+    <Parameter<'a> as Deref>::Target: AsRef<T>,
+{
+    fn as_ref(&self) -> &T {
+        self.deref().as_ref()
+    }
+}
+
 impl<'a> PartialEq for Parameter<'a> {
     fn eq(&self, other: &Self) -> bool {
         self.0 == other.0
@@ -768,7 +782,7 @@ mod tests {
     fn test_add_empty_cmdline() {
         let mut kargs = Cmdline::from("");
         assert!(matches!(kargs.add(&param("foo")), Action::Added));
-        assert_eq!(kargs.as_ref(), "foo");
+        assert_eq!(&*kargs, "foo");
     }
 
     #[test]
@@ -808,7 +822,7 @@ mod tests {
     fn test_add_or_modify_empty_cmdline() {
         let mut kargs = Cmdline::from("");
         assert!(matches!(kargs.add_or_modify(&param("foo")), Action::Added));
-        assert_eq!(kargs.as_ref(), "foo");
+        assert_eq!(&*kargs, "foo");
     }
 
     #[test]
@@ -914,4 +928,24 @@ mod tests {
         assert_eq!(params[1], param("baz=qux"));
         assert_eq!(params[2], param("wiz"));
     }
+
+    #[test]
+    fn test_cmdline_eq() {
+        // Ordering, quoting, and the whole dash-underscore
+        // equivalence thing shouldn't affect whether these are
+        // semantically equal
+        assert_eq!(
+            Cmdline::from("foo bar-with-delim=\"with spaces\""),
+            Cmdline::from("\"bar_with_delim=with spaces\" foo")
+        );
+
+        // Uneven lengths are not equal even if the parameters are. Or
+        // to put it another way, duplicate parameters break equality.
+        // Check with both orderings.
+        assert_ne!(Cmdline::from("foo"), Cmdline::from("foo foo"));
+        assert_ne!(Cmdline::from("foo foo"), Cmdline::from("foo"));
+
+        // Equal lengths but differing duplicates are also not equal
+        assert_ne!(Cmdline::from("a a b"), Cmdline::from("a b b"));
+    }
 }

crates/lib/src/bootc_composefs/boot.rs

@@ -5,6 +5,7 @@ use std::path::Path;
 
 use anyhow::{anyhow, Context, Result};
 use bootc_blockdev::find_parent_devices;
+use bootc_kernel_cmdline::utf8::Cmdline;
 use bootc_mount::inspect_filesystem_of_dir;
 use bootc_mount::tempmount::TempMount;
 use camino::{Utf8Path, Utf8PathBuf};
@@ -380,13 +381,17 @@ pub(crate) fn setup_composefs_bls_boot(
     let (root_path, esp_device, cmdline_refs, fs, bootloader) = match setup_type {
         BootSetupType::Setup((root_setup, state, fs)) => {
             // root_setup.kargs has [root=UUID=<UUID>, "rw"]
-            let mut cmdline_options = String::from(root_setup.kargs.join(" "));
+            let mut cmdline_options = Cmdline::new();
 
-            if state.composefs_options.insecure {
-                cmdline_options.push_str(&format!(" {COMPOSEFS_CMDLINE}=?{id_hex}"));
+            cmdline_options.extend(&root_setup.kargs);
+
+            let composefs_cmdline = if state.composefs_options.insecure {
+                format!("{COMPOSEFS_CMDLINE}=?{id_hex}")
             } else {
-                cmdline_options.push_str(&format!(" {COMPOSEFS_CMDLINE}={id_hex}"));
-            }
+                format!("{COMPOSEFS_CMDLINE}={id_hex}")
+            };
+
+            cmdline_options.extend(&Cmdline::from(&composefs_cmdline));
 
             // Locate ESP partition device
             let esp_part = esp_in(&root_setup.device_info)?;
@@ -407,12 +412,10 @@ pub(crate) fn setup_composefs_bls_boot(
             (
                 Utf8PathBuf::from("/sysroot"),
                 get_esp_partition(&sysroot_parent)?.0,
-                [
-                    format!("root=UUID={}", this_arch_root()),
-                    RW_KARG.to_string(),
-                    format!("{COMPOSEFS_CMDLINE}={id_hex}"),
-                ]
-                .join(" "),
+                Cmdline::from(format!(
+                    "root=UUID={} {RW_KARG} {COMPOSEFS_CMDLINE}={id_hex}",
+                    this_arch_root()
+                )),
                 fs,
                 bootloader,
             )