Enhancement/efi shell interface #1679

RenTrieu · 2025-05-30T01:04:27Z

This PR will implement the wrappers for EFI Shell Protocol functions to address #448 . This PR does branch off of the changes made in #596 so big thanks to @timrobertsdev for laying the groundwork!

Currently, I've implemented wrappers and wrote tests for GetEnv(), SetEnv(), GetCurDir() and SetCurDir(). My plan is to first write tests for and finish implementing Execute(), CloseFile(), CreateFile(), FindFiles(), FindFilesInDir(), and GetFileSize() since they have already been started. Then I can implement the rest in either this PR or subsequent follow up PRs (whichever is preferable).

I do have some questions about my get_env() implementation. UEFI's GetEnv() returns the value of a variable if its name is specified. However if its name is not specified, it returns all known variable names in a *const Char16 where the names are delimited by a NULL and the end of the list is terminated by a double NULL.

My initial approach was to parse the *const Char16 into a Vec in this case. I wrapped the return value in an Enum that can be unpacked into a single &CStr16 or into a vector of &CStr16s depending on which is expected. Is this approach okay? I was concerned that if I simply returned a &CStr16 in the "name list" case that the true size of the string wouldn't be visible since the names are separated by NULL. Also, is it okay for me to use alloc::vec::Vec? I saw that a lot of other protocols don't use any Vec at all so I'm not clear on if there are some restrictions for doing so.

Checklist

uefi/src/proto/shell/mod.rs

phip1611

Thanks for working on this! As a general rule of thumb: High-level wrappers should be close to the API of std and not replicate "ugly" or unusual details of the low-level API. See my suggestions for get_var

uefi/src/proto/shell/mod.rs

nicholasbishop · 2025-05-30T17:14:42Z

Some general guidance:

Start with a PR that just updates uefi-raw. (As Philipp noted here, the definition of the shell protocol that matches the C spec should go in uefi-raw. See for example RngProtocol. api_guidelines has some notes about how to implement stuff in uefi-raw.)
Once that's merged, do small PRs that add ergonomic wrappers to the uefi crate. Small PRs will help get review done faster.
Regarding Vec, it's generally OK to use, but does need to be gated by the alloc feature. (I haven't looked at the code yet though, I'm just commenting generally.)

RenTrieu · 2025-05-30T22:27:26Z

Thanks for the feedback! I will start with making the PR to update uefi-raw as Nicholas suggested, then come back to address the other comments.

This commit implements wrappers for the following EFI Shell Protocol functions: set_env(), get_env(), set_cur_dir(), and get_cur_dir().

This commit includes tests for the following EFI Shell Protocol functions: get_env(), set_env(), get_cur_dir(), and set_cur_dir().

RenTrieu · 2025-06-03T14:48:16Z

Hi all. Now that #1680 is merged, I was wondering if it would be alright to use this PR to work on the 4 ergonomic wrappers I've implemented (GetEnv(), SetEnv(), GetCurDir(), SetCurDir()). Is this sufficiently small enough? Or should I split this into two PRs- one for the Env functions and the other for CurDir?

If it is okay to continue to use this PR, then I plan to rebase this branch onto the current main, squash the commits, and then continue from there.

nicholasbishop · 2025-06-03T14:56:16Z

That sounds good, start with doing that in this PR. Once the changes are in this PR it's possible I might ask for it to be split up into more than one PR for review, but we can start with the assumption that one PR is OK.

RenTrieu · 2025-06-03T20:21:07Z

I've rebased this branch onto the current main and updated it to disclude the ShellProtocol definition moved to uefi-raw. I was wondering if there is any guidance for writing tests. So far, I have done my best to look at other files and imitate the format such as uefi-test-runner/src/proto/media.rs and uefi-test-runner/src/proto/rng.rs, but if there is anything more specific I can revise what I have.

uefi/src/proto/shell/mod.rs

RenTrieu · 2025-06-11T03:01:04Z

Thanks for the feedback! I have updated get_envs() to return a Vars struct that implements the Iterator trait. However, I do have a question about my implementation. When checking if a given Char16 is NULL, I use

Char16::from_u16_unchecked(0)

to represent the NULL value for the comparison. I saw that one of the suggestions requested to use

Char16::from_u16(0).unwrap()

instead, but I don't see this function implemented for Char16. Is it alright to use the unchecked variant?

I'll start working on unit tests next! However, for Miri I will need to take some time to read its docs since I'm not familiar.

uefi/src/proto/shell/mod.rs

uefi-test-runner/src/proto/shell.rs

RenTrieu · 2025-06-13T02:37:37Z

I've updated the PR to use the cstr16! macro instead of the buffers and also added a unit test for the Vars struct. Thank you for the tip on mocking the inner value with the Vec, it was very helpful. Please let me know if the unit test looks alright.

uefi-test-runner/src/proto/shell.rs

phip1611 · 2025-06-13T06:45:19Z

LGTM! Please squash your commits a little to smaller but sensible units, then I think we are good to go

uefi-test-runner/src/proto/shell.rs

The UEFI get_env() implementation is used for getting individual environment variable values and also environment variable names. This is not intuitive so this commit splits the function into two dedicated ones: one for accessing values and the other for accessing names. Co-authored-by: Philipp Schuster <[email protected]>

Co-authored-by: Philipp Schuster <[email protected]>

RenTrieu · 2025-06-13T22:48:26Z

Commits have been squashed :)

