Skip to content

Enhancement/efi shell interface #1679

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
160 changes: 158 additions & 2 deletions uefi-test-runner/src/proto/shell.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,169 @@
// SPDX-License-Identifier: MIT OR Apache-2.0

use uefi::boot;
use uefi::boot::ScopedProtocol;
use uefi::proto::shell::Shell;
use uefi::{boot, cstr16};
use uefi_raw::Status;

/// Test `get_env()`, `get_envs()`, and `set_env()`
pub fn test_env(shell: &ScopedProtocol<Shell>) {
/* Test retrieving list of environment variable names */
let mut cur_env_vec = shell.get_envs();
assert_eq!(cur_env_vec.next().unwrap(), cstr16!("path"),);
// check pre-defined shell variables; see UEFI Shell spec
assert_eq!(cur_env_vec.next().unwrap(), cstr16!("nonesting"),);
let cur_env_vec = shell.get_envs();
let default_len = cur_env_vec.count();

/* Test setting and getting a specific environment variable */
let cur_env_vec = shell.get_envs();
let test_var = cstr16!("test_var");
let test_val = cstr16!("test_val");
assert!(shell.get_env(test_var).is_none());
let status = shell.set_env(test_var, test_val, false);
assert_eq!(status, Status::SUCCESS);
let cur_env_str = shell
.get_env(test_var)
.expect("Could not get environment variable");
assert_eq!(cur_env_str, test_val);

let mut found_var = false;
for env_var in cur_env_vec {
if env_var == test_var {
found_var = true;
}
}
assert!(!found_var);
let cur_env_vec = shell.get_envs();
let mut found_var = false;
for env_var in cur_env_vec {
if env_var == test_var {
found_var = true;
}
}
assert!(found_var);

let cur_env_vec = shell.get_envs();
assert_eq!(cur_env_vec.count(), default_len + 1);

/* Test deleting environment variable */
let test_val = cstr16!("");
let status = shell.set_env(test_var, test_val, false);
assert_eq!(status, Status::SUCCESS);
assert!(shell.get_env(test_var).is_none());

let cur_env_vec = shell.get_envs();
let mut found_var = false;
for env_var in cur_env_vec {
if env_var == test_var {
found_var = true;
}
}
assert!(!found_var);
let cur_env_vec = shell.get_envs();
assert_eq!(cur_env_vec.count(), default_len);
}

/// Test `get_cur_dir()` and `set_cur_dir()`
pub fn test_cur_dir(shell: &ScopedProtocol<Shell>) {
/* Test setting and getting current file system and current directory */
let fs_var = cstr16!("fs0:");
let dir_var = cstr16!("/");
let status = shell.set_cur_dir(Some(fs_var), Some(dir_var));
assert_eq!(status, Status::SUCCESS);

let cur_fs_str = shell
.get_cur_dir(Some(fs_var))
.expect("Could not get the current file system mapping");
let expected_fs_str = cstr16!("FS0:\\");
assert_eq!(cur_fs_str, expected_fs_str);

// Changing current file system
let fs_var = cstr16!("fs1:");
let dir_var = cstr16!("/");
let status = shell.set_cur_dir(Some(fs_var), Some(dir_var));
assert_eq!(status, Status::SUCCESS);

let cur_fs_str = shell
.get_cur_dir(Some(fs_var))
.expect("Could not get the current file system mapping");
assert_ne!(cur_fs_str, expected_fs_str);
let expected_fs_str = cstr16!("FS1:\\");
assert_eq!(cur_fs_str, expected_fs_str);

// Changing current file system and current directory
let fs_var = cstr16!("fs0:");
let dir_var = cstr16!("efi/");
let status = shell.set_cur_dir(Some(fs_var), Some(dir_var));
assert_eq!(status, Status::SUCCESS);

let cur_fs_str = shell
.get_cur_dir(Some(fs_var))
.expect("Could not get the current file system mapping");
assert_ne!(cur_fs_str, expected_fs_str);
let expected_fs_str = cstr16!("FS0:\\efi");
assert_eq!(cur_fs_str, expected_fs_str);

/* Test current working directory cases */

// At this point, the current working file system has not been set
// So we expect a NULL output
assert!(shell.get_cur_dir(None).is_none());

// Setting the current working file system and current working directory
let dir_var = cstr16!("fs0:/");
let status = shell.set_cur_dir(None, Some(dir_var));
assert_eq!(status, Status::SUCCESS);
let cur_fs_str = shell
.get_cur_dir(Some(fs_var))
.expect("Could not get the current file system mapping");
let expected_fs_str = cstr16!("FS0:");
assert_eq!(cur_fs_str, expected_fs_str);

let cur_fs_str = shell
.get_cur_dir(None)
.expect("Could not get the current file system mapping");
assert_eq!(cur_fs_str, expected_fs_str);

// Changing current working directory
let dir_var = cstr16!("/efi");
let status = shell.set_cur_dir(None, Some(dir_var));
assert_eq!(status, Status::SUCCESS);
let cur_fs_str = shell
.get_cur_dir(Some(fs_var))
.expect("Could not get the current file system mapping");
let expected_fs_str = cstr16!("FS0:\\efi");
assert_eq!(cur_fs_str, expected_fs_str);
let cur_fs_str = shell
.get_cur_dir(None)
.expect("Could not get the current file system mapping");
assert_eq!(cur_fs_str, expected_fs_str);

// Changing current directory in a non-current working file system
let fs_var = cstr16!("fs0:");
let dir_var = cstr16!("efi/tools");
let status = shell.set_cur_dir(Some(fs_var), Some(dir_var));
assert_eq!(status, Status::SUCCESS);
let cur_fs_str = shell
.get_cur_dir(None)
.expect("Could not get the current file system mapping");
assert_ne!(cur_fs_str, expected_fs_str);

let expected_fs_str = cstr16!("FS0:\\efi\\tools");
let cur_fs_str = shell
.get_cur_dir(Some(fs_var))
.expect("Could not get the current file system mapping");
assert_eq!(cur_fs_str, expected_fs_str);
}

