More kargs improvements and hoisting parsing into clap #1761
Conversation
Prep so we can parse these directly via clap Signed-off-by: John Eckersberg <[email protected]>
Signed-off-by: John Eckersberg <[email protected]>
Implement generic AsRef in terms of Deref as suggested in: https://doc.rust-lang.org/std/convert/trait.AsRef.html#generic-implementations I'm not 100% sold on this since in some case (like the change to the tests here) you have to end up doing `&*` coercion to get the right type, but this at least makes everything consistent. Would be a bit nicer when/if str_as_str[1] stabilizes, at least for the most common UTF-8 case. [1] rust-lang/rust#130366 Signed-off-by: John Eckersberg <[email protected]>
|
|
||
| use crate::composefs_consts::COMPOSEFS_CMDLINE; | ||
|
|
||
| #[derive(Debug, PartialEq, PartialOrd, Eq, Default)] |
There was a problem hiding this comment.
I want to specifically call out that I removed this PartialOrd derive, I'm not sure why it was there and I don't think it makes sense to apply an ordering to this type. But it was blocking me from switching to Cmdline here since Cmdline does not impl PartialOrd. I could add it but I don't think it makes sense.
There was a problem hiding this comment.
Actually maybe it would make sense. I could see adding a canonicalize() method to Cmdline which would sort the params and render them out with some opinionated quoting 🤔
Edit: Or at least make sense for Cmdline... still not sure it makes sense here for BLSConfigType but if I end up adding it for Cmdline I'll just put it back for now.
There was a problem hiding this comment.
Well down below I added PartialOrd/Ord for Parameter which makes sense in the context of comparing two Cmdlines by sorting their Parameters. But I still don't think it makes sense for Cmdline itself to have ordering.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a nice improvement, refactoring kernel argument handling to use the Cmdline type throughout the codebase. This centralizes parsing logic and makes the code more robust and readable. The changes to use CmdlineOwned in clap structures and standardizing Deref/AsRef implementations are well-executed. I have one suggestion to improve consistency and efficiency in how kernel arguments from the install configuration are processed.
Plus all of the various places this trickles down. Signed-off-by: John Eckersberg <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, refactoring kernel argument handling to use a dedicated Cmdline type. This significantly enhances type safety and maintainability by moving away from string manipulation. The changes are well-applied across the codebase. I've found one issue in the PartialEq implementation for Cmdline that doesn't correctly handle duplicate parameters, and I've provided a suggestion to fix it. The rest of the changes look solid.
…terKey We need to be able to check equality on Cmdline by comparing sorted parameters. Signed-off-by: John Eckersberg <[email protected]>
…dline Signed-off-by: John Eckersberg <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves kernel argument handling by introducing and consistently using the Cmdline type from the bootc-kernel-cmdline crate. Hoisting the parsing logic into clap and using the Cmdline type throughout the installation process makes the code cleaner and more robust.
I've found a couple of critical issues that will prevent the code from compiling, which I've detailed in the comments. Once these are addressed, this will be a great improvement.
| Cmdline::from(format!( | ||
| "root=UUID={} {RW_KARG} {COMPOSEFS_CMDLINE}={id_hex}", | ||
| this_arch_root() | ||
| )), |
There was a problem hiding this comment.
I think it'd be cleaner for this to remain split up kargs instead of being parsed from a string again.
There was a problem hiding this comment.
It's all just Vec<u8> under the covers, it doesn't really get parsed again. The only "parsing" is when it hands out slices via the various iter methods.
2 participants
Uh oh!
There was an error while loading. Please reload this page.