Skip to content

kernel_cmdline: Simplify Display impl for utf8::Parameter #1749

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 12, 2025
+35 −16
Merged

kernel_cmdline: Simplify Display impl for utf8::Parameter #1749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2006c10
kernel_cmdline: Implement `Deref` for `Cmdline`
jeckersb Nov 10, 2025
fcc9258
kernel_cmdline: Simplify Display impl for utf8::Parameter
jeckersb Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/kernel_cmdline/src/bytes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ impl<'a, T: AsRef<[u8]> + ?Sized> From<&'a T> for Cmdline<'a> {
}
}

impl Deref for Cmdline<'_> {
type Target = [u8];

fn deref(&self) -> &Self::Target {
&self.0
}
}

impl From<Vec<u8>> for CmdlineOwned {
/// Creates a new `Cmdline` from an owned `Vec<u8>`.
fn from(input: Vec<u8>) -> Self {
Expand Down
43 changes: 27 additions & 16 deletions crates/kernel_cmdline/src/utf8.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,16 @@ impl<'a> Cmdline<'a> {
}
}

impl Deref for Cmdline<'_> {
type Target = str;

fn deref(&self) -> &Self::Target {
// SAFETY: We know this is valid UTF-8 since we only
// construct the underlying `bytes` from valid UTF-8
str::from_utf8(&self.0).expect("We only construct the underlying bytes from valid UTF-8")
Comment on lines +220 to +222
Copy link
Contributor

@bshephar bshephar Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from_utf8 is a safe operation right? Isn't it from_utf8_unchecked that is the unsafe method?
Safe:
https://doc.rust-lang.org/std/primitive.str.html#method.from_utf8

If you are sure that the byte slice is valid UTF-8, and you don’t want to incur the overhead of the validity check, there is an unsafe version of this function, from_utf8_unchecked, which has the same behavior but skips the check.

Unsafe:
https://doc.rust-lang.org/std/str/fn.from_utf8_unchecked.html

Just wondering if the safety comment is necessary here. Seems like it would only be necessary if using the unchecked version.

Copy link
Collaborator Author

@jeckersb jeckersb Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's safe and probably not correct to the letter of the law of rust conventions to label it like that. It's like that because in the very earliest version of this code, it did actually use from_utf8_unchecked. But really it's not so performance critical that we need to squeeze out a few cycles and avoid the safe code. So we switched it to do the checked version and take the performance "penalty". In theory however, those comments still hold true and we should never hit the case where the underlying bytes are invalid UTF-8 and if we really really wanted to we could switch back to using the unchecked version.

Or put another way... it should be 100% impossible for this to panic. But if something does goes wrong, at least it will panic instead of undefined behavior.

}
}

impl<'a> AsRef<str> for Cmdline<'a> {
fn as_ref(&self) -> &str {
str::from_utf8(self.0.as_ref())
Expand All @@ -222,8 +232,7 @@ impl<'a> AsRef<str> for Cmdline<'a> {

impl<'a> std::fmt::Display for Cmdline<'a> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
let as_str: &str = self.as_ref();
write!(f, "{as_str}")
f.write_str(self)
}
}

Expand Down Expand Up @@ -257,7 +266,7 @@ impl<'a, 'other> Extend<Parameter<'other>> for Cmdline<'a> {
#[derive(Clone, Debug, Eq)]
pub struct ParameterKey<'a>(bytes::ParameterKey<'a>);

impl<'a> std::ops::Deref for ParameterKey<'a> {
impl<'a> Deref for ParameterKey<'a> {
type Target = str;

fn deref(&self) -> &Self::Target {
Expand Down Expand Up @@ -295,8 +304,7 @@ impl<'a, T: AsRef<str> + ?Sized> From<&'a T> for ParameterKey<'a> {

impl<'a> std::fmt::Display for ParameterKey<'a> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
let as_str: &str = self;
write!(f, "{as_str}")
f.write_str(self)
}
}

Expand Down Expand Up @@ -368,20 +376,11 @@ impl<'a> TryFrom<bytes::Parameter<'a>> for Parameter<'a> {

impl<'a> std::fmt::Display for Parameter<'a> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
match self.value() {
Some(v) => {
if v.contains(|ch: char| ch.is_ascii_whitespace()) {
write!(f, "{}=\"{}\"", self.key(), v)
} else {
write!(f, "{}={}", self.key(), v)
}
}
None => write!(f, "{}", self.key()),
}
f.write_str(self)
}
}

impl<'a> std::ops::Deref for Parameter<'a> {
impl<'a> Deref for Parameter<'a> {
type Target = str;

fn deref(&self) -> &Self::Target {
Expand Down Expand Up @@ -449,6 +448,18 @@ mod tests {
assert_eq!(outside_quoted, value_quoted);
}

#[test]
fn test_parameter_display() {
// Basically this should always return the original data
// without modification.

// unquoted stays unquoted
assert_eq!(param("foo").to_string(), "foo");

// quoted stays quoted
assert_eq!(param("\"foo\"").to_string(), "\"foo\"");
}

#[test]
fn test_parameter_extra_whitespace() {
let p = param(" foo=bar ");
Expand Down