pub fn test() {
info!("Running shell protocol tests");

let handle = boot::get_handle_for_protocol::<Shell>().expect("No Shell handles");

let mut _shell =
let shell =
boot::open_protocol_exclusive::<Shell>(handle).expect("Failed to open Shell protocol");

test_env(&shell);
test_cur_dir(&shell);
}
204 changes: 200 additions & 4 deletions uefi/src/proto/shell/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,208 @@

//! EFI Shell Protocol v2.2

use crate::proto::unsafe_protocol;
use uefi_macros::unsafe_protocol;
use uefi_raw::Status;

pub use uefi_raw::protocol::shell::ShellProtocol;
use core::marker::PhantomData;
use core::ptr;

use uefi_raw::protocol::shell::ShellProtocol;

use crate::{CStr16, Char16};

/// Shell Protocol
#[derive(Debug)]
#[repr(transparent)]
#[unsafe_protocol(uefi_raw::protocol::shell::ShellProtocol::GUID)]
pub struct Shell(uefi_raw::protocol::shell::ShellProtocol);
#[unsafe_protocol(ShellProtocol::GUID)]
pub struct Shell(ShellProtocol);

/// Iterator over the names of environmental variables obtained from the Shell protocol.
#[derive(Debug)]
pub struct Vars<'a> {
/// Char16 containing names of environment variables
inner: *const Char16,
/// Placeholder to attach a lifetime to `Vars`
placeholder: PhantomData<&'a CStr16>,
}

