LGTM at a first glance. But I have one general question, tho.

Why does it seem like the UEFI specification allows multiple ways to achieve the same for so many things? For example, there is theEFI_SHELL_CREATE_FILE function which seems to do the same asEFI_FILE_PROTOCOL.Open() (with the CREATE flag)?

@phip1611 The UEFI spec is a true mystery sometimes 😆

Specifically for this protocol, I believe it's due to it being an interface to the EFI Shell application itself and not part of the core UEFI specification and interfaces. I'm assuming that EFI Shell does the obvious/simple thing under the hood and just wraps the normal firmware functions but I haven't taken a look at the source in edk2 yet.

Edit: Thinking about it more, is this the sort of protocol that should live in its own crate separate fromuefi core? It's a separate specification document, an EFI application instead of firmware, and while many vendors will include it, it's not required to be shipped alongside the firmware and can be provided externally. This would also mean that we could have a pure-Rust implementation of the shell itself as well as the API.

is ShellFileIter along the lines of what you were thinking for an iterator over the firmware-returned linked-list? The casts feel a little shaky to me but I'm fairly confident they're valid.

Yes, I haven't reviewed in detail yet but it seems about right to me.

Thinking about it more, is this the sort of protocol that should live in its own crate separate from uefi core?

I think it's fine to have in this crate; we already have at least one protocol that is defined outside the main spec and I plan to add another one soon for TPMs. Protocols should only impact the binary of an app if the app actually uses the protocol, so I don't think there's any problem with keeping it in this crate.

@phip1611@nicholasbishop

I've been trying to come up with a safe and ergonomic abstraction over the linked lists returned byfind_files and friends and have so far come up with theFileList struct. This involved a lot of turning pointers into references, and I believe it is sound, but another set of eyes or two would be appreciated.

FileList should be sound for the following reasons:

1. We keep a reference to the shell protocol as part of the struct and use that lifetime for all functions returning a lifetime. This ensures that there is no way to keep these references around unless the shell is still running.
2. TheFreeFileList UEFI function is not exposed. Instead, we call it during thedrop call forFileList to automatically call the firmware to free the memory when it is dropped. This ensures that the underlying structs are kept alive for the duration of any references to them.

Please let me know if either of these assumptions are incorrect; this much pointer wrangling and lifetime reasoning is a bit new for me.

Edit: I actually think I just found an issue regarding the lifetimes. If I'm using the protocol lifetime, then any returned references could be alive even after the list is dropped and they become invalid.

At a high level the lifetimes look right to me. Methods onFileList return references with a lifetime tied toself, and in turnFileList has a reference to the shell protocol to handle drop, so there should be no danger of any use-after-free.

I haven't reviewed in too much detail yet since the PR is still in draft, but I think this is the right direction :)

@timrobertsdev do you plan on adding support forEFI_SHELL_PARAMETERS_PROTOCOL andEFI_SHELL_DYNAMIC_COMMAND_PROTOCOL too?

@felipebalbi

Probably not in this PR but in a follow-up.

Hi@timrobertsdev, just checking if you need any assistance on our end in moving this PR forward? It would also be fine to split the work across multiple PRs, for example we could merge an initialShell protocol with only a few methods implemented, then add the more complicated ones over time.

No rush, just wanted to check :)

@nicholasbishop

Thanks for checking in! I actually did hit a bit of a roadblock when trying to test the protocol and then irl work kinda took over.

I'll have some upcoming free time to develop this a bit more, but I'm not sure how I would go about locating the EFI Shell application and starting it in order to run the tests. The high-level fs abstraction looks promising, as I wasn't able to find a good way to turn a filepath into aDevicePath I believe. Any advice for locating and starting an EFI application once we've started the test app? Or even if that's possible?

Any advice for locating and starting an EFI application once we've started the test app?

A couple thoughts:

1. You can load the raw bytes of an executable and pass them toload_image rather than using aDevicePath
2. Or, you can use aDevicePath. Roughly it could look something like this, assuming you are loading an executable from the same partition as the currently-running executrable: usehttps://docs.rs/uefi/latest/uefi/proto/loaded_image/struct.LoadedImage.html#method.file_path to get the currently-running executable's device path, then use thebuild API to make a new modified DevicePath with the file path part altered.

That's just my general advice for loading an executable, I can't say for sure it would help here since I'm not that familiar with the UEFI shell and haven't thought about exactly what this test would look like.

@nicholasbishop I'll give that a try this weekend. I'm still confused how I could start another app (EFI Shell) and still call into it from the previous app without multithreading, but I guess trying to run it would make that clear.

@nicholasbishop

