Kargs followup

[cgwalters]

No description provided.

[gemini-code-assist]

Code Review

This pull request refactors kernel argument parsing by introducing a ParameterKey type to handle key comparisons correctly and adds a convenient find method on Cmdline. The changes are well-structured and improve the clarity of the code. I've pointed out a couple of documentation and comment issues in kernel.rs that should be addressed to avoid confusion.

[jeckersb]

Maybe instead of the .0 thing leaking out here, we add starts_with to ParameterKey?

[jeckersb]

diff --git i/crates/lib/src/install.rs w/crates/lib/src/install.rs
index 18a6d6fa..3775ce8d 100644
--- i/crates/lib/src/install.rs
+++ w/crates/lib/src/install.rs
@@ -1669,7 +1669,7 @@ fn find_root_args_to_inherit(cmdline: &Cmdline, root_info: &Filesystem) -> Resul
         let rootflags = cmdline.find(crate::kernel::ROOTFLAGS);
         let inherit_kargs = cmdline
             .iter()
-            .filter(|arg| arg.key.0.starts_with(crate::kernel::INITRD_ARG_PREFIX));
+            .filter(|arg| arg.key.starts_with(crate::kernel::INITRD_ARG_PREFIX));
         (
             root.value_lossy(),
             rootflags
diff --git i/crates/lib/src/kernel.rs w/crates/lib/src/kernel.rs
index 04ae5fc0..b6bfa7ae 100644
--- i/crates/lib/src/kernel.rs
+++ w/crates/lib/src/kernel.rs
@@ -68,7 +68,13 @@ impl<'a> Cmdline<'a> {
 ///
 /// Handles quoted values and treats dashes and underscores in keys as equivalent.
 #[derive(Debug, Eq)]
-pub(crate) struct ParameterKey<'a>(pub &'a [u8]);
+pub(crate) struct ParameterKey<'a>(&'a [u8]);
+
+impl<'a> ParameterKey<'a> {
+    pub fn starts_with(&'a self, key: impl AsRef<[u8]>) -> bool {
+        self.0.starts_with(key.as_ref())
+    }
+}

 impl<'a> From<&'a [u8]> for ParameterKey<'a> {
     fn from(value: &'a [u8]) -> Self {

[jeckersb]

... or maybe ParameterKey just Derefs to the inner slice so it's mostly transparent? Might get conflicting or ambiguous implementations of PartialEq though? I'll see if that remotely works at all.

[jeckersb]

Yeah that seems to work, and I think it makes sense to impl Defer for ParameterKey:

https://doc.rust-lang.org/std/ops/trait.Deref.html#when-to-implement-deref-or-derefmut

In general, deref traits should be implemented if:

1. a value of the type transparently behaves like a value of the target type;
2. the implementation of the deref function is cheap; and
3. users of the type will not be surprised by any deref coercion behavior.

... which instead gives something like:

diff --git i/crates/lib/src/install.rs w/crates/lib/src/install.rs
index 18a6d6fa..3775ce8d 100644
--- i/crates/lib/src/install.rs
+++ w/crates/lib/src/install.rs
@@ -1669,7 +1669,7 @@ fn find_root_args_to_inherit(cmdline: &Cmdline, root_info: &Filesystem) -> Resul
         let rootflags = cmdline.find(crate::kernel::ROOTFLAGS);
         let inherit_kargs = cmdline
             .iter()
-            .filter(|arg| arg.key.0.starts_with(crate::kernel::INITRD_ARG_PREFIX));
+            .filter(|arg| arg.key.starts_with(crate::kernel::INITRD_ARG_PREFIX));
         (
             root.value_lossy(),
             rootflags
diff --git i/crates/lib/src/kernel.rs w/crates/lib/src/kernel.rs
index 04ae5fc0..445e5775 100644
--- i/crates/lib/src/kernel.rs
+++ w/crates/lib/src/kernel.rs
@@ -68,7 +68,15 @@ impl<'a> Cmdline<'a> {
 ///
 /// Handles quoted values and treats dashes and underscores in keys as equivalent.
 #[derive(Debug, Eq)]
-pub(crate) struct ParameterKey<'a>(pub &'a [u8]);
+pub(crate) struct ParameterKey<'a>(&'a [u8]);
+
+impl<'a> std::ops::Deref for ParameterKey<'a> {
+    type Target = [u8];
+
+    fn deref(&self) -> &'a Self::Target {
+        self.0
+    }
+}

 impl<'a> From<&'a [u8]> for ParameterKey<'a> {
     fn from(value: &'a [u8]) -> Self {
@@ -108,7 +116,7 @@ impl<'a> Parameter<'a> {
     ///
     /// Invalid UTF-8 sequences are replaced with the Unicode replacement character.
     pub fn key_lossy(&self) -> String {
-        String::from_utf8_lossy(self.key.0).to_string()
+        String::from_utf8_lossy(&self.key).to_string()
     }

     /// Returns the value as a lossy UTF-8 string.

[cgwalters]

Maybe instead of the .0 thing leaking out here, we add starts_with to ParameterKey?

We could also add cmdline.find_starting_with as high level sugar.

Yeah that seems to work, and I think it makes sense to impl Defer for ParameterKey:

The rationale for not implementing that is it makes == theoretically ambiguous right?

But I don't have a strong opinion if you prefer the Deref. From my PoV feel free to force push to this PR too

[jeckersb]

Yeah that seems to work, and I think it makes sense to impl Defer for ParameterKey:

The rationale for not implementing that is it makes == theoretically ambiguous right?

Not so much ambiguous (there's clearly defined behavior in the reference), but slightly non-obvious. If it's truly ambiguous the compiler will straight up yell at you 😄

But I don't have a strong opinion if you prefer the Deref. From my PoV feel free to force push to this PR too

I think the Deref makes sense in this context so I slightly favor it. I'll update the PR to use it.

[jeckersb]

I guess I should push the approve button, too 😄