impl<'a> Iterator for Vars<'a> {
type Item = &'a CStr16;
// We iterate a list of NUL terminated CStr16s.
// The list is terminated with a double NUL.
fn next(&mut self) -> Option<Self::Item> {
let cur_start = self.inner;
let mut cur_len = 0;
unsafe {
if *(cur_start) == Char16::from_u16_unchecked(0) {
return None;
}
while *(cur_start.add(cur_len)) != Char16::from_u16_unchecked(0) {
cur_len += 1;
}
self.inner = self.inner.add(cur_len + 1);
Some(CStr16::from_ptr(cur_start))
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the implementation could be simplified, something like this:

    fn next(&mut self) -> Option<Self::Item> {
        let s = unsafe { CStr16::from_ptr(self.inner) };
        if s.is_empty() {
            None
        } else {
            self.inner = unsafe { self.inner.add(s.num_chars() + 1) };
            Some(s)
        }
    }


impl Shell {
/// Gets the value of the specified environment variable
///
/// # Arguments
///
/// * `name` - The environment variable name of which to retrieve the
/// value.
///
/// # Returns
///
/// * `Some(<env_value>)` - &CStr16 containing the value of the
/// environment variable
/// * `None` - If environment variable does not exist
#[must_use]
pub fn get_env(&self, name: &CStr16) -> Option<&CStr16> {
Copy link
Member

Choose a reason for hiding this comment

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

In #1679 (comment), it was suggested to use the same name as std: var. I think we should do that here, for several reasons:

let name_ptr: *const Char16 = core::ptr::from_ref::<CStr16>(name).cast();
let var_val = unsafe { (self.0.get_env)(name_ptr.cast()) };
if var_val.is_null() {
None
} else {
unsafe { Some(CStr16::from_ptr(var_val.cast())) }
}
}

/// Gets the list of environment variables
Copy link
Member

Choose a reason for hiding this comment

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

"Gets an iterator over the names of all environment variables."

///
/// # Returns
///
/// * `Vec<env_names>` - Vector of environment variable names
Copy link
Member

Choose a reason for hiding this comment

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

Out of date, it returns Vars now. You can probably drop the Returns section here though, it's redundent with the top-level description.

#[must_use]
pub fn get_envs(&self) -> Vars {
Copy link
Member

Choose a reason for hiding this comment

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

I think var_names would be a better name for this function (see comment on get_env for details). However, even better might be to make it match std and have Vars return an iterator over both names and values (and then the method can be named simply vars). Is that doable?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, sounds reasonable. High level abstractions should be close to known interfaces (such as std) and not replicate low-level details

let env_ptr = unsafe { (self.0.get_env)(ptr::null()) };
Vars {
inner: env_ptr.cast::<Char16>(),
placeholder: PhantomData,
}
}

/// Sets the environment variable
///
/// # Arguments
///
/// * `name` - The environment variable for which to set the value
/// * `value` - The new value of the environment variable
/// * `volatile` - Indicates whether the variable is volatile or
/// not
///
/// # Returns
///
/// * `Status::SUCCESS` - The variable was successfully set
pub fn set_env(&self, name: &CStr16, value: &CStr16, volatile: bool) -> Status {
Copy link
Member

Choose a reason for hiding this comment

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

Here and on other methods, return Result (https://docs.rs/uefi/latest/uefi/type.Result.html) instead of a raw Status. This is more convenient for users since it works with ?.

Copy link
Member

Choose a reason for hiding this comment

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

set_var to match std

let name_ptr: *const Char16 = core::ptr::from_ref::<CStr16>(name).cast();
Copy link
Member

Choose a reason for hiding this comment

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

Instead of core::ptr::from_ref, you can use name.as_ptr() (and ditto for value below).

let value_ptr: *const Char16 = core::ptr::from_ref::<CStr16>(value).cast();
unsafe { (self.0.set_env)(name_ptr.cast(), value_ptr.cast(), volatile) }
}

/// Returns the current directory on the specified device
///
/// # Arguments
///
/// * `file_system_mapping` - The file system mapping for which to get
/// the current directory
///
/// # Returns
///
/// * `Some(cwd)` - CStr16 containing the current working directory
/// * `None` - Could not retrieve current directory
#[must_use]
pub fn get_cur_dir(&self, file_system_mapping: Option<&CStr16>) -> Option<&CStr16> {
Copy link
Member

Choose a reason for hiding this comment

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

Rename to current_dir to match std.

let mapping_ptr: *const Char16 = file_system_mapping.map_or(ptr::null(), |x| (x.as_ptr()));
Copy link
Member

Choose a reason for hiding this comment

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

nit: parens not needed around x.as_ptr(). Might be clearer to write as map_or(ptr::null(), CStr16::as_ptr).

let cur_dir = unsafe { (self.0.get_cur_dir)(mapping_ptr.cast()) };
if cur_dir.is_null() {
None
} else {
unsafe { Some(CStr16::from_ptr(cur_dir.cast())) }
}
}

/// Changes the current directory on the specified device
///
/// # Arguments
///
/// * `file_system` - Pointer to the file system's mapped name.
/// * `directory` - Points to the directory on the device specified by
/// `file_system`.
///
/// # Returns
///
/// * `Status::SUCCESS` - The directory was successfully set
///
/// # Errors
///
/// * `Status::EFI_NOT_FOUND` - The directory does not exist
pub fn set_cur_dir(&self, file_system: Option<&CStr16>, directory: Option<&CStr16>) -> Status {
Copy link
Member

Choose a reason for hiding this comment

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

set_current_dir to match std

let fs_ptr: *const Char16 = file_system.map_or(ptr::null(), |x| (x.as_ptr()));
let dir_ptr: *const Char16 = directory.map_or(ptr::null(), |x| (x.as_ptr()));
unsafe { (self.0.set_cur_dir)(fs_ptr.cast(), dir_ptr.cast()) }
}
}

#[cfg(test)]
mod tests {
use super::*;
use alloc::vec::Vec;
use uefi::cstr16;

/// Testing Vars struct
#[test]
fn test_vars() {
// Empty Vars
let mut vars_mock = Vec::<u16>::new();
vars_mock.push(0);
vars_mock.push(0);
let mut vars = Vars {
inner: vars_mock.as_ptr().cast(),
placeholder: PhantomData,
};
assert!(vars.next().is_none());

// One environment variable in Vars
let mut vars_mock = Vec::<u16>::new();
vars_mock.push(b'f' as u16);
vars_mock.push(b'o' as u16);
vars_mock.push(b'o' as u16);
vars_mock.push(0);
vars_mock.push(0);
let vars = Vars {
inner: vars_mock.as_ptr().cast(),
placeholder: PhantomData,
};
assert_eq!(vars.collect::<Vec<_>>(), Vec::from([cstr16!("foo")]));

// Multiple environment variables in Vars
let mut vars_mock = Vec::<u16>::new();
vars_mock.push(b'f' as u16);
vars_mock.push(b'o' as u16);
vars_mock.push(b'o' as u16);
vars_mock.push(b'1' as u16);
vars_mock.push(0);
vars_mock.push(b'b' as u16);
vars_mock.push(b'a' as u16);
vars_mock.push(b'r' as u16);
vars_mock.push(0);
vars_mock.push(b'b' as u16);
vars_mock.push(b'a' as u16);
vars_mock.push(b'z' as u16);
vars_mock.push(b'2' as u16);
vars_mock.push(0);
vars_mock.push(0);

let vars = Vars {
inner: vars_mock.as_ptr().cast(),
placeholder: PhantomData,
};
assert_eq!(
vars.collect::<Vec<_>>(),
Vec::from([cstr16!("foo1"), cstr16!("bar"), cstr16!("baz2")])
);
}
}