-
Notifications
You must be signed in to change notification settings - Fork 149
kernel_cmdline: More prep work for bootc_kargs integration #1729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
No reason not to, generally useful. Signed-off-by: John Eckersberg <[email protected]>
This creates an empty, owned ('static) Cmdline.
Signed-off-by: John Eckersberg <[email protected]>
641fcc1 to
67ba584
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does a nice job of refactoring the kernel command line parsing logic. The introduction of iter_bytes and iter_str provides a cleaner separation of concerns and a more flexible API. The consolidation of parsing logic into CmdlineIterBytes is a good move. I've suggested one simplification to the Parameter::parse function to make it more direct and efficient. Overall, these are solid improvements.
67ba584 to
6cff89f
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a nice refactoring of the kernel command line parsing logic. The separation of concerns between splitting the command line into parameters and parsing individual parameters is a great improvement for code clarity and maintainability. The addition of iter_bytes and iter_str provides useful flexibility. The new Cmdline::new() constructors and the addition of Clone are also welcome improvements. The changes are well-tested.
I have one minor suggestion to improve the clarity of an internal function's signature, but overall this is a solid set of changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a solid refactoring of the kernel command line parsing logic. The introduction of iter_bytes and iter_str cleanly separates the logic of splitting parameters from parsing them. The addition of Cmdline::new() and deriving Clone for Cmdline are also good improvements. The test cases are well-updated to reflect these changes. I have a couple of minor suggestions to improve documentation and comments for better clarity and to prevent potential misuse.
This commit refactors the kernel_cmdline parsing implementation to reduce code duplication and provide a clearer separation of concerns. Key changes: 1. Introduced CmdlineIterBytes as a dedicated iterator for splitting the command line into raw parameter byte slices. This centralizes the quote-aware whitespace splitting logic in one place. 2. Refactored CmdlineIter to wrap CmdlineIterBytes instead of duplicating the splitting logic. This eliminates redundant code and ensures consistent behavior. 3. Consolidated Parameter::parse and Parameter::parse_one into a single parse() method. Parameter::parse now uses CmdlineIterBytes directly to find the first parameter, then passes it to parse_internal() which assumes the input is already a single parameter. 4. Added iter_bytes() and iter_str() methods to Cmdline for cases where users need raw byte slices or strings without full Parameter parsing overhead. 5. Removed utf8::Parameter::parse_one() in favor of a simplified parse() that wraps the bytes implementation. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: John Eckersberg <[email protected]>
6cff89f to
c6df808
Compare
First two commits are trivial, the last one is rather involved but the commit message explains what's going on. Ideally I'd like to break that up a bit but it was getting awkward trying to get the sequencing of diffs right.