kernel_cmdline: Refactor parsing to improve quote and whitespace handling

[jeckersb]

This moves the bulk of the parsing logic into Parameter. Its
parse method returns a 2-tuple, its items being:

1. An Option<Parameter> which contains the parsed Parameter.
   This will be None if the provided input is empty or contains only
   whitespace.

2. A byte slice of the rest of the input which was not consumed by
   the yielded Parameter.

This also adds new tests for some cases that would fail under the
previous parser implementation.

Signed-off-by: John Eckersberg jeckersb@redhat.com

[gemini-code-assist]

Code Review

This pull request is a great improvement, transitioning from infallible From traits to fallible TryFrom for more robust kernel command line parameter parsing. The added validation for keys and improved whitespace handling are valuable additions. I've found a critical issue where invalid input can cause a panic, a high-severity bug in ParameterStr parsing, and a minor opportunity for code simplification. Overall, these changes significantly enhance the reliability of the parsing logic.

[cgwalters]

Looks OK to me; I think it's notable that this doesn't change any external users though so it means we're not fixing any bugs. That's fine of course, just an internal cleanup.

[jeckersb]

Looks OK to me; I think it's notable that this doesn't change any external users though so it means we're not fixing any bugs. That's fine of course, just an internal cleanup.

Primary motivation for this is that I wanted to use Parameter in composefs-boot, and I realized you could feed it anything which made me sad

[cgwalters]

I think it might improve ergonomics to define this in terms of a function that returns Option, and have this one just use .ok_or_else.

The reason here is that the one that returns Option can be const and so since Rust 1.83 it should be possible to do e.g.:

let parameter = const { Parameter::from_bytes("foo").unwrap() }

So for constant strings like this if it's invalid we fail at compile time, not runtime.

[jeckersb]

Bah it occurs to me that most of my premise here is just wrong. Going back to #1425 (comment), you can in fact quote the key and have internal whitespace. Dumb, but valid.

[jeckersb]

I think maybe the better way to approach this is to move the parameter splitting out of Cmdline and into Parameter/ParameterStr themselves. Then initializing a new Parameter from a slice of bytes (or &str or whatnot) will consume as much of the slice as necessary to emit itself, and also hand back everything else that it didn't consume. I think everything except empty/whitespace should parse, so you'd end up with something like this under impl Parameter:

fn parse(input: &'a [u8]) -> (Option<Self>, &'a [u8])

Then Cmdline can just emit items while parsing parameters is Some.

This would also make the semantics in external use case of Parameter in places like composefs-boot more obvious.

[jeckersb]

Another thing is probably to not implement From and/or TryFrom for ParameterKey/ParameterKeyStr so they can't be constructed out of context from their parent Parameter/ParameterStr respectively.

[cgwalters]

Primary motivation for this is that I wanted to use Parameter in composefs-boot, and I realized you could feed it anything which made me sad

Isn't there a more fundamental problem in that composefs-boot still has its own karg parsing distinct from this crate, so we either need to lower this to that (or I'm still tempted to just move composefs-boot over here)

[jeckersb]

Primary motivation for this is that I wanted to use Parameter in composefs-boot, and I realized you could feed it anything which made me sad

Isn't there a more fundamental problem in that composefs-boot still has its own karg parsing distinct from this crate, so we either need to lower this to that (or I'm still tempted to just move composefs-boot over here)

Yeah I was/am in the process of removing all of the kargs parsing bits over there and having it use kernel_cmdline from here instead where needed.

[jeckersb]

Ok pretty much completely redid this, I think this is better but I gotta stop messing with it for now.

/gemini review

[gemini-code-assist]

Code Review

This pull request is a solid refactoring of the kernel command line parsing logic. Moving the parsing into Parameter::parse and introducing CmdlineIter significantly improves the code's clarity, maintainability, and correctness, especially around whitespace and quote handling. The new tests are a great addition and cover important edge cases. I found one minor issue in a test case, which I've detailed below.