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

phip1611 · 2025-05-30T06:32:15Z

uefi/src/proto/shell/mod.rs

+    /// * `Some(Vec<env_names>)` - Vector of environment variable names
+    /// * `None` - Environment variable doesn't exist
+    #[must_use]
+    pub fn get_env<'a>(&'a self, name: Option<&CStr16>) -> Option<EnvOutput<'a>> {


I'd much rather have the high-level API to not replicate the low-level interface here. Instead, we can use the low-level interface to create an API similar to:

std::env::var

Returns an Option (Or Result>), depending on your impl

std::env::vars

Returns a Vec in our case

Then we also don't need the EnvOutput type effectively acting as a Either type.

I've changed get_env() to return an Option<Vec<&CStr16>>. In the case where name is specified, the function will return a Vec of length 1 where the resulting &CStr16 contains the value of that environment variable. Otherwise in the name=None case, the Vec would contain the names of all environment variables. That way, the caller can just deal with the one type.

Is this more so along the lines of what you are thinking?

Also, I've chosen to wrap the return value in Option since the specification returns a NULL pointer in the case that name does not point to a valid environment variable, so I don't believe there are any errors we'd expect to return.

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

phip1611 · 2025-05-30T06:35:40Z

uefi/src/proto/shell/mod.rs

+/// TODO
+#[repr(C)]
+#[derive(Debug, Clone, Copy)]
+pub struct ListEntry {


Having now lifetime here makes it easy to introduce bugs. Also, this is code that we can't test using miri unfortunately.

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.

phip1611 · 2025-06-05T06:20:07Z

uefi/src/proto/shell/mod.rs

+    /// * `Some(Vec<env_names>)` - Vector of environment variable names
+    /// * `None` - Environment variable doesn't exist
+    #[must_use]
+    pub fn get_env<'a>(&'a self, name: Option<&CStr16>) -> Option<Vec<&'a CStr16>> {


Please don't export this unintuitive/unrusty UEFI API in this high-level wrapper. Instead, please aim to stay close to std. This means:

https://doc.rust-lang.org/proc_macro/tracked_env/fn.var.html (with `&CStr16)

https://doc.rust-lang.org/std/env/fn.vars.html )

Hi Philipp, thanks again for the resources. However, it is not clear to me what you are asking. Are you asking for get_env() to have a similar return type to the var() and vars() functions? Or are you asking for get_env() to be split up into two separate functions- one dedicated to returning the value of an environment variable and the other dedicated to returning the current environment variables?

Sorry for the confusion!

Or are you asking for get_env() to be split up into two separate functions- one dedicated to returning the value of an environment variable and the other dedicated to returning the current environment variables?

This.

No worries! I can see how having separate dedicated functions is more intuitive. In the spirit of mirroring std, I have updated the protocol with these two new functions using the same naming convention as var and vars where:
get_env() - retrieves the value of an environment variable
get_envs() - retrieves the names of all environment variables

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.

phip1611 reviewed May 30, 2025

View reviewed changes

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

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

uefi: Revising EFI Shell Protocol docstrings

0894142

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

phip1611 requested changes Jun 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Enhancement/efi shell interface #1679

Enhancement/efi shell interface #1679

Uh oh!

RenTrieu commented May 30, 2025

Uh oh!

phip1611 May 30, 2025 •

edited

Loading

Uh oh!

RenTrieu Jun 3, 2025

Uh oh!

phip1611 left a comment •

edited

Loading

Uh oh!

phip1611 May 30, 2025

Uh oh!

Uh oh!

nicholasbishop commented May 30, 2025

Uh oh!

RenTrieu commented May 30, 2025

Uh oh!

RenTrieu commented Jun 3, 2025

Uh oh!

nicholasbishop commented Jun 3, 2025

Uh oh!

RenTrieu commented Jun 3, 2025

Uh oh!

phip1611 Jun 5, 2025

Uh oh!

RenTrieu Jun 5, 2025

Uh oh!

phip1611 Jun 5, 2025

Uh oh!

RenTrieu Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

Enhancement/efi shell interface #1679

Are you sure you want to change the base?

Enhancement/efi shell interface #1679

Uh oh!

Conversation

RenTrieu commented May 30, 2025

Checklist

Uh oh!

phip1611 May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RenTrieu Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

phip1611 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phip1611 May 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicholasbishop commented May 30, 2025

Uh oh!

RenTrieu commented May 30, 2025

Uh oh!

RenTrieu commented Jun 3, 2025

Uh oh!

nicholasbishop commented Jun 3, 2025

Uh oh!

RenTrieu commented Jun 3, 2025

Uh oh!

phip1611 Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

RenTrieu Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

phip1611 Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

RenTrieu Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

phip1611 May 30, 2025 •

edited

Loading

phip1611 left a comment •

edited

Loading