Skip to content

Conversation

Johan-Liebert1
Copy link
Collaborator

No description provided.

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
@bootc-bot bootc-bot bot requested a review from jeckersb September 22, 2025 13:16
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 refactors the EFI variable reading logic into a new utility function read_uefi_var. This function correctly parses the variables as UTF-16 LE, which is a great improvement for correctness. The overall change improves code organization and maintainability. I've added a few suggestions to further improve code clarity and adhere to Rust idioms.

Comment on lines +168 to +169
EfiError::SystemNotUEFI => return Ok(Bootloader::Grub),
EfiError::MissingVar => return Ok(Bootloader::Grub),
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The match arms for EfiError::SystemNotUEFI and EfiError::MissingVar have the same logic. They can be combined into a single arm using the | operator for conciseness and better readability.

Suggested change
EfiError::SystemNotUEFI => return Ok(Bootloader::Grub),
EfiError::MissingVar => return Ok(Bootloader::Grub),
EfiError::SystemNotUEFI | EfiError::MissingVar => return Ok(Bootloader::Grub),

Comment on lines +196 to +199
#[allow(dead_code)]
InvalidData(&'static str),
#[allow(dead_code)]
Io(std::io::Error),
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The #[allow(dead_code)] attributes on the InvalidData and Io variants of EfiError are unnecessary. The InvalidData variant is used in read_uefi_var, and the Io variant is used via the From<std::io::Error> implementation and the ? operator. Removing these attributes will make the code cleaner and avoid suppressing potential future warnings about genuinely unused code.

Suggested change
#[allow(dead_code)]
InvalidData(&'static str),
#[allow(dead_code)]
Io(std::io::Error),
InvalidData(&'static str),
Io(std::io::Error),

Comment on lines +210 to +246
pub fn read_uefi_var(var_name: &str) -> Result<String, EfiError> {
use crate::install::EFIVARFS;
use cap_std_ext::cap_std::ambient_authority;

let efivarfs = match Dir::open_ambient_dir(EFIVARFS, ambient_authority()) {
Ok(dir) => dir,
Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Err(EfiError::SystemNotUEFI),
Err(e) => Err(e)?,
};

match efivarfs.read(var_name) {
Ok(loader_bytes) => {
if loader_bytes.len() % 2 != 0 {
return Err(EfiError::InvalidData(
"EFI var length is not valid UTF-16 LE",
));
}

// EFI vars are UTF-16 LE
let loader_u16_bytes: Vec<u16> = loader_bytes
.chunks_exact(2)
.map(|x| u16::from_le_bytes([x[0], x[1]]))
.collect();

let loader = String::from_utf16(&loader_u16_bytes)
.map_err(|_| EfiError::InvalidData("EFI var is not UTF-16"))?;

return Ok(loader);
}

Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
return Err(EfiError::MissingVar);
}

Err(e) => Err(e)?,
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The read_uefi_var function can be refactored to be more idiomatic and readable by using the ? operator for error handling and avoiding multiple match statements with early returns. This improves maintainability.

pub fn read_uefi_var(var_name: &str) -> Result<String, EfiError> {
    use crate::install::EFIVARFS;
    use cap_std_ext::cap_std::ambient_authority;

    let efivarfs = Dir::open_ambient_dir(EFIVARFS, ambient_authority()).map_err(|e| {
        if e.kind() == std::io::ErrorKind::NotFound {
            EfiError::SystemNotUEFI
        } else {
            e.into()
        }
    })?;

    let loader_bytes = efivarfs.read(var_name).map_err(|e| {
        if e.kind() == std::io::ErrorKind::NotFound {
            EfiError::MissingVar
        } else {
            e.into()
        }
    })?;

    if loader_bytes.len() % 2 != 0 {
        return Err(EfiError::InvalidData(
            "EFI var length is not valid UTF-16 LE",
        ));
    }

    // EFI vars are UTF-16 LE
    let loader_u16_bytes: Vec<u16> = loader_bytes
        .chunks_exact(2)
        .map(|x| u16::from_le_bytes([x[0], x[1]]))
        .collect();

    String::from_utf16(&loader_u16_bytes)
        .map_err(|_| EfiError::InvalidData("EFI var is not UTF-16"))
}

return Ok(loader);
}

Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did coreos/cap-std-ext#78 motivated by this btw

@cgwalters cgwalters merged commit 2898860 into bootc-dev:main Sep 22, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants