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 7 commits into
base: main
Choose a base branch
from

Conversation

RenTrieu
Copy link
Contributor

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

  • Execute
  • GetEnv
  • SetEnv
  • GetAlias
  • SetAlias
  • GetHelpText
  • GetDevicePathFromMap
  • GetMapFromDevicePath
  • GetDevicePathFromFilePath
  • GetFilePathFromDevicePath
  • SetMap
  • GetCurDir
  • SetCurDir
  • OpenFileList
  • FreeFileList
  • RemoveDupInFileList
  • BatchIsActive
  • IsRootShell
  • EnablePageBreak
  • DisablePageBreak
  • GetPageBreak
  • GetDeviceName
  • GetFileInfo
  • SetFileInfo
  • OpenFileByName
  • CloseFile
  • CreateFile
  • ReadFile
  • WriteFile
  • DeleteFile
  • DeleteFileByName
  • GetFilePosition
  • SetFilePosition
  • FlushFile
  • FindFiles
  • FindFilesInDir
  • GetFileSize
  • OpenRoot
  • OpenRootByHandle
  • ExecutionBreak
  • Version Variables
  • RegisterGuidName
  • GetGuidName
  • GetGuidFromName
  • GetEnvEx

Copy link
Member

@phip1611 phip1611 left a comment

Choose a reason for hiding this comment

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

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

@nicholasbishop
Copy link
Member

Some general guidance:

  1. 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.)
  2. Once that's merged, do small PRs that add ergonomic wrappers to the uefi crate. Small PRs will help get review done faster.
  3. 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
Copy link
Contributor Author

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.

RenTrieu added 2 commits June 2, 2025 14:56
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
Copy link
Contributor Author

RenTrieu commented Jun 3, 2025

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
Copy link
Member

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 RenTrieu force-pushed the enhancement/efi_shell_interface branch from b9a3be8 to 0f30078 Compare June 3, 2025 18:31
@RenTrieu RenTrieu marked this pull request as ready for review June 3, 2025 20:06
@RenTrieu
Copy link
Contributor Author

RenTrieu commented Jun 3, 2025

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.

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.
@RenTrieu RenTrieu force-pushed the enhancement/efi_shell_interface branch from 36f37cc to 910e291 Compare June 11, 2025 02:17
@RenTrieu RenTrieu force-pushed the enhancement/efi_shell_interface branch from 910e291 to 42cad4a Compare June 11, 2025 02:24
@RenTrieu
Copy link
Contributor Author

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.


/// Contains environment variables
#[derive(Debug)]
pub struct Vars<'a> {
Copy link
Member

Choose a reason for hiding this comment

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

Great! Please also add a regular unit test for it. You can mock the corresponding data structure using something like:

let mut vars_mock = Vec::<u16>::new();
vars_mock.push(b'H' as u16);
vars_mock.push(b'i' as u16);
vars_mock.push(0);
vars_mock.push(b'B' as u16);
vars_mock.push(b'y' as u16);
vars_mock.push(b'e' as u16);
vars_mock.push(0);
vars_mock.push(0);


/* Test setting and getting a specific environment variable */
let cur_env_vec = shell.get_envs();
let mut test_env_buf = [0u16; 32];
Copy link
Member

Choose a reason for hiding this comment

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

no need for these buffers anymore ;)

@RenTrieu RenTrieu force-pushed the enhancement/efi_shell_interface branch from 5661358 to 6b35e5a Compare June 11, 2025 15:40
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.

3 participants