nicholasbishop · 2025-06-19T18:07:30Z

uefi/src/proto/shell/mod.rs

+    ///   environment variable
+    /// * `None` - If environment variable does not exist
+    #[must_use]
+    pub fn get_env(&self, name: &CStr16) -> Option<&CStr16> {


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

Consistency in APIs is good; anyone who is familiar with std::env will instinctively know how to use this API

Per https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter, Rust APIs generally should not use a get_ prefix

It's clearer to say you are getting the variable (noun) rather than the environment (which can be a noun, but in "environment variable" it's an adjective).

nicholasbishop · 2025-06-19T18:09:25Z

uefi/src/proto/shell/mod.rs

+    ///
+    /// # Returns
+    ///
+    /// * `Vec<env_names>` - Vector of environment variable names


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

nicholasbishop · 2025-06-19T18:10:06Z

uefi/src/proto/shell/mod.rs

+        }
+    }
+
+    /// Gets the list of environment variables


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

nicholasbishop · 2025-06-19T18:12:10Z

uefi/src/proto/shell/mod.rs

+    ///
+    /// * `Vec<env_names>` - Vector of environment variable names
+    #[must_use]
+    pub fn get_envs(&self) -> Vars {


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?

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

nicholasbishop · 2025-06-19T18:15:51Z

uefi/src/proto/shell/mod.rs

+    ///
+    /// * `Status::SUCCESS` - The variable was successfully set
+    pub fn set_env(&self, name: &CStr16, value: &CStr16, volatile: bool) -> Status {
+        let name_ptr: *const Char16 = core::ptr::from_ref::<CStr16>(name).cast();


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

nicholasbishop · 2025-06-19T18:18:16Z

uefi/src/proto/shell/mod.rs

+    /// * `None` - Could not retrieve current directory
+    #[must_use]
+    pub fn get_cur_dir(&self, file_system_mapping: Option<&CStr16>) -> Option<&CStr16> {
+        let mapping_ptr: *const Char16 = file_system_mapping.map_or(ptr::null(), |x| (x.as_ptr()));


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

nicholasbishop · 2025-06-19T18:18:42Z

uefi/src/proto/shell/mod.rs

+    /// # Errors
+    ///
+    /// * `Status::EFI_NOT_FOUND` - The directory does not exist
+    pub fn set_cur_dir(&self, file_system: Option<&CStr16>, directory: Option<&CStr16>) -> Status {


set_current_dir to match std

nicholasbishop · 2025-06-19T18:19:29Z

uefi/src/proto/shell/mod.rs

+    /// # Returns
+    ///
+    /// * `Status::SUCCESS` - The variable was successfully set
+    pub fn set_env(&self, name: &CStr16, value: &CStr16, volatile: bool) -> Status {


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 ?.

nicholasbishop · 2025-06-19T18:20:27Z

uefi/src/proto/shell/mod.rs

+    /// # Returns
+    ///
+    /// * `Status::SUCCESS` - The variable was successfully set
+    pub fn set_env(&self, name: &CStr16, value: &CStr16, volatile: bool) -> Status {


set_var to match std

nicholasbishop · 2025-06-19T18:29:04Z

uefi/src/proto/shell/mod.rs

+            Some(CStr16::from_ptr(cur_start))
+        }
+    }
+}


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) } }

phip1611 reviewed May 30, 2025

View reviewed changes

uefi/src/proto/shell/mod.rs Outdated Show resolved Hide resolved

phip1611 requested changes May 30, 2025

View reviewed changes

phip1611 reviewed May 30, 2025

View reviewed changes

uefi/src/proto/shell/mod.rs Outdated Show resolved Hide resolved

phip1611 reviewed May 30, 2025

View reviewed changes

uefi/src/proto/shell/mod.rs Outdated Show resolved Hide resolved

RenTrieu added 2 commits June 2, 2025 14:56

uefi: Implementing wrappers for EFI Shell env and cur_dir functions

92e07bb

This commit implements wrappers for the following EFI Shell Protocol functions: set_env(), get_env(), set_cur_dir(), and get_cur_dir().

uefi-test-runner: Added tests for EFI Shell env and cur_dir functions

0f30078

This commit includes tests for the following EFI Shell Protocol functions: get_env(), set_env(), get_cur_dir(), and set_cur_dir().

RenTrieu force-pushed the enhancement/efi_shell_interface branch from b9a3be8 to 0f30078 Compare June 3, 2025 18:31

RenTrieu marked this pull request as ready for review June 3, 2025 20:06

phip1611 requested changes Jun 5, 2025

View reviewed changes

uefi/src/proto/shell/mod.rs Outdated Show resolved Hide resolved

phip1611 reviewed Jun 10, 2025

View reviewed changes

uefi/src/proto/shell/mod.rs Outdated Show resolved Hide resolved

phip1611 reviewed Jun 10, 2025

View reviewed changes

uefi/src/proto/shell/mod.rs Outdated Show resolved Hide resolved

phip1611 reviewed Jun 10, 2025

View reviewed changes

uefi/src/proto/shell/mod.rs Outdated Show resolved Hide resolved

phip1611 reviewed Jun 10, 2025

View reviewed changes

uefi/src/proto/shell/mod.rs Outdated Show resolved Hide resolved

RenTrieu force-pushed the enhancement/efi_shell_interface branch 2 times, most recently from 910e291 to 42cad4a Compare June 11, 2025 02:24