Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork175
Updating Uefi Raw for EFI Shell Protocol#1680
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Updating Uefi Raw for EFI Shell Protocol#1680
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for working on this! 🙌 Just a quick note: we value good git commit hygiene. This doesn't necessarily mean a single commit, but commits should follow this structure:
We can still squash-merge your PR if needed, but I wanted to make you aware of our expectations for future contributions. 🙂 |
pub struct ShellProtocol { | ||
pub execute: unsafe extern "efiapi" fn( | ||
parent_image_handle: *const Handle, | ||
command_line: *const Char16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicholasbishop, I think we should use
command_line:*constChar16, | |
command_line:Option<*constChar16>, |
here and other optional parameters, right? I know that this is not streamlined across the code base for allOPTIONAL
parameters. What do you say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
uefi-raw should stick close to the C API, so it should not useOption
. (Personally I think the UEFI spec should just remove theOPTIONAL
annotation, as they use it quite inconsistently and it doesn't have a clear purpose.)
(Also, it wouldn't be compatible with the C ABI unless we also switched from a raw pointer to NonNull.Option<NonNull>
is ABI compatible with a raw pointer, butOption<const*>
is not, because it can't use the niche optimization.)
Uh oh!
There was an error while loading.Please reload this page.
Thanks again for your feedback! In general, is the "component" the crate I'm modifying? (e.g. in my case the component would be |
Yes, but we are not overly strict. |
phip1611 commentedMay 31, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Also, please rebase your branch onto the latest upstream main when it is ready, rather than merging main into it. Also, it is fine to merge all commits of@timrobertsdev into one, as the commit messages litteraly say to squash them. In case you need help with git and the necessary steps, feel free to reach out to us! |
I believe that I've rebased the branch onto the latest upstream main. I've run the following commands:
After which, I would expect to run |
phip1611 commentedMay 31, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Ehh, where do you have that from?! The typical workflow is as follows: git checkout<your branch>git fetch --allgit rebase upstream/main<resolve all your possible conflicts and`git rebase --continue`until done)git push -f In case something goes wrong on your side, you can undo the rebase using a combination of Force-pushing to GitHub is fine, the comments will still be viewable. |
Ah, thank you for the recommended commands. I will follow it and push the results shortly. If you don't mind, I do have a follow up question regarding my initial atypical approach. I hope to better understand why the typical workflow is preferred so I can make better contributions in the future.
My intent with this was to combine all new commits in this branch into one and then add it back on top of the current main commit. The resulting log would look like:
Is this not preferred because it removes the history? I'm unclear if the result is different from rebasing and then squashing later. |
8401ea6
toce47a52
Comparephip1611 commentedJun 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Rebasing onto a more recent state and combining commits ("rewriting git history") are two distinct things. Please look at them separately to make things easier. In your case, one possible path forward would be to:
|
ce47a52
to4796ad4
CompareI see. Thank you for your guidance and patience. I've followed your instruction to squash the commits and then rebase the branch onto |
phip1611 commentedJun 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Philipp Schuster <phip1611@gmail.com>
Sounds good! I will squash them together. |
4796ad4
to4772497
CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
uefi-raw/src/protocol/shell.rs Outdated
/// Used to specify where component names should be taken from | ||
pub type ShellDeviceNameFlags = u32; | ||
pub const DEVICE_NAME_USE_COMPONENT_NAME: u32 = 0x0000001; | ||
pub const DEVICE_NAME_USE_DEVICE_PATH: u32 = 0x0000002; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Use thebitflags
macro for this type and constants. See for examplehttps://github.com/rust-osdev/uefi-rs/blob/main/uefi-raw/src/protocol/console/serial.rs#L6.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Thank you for the review,@nicholasbishop . I've addressed your comments, but I'm unsure if I've correctly used the |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
I've revised the comments relating to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Lgtm, thanks!
462de06
Uh oh!
There was an error while loading.Please reload this page.
This PR updates the
uefi-raw
crate to include the definition for the EFI Shell Protocol as part of the effort to address#448 . Please let me know if there's anything I missed or if there are any improvements I can make!