I haven't been able to find a way to run tests on the EFI Shell app at all. While this protocol looks correct so far and compiles (minus the tests), I don't feel very comfortable merging it without working tests. Thoughts?

I've started putting together a solution for testing stuff inside the UEFI shell:#793

Not finished yet, but seems like a viable path.

Uh, I think I might've done something wrong with the git history while rebasing this onto the most recent master changes. Is that fixable?

Yes, this is fixable :) Try something like this:

First, use a simple merge to make sure your branch is fully up-to-date with the main branch. We're really just doing this for comparison purposes to make sure we don't lose any changes in the following rebase.

git fetch origingit checkout <your branch>git merge origin/main

Make a note of the resulting commit hash for later use.

git log -1<some commit hash>

Now initiate the rebase.

git rebase origin/main

This is going to hit a conflict like this:

CONFLICT (add/add): Merge conflict in uefi/src/proto/shell/mod.rserror: could not apply 36f405cc... Begin implementing the EFI Shell Protocol

In this case an earlier commit has added the file, and then we have a later commit adding the same file. Let's resolve by taking the later commit's version:

git checkout --theirs uefi/src/proto/shell/mod.rsgit add uefi/src/proto/shell/mod.rs

Now we can continue:

git rebase --continue

The rebase should now finish. We can validate that the result matches our test merge from the beginning:

git diff <some commit hash recorded earlier>

There should be no output, indicating we didn't make any file changes in the rebase.

The resulting history is still messy:

d35567cd (HEAD -> efi-shell) Rustfmt, clippy, squash.e100cb9e squash, rustfmtd8aa43ca Remove (non-working) tests to evaluate other options. Update error handling in line with the rest of the crate. squash this0697c2eb Implement some tests and `FileTree` abstraction.2256055a Begin implementing the EFI Shell Protocol This message should be squashed2c99e214 Implement some tests and `FileTree` abstraction.29bf4b47 Begin implementing the EFI Shell Protocol This message should be squashed

From there you can do an interactive rebase to squash stuff as desired:

git rebase -i HEAD~7

I haven't been able to find a way to run tests on the EFI Shell app at all. While this protocol looks correct so far and compiles (minus the tests), I don't feel very comfortable merging it without working tests. Thoughts?

The changes for running the tests under the shell have landed, so you should be able to add tests touefi-test-runner now :)

Awesome, I'll write up some tests asap so we can get this merged.

@nicholasbishop Or anyone else who sees this, I'm having a weird issue with my first test (testing thefind_files function). The function is returning INVALID_PARAMETER when calling the firmware'sShellFindFiles despite not appearing in the error list for the function in the specification.

https://github.com/tianocore/edk2/blob/57796711371d42d980d50bc9299972b109d09035/ShellPkg/Application/Shell/ShellProtocol.c#L2603 is the edk2 implementation, where it can return INVALID_PARAMETER in two difference places, though as far as I can tell, the inputs provided shouldn't be triggering it. Any thoughts? I'm gonna try and attach a debugger to qemu again to see what's going on on the firmware side.

edit: GetCurDir ends up returning null from firmware. I'm not sure why it thinks there aren't any filesystems mounted when at the very least the esp should be.

I think it's due to using*const CStr16 in the interface.CStr16 is a DST, so the pointer is a wide pointer that's not compatible with normal C pointers. Change it to*const Char16 instead:

diff --git a/uefi/src/proto/shell/mod.rs b/uefi/src/proto/shell/mod.rsindex 92548b7d..ae664f97 100644--- a/uefi/src/proto/shell/mod.rs+++ b/uefi/src/proto/shell/mod.rs@@ -59,7 +59,7 @@ pub struct Shell {     set_file_position: usize,     flush_file: extern "efiapi" fn(file_handle: ShellFileHandle) -> Status,     find_files: extern "efiapi" fn(-        file_pattern: *const CStr16,+        file_pattern: *const Char16,         out_file_list: *mut *mut ShellFileInfo,     ) -> Status,     find_files_in_dir: extern "efiapi" fn(@@ -179,7 +179,7 @@ impl Shell {         if out_ptr.is_null() {             panic!("outptr null");         }-        (self.find_files)(file_pattern, out_ptr).to_result_with_val(|| {+        (self.find_files)(file_pattern.as_ptr(), out_ptr).to_result_with_val(|| {             // safety: if we're here, out_list is valid, but maybe null             let out_list = unsafe { out_list.assume_init() };             if out_list.is_null() {

Thanks, I think I need to take a second look at everything that uses aCStr16,create_file and friends have been returning NOT_FOUND as well.

Closing this PR, unfortunately I don't have time to work on it and won't in the near future. If anyone wants to pick it up and finish it they're more than welcome to.

No worries, thanks for your efforts on it :)