Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

RenTrieu
Copy link
Contributor

This PR updates theuefi-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!

@phip1611
Copy link
Member

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:

<component>: <short description>A more detailed explanation of *what* was done and *why* it was necessary or beneficial.

We can still squash-merge your PR if needed, but I wanted to make you aware of our expectations for future contributions. 🙂

@phip1611phip1611 self-requested a reviewMay 31, 2025 06:49
pub struct ShellProtocol {
pub execute: unsafe extern "efiapi" fn(
parent_image_handle: *const Handle,
command_line: *const Char16,
Copy link
Member

@phip1611phip1611May 31, 2025
edited
Loading

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

Suggested change
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?

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

@RenTrieu
Copy link
ContributorAuthor

Thanks again for your feedback! In general, is the "component" the crate I'm modifying? (e.g. in my case the component would beuefi-raw)?

@phip1611
Copy link
Member

Yes, but we are not overly strict.uefi-raw: add EFI Shell protocol might be a good start,

@phip1611
Copy link
Member

phip1611 commentedMay 31, 2025
edited
Loading

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!

@RenTrieu
Copy link
ContributorAuthor

I believe that I've rebased the branch onto the latest upstream main. I've run the following commands:

git reset --soft <commit of latest upstream/main>git add -A .git commit

After which, I would expect to rungit push --force.
Does this look right? Also, I'm unclear on how rebasing and force pushing will affect the PR thread on Github. Should I wait for@nicholasbishop to respond on the open comment before pushing?

@phip1611
Copy link
Member

phip1611 commentedMay 31, 2025
edited
Loading

git reset --soft <commit of latest upstream/main>
git add -A .
git commit

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 ofgit reflog andgit checkout -f HEAD@{}.

Force-pushing to GitHub is fine, the comments will still be viewable.

@RenTrieu
Copy link
ContributorAuthor

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.

git reset --soft <commit of latest upstream/main>
git add -A .
git commit

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:

* commit bb92c1a659ea0af6f135d2cd6de8c9c7c3911a96 (HEAD -> enhancement/efi_shell_protocol_uefi_raw)| Author: Ren Trieu <ren.trieu.n@gmail.com>| Date:   Sat May 31 09:25:28 2025 -0700| |     uefi-raw: Add EFI Shell Protocol|   *   commit dab0752e393638d8bef93c6d85839b5b04fd1461 (upstream/main, upstream/HEAD)|\  Merge: d7cec59e 080e87b2| | Author: Nicholas Bishop <nbishop@nbishop.net>| | Date:   Thu May 29 19:23:40 2025 +0000| | | |     Merge pull request #1674 from seijikun/mr-safe-pciroot| |     | |     uefi: Add (partial) safe protocol implementation for PCI_ROOT_BRIDGE_IO_PROTOCOL| |

Is this not preferred because it removes the history? I'm unclear if the result is different from rebasing and then squashing later.

@RenTrieuRenTrieuforce-pushed theenhancement/efi_shell_protocol_uefi_raw branch from8401ea6 toce47a52CompareMay 31, 2025 19:25
@phip1611
Copy link
Member

phip1611 commentedJun 1, 2025
edited
Loading

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.

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:

  1. squash your commits into one or fewer using:
  • git rebase --interactive HEAD~11 (the number depends on where you want to start the interactive rebase)
  • use thes command to squash commits
  • ChatGPT will guide you through this with this prompt:
Please create a minimal example (print the git commands with explanation) to do an interactive rebase of the past three commits. The commits 2 and 3 should be squashed into the first commit
  1. rebase onto origin/main afterwards
RenTrieu reacted with thumbs up emoji

@RenTrieuRenTrieuforce-pushed theenhancement/efi_shell_protocol_uefi_raw branch fromce47a52 to4796ad4CompareJune 1, 2025 13:12
@RenTrieu
Copy link
ContributorAuthor

I see. Thank you for your guidance and patience. I've followed your instruction to squash the commits and then rebase the branch ontoupstream/main. Please let me know if the branch now looks alright or if there are still issues I should address.

@phip1611
Copy link
Member

phip1611 commentedJun 1, 2025
edited
Loading

yep, well done :) IMHO, you can also squash these three into the first one:
image

Co-authored-by: Philipp Schuster <phip1611@gmail.com>
@RenTrieu
Copy link
ContributorAuthor

Sounds good! I will squash them together.

phip1611 reacted with laugh emoji

@RenTrieuRenTrieuforce-pushed theenhancement/efi_shell_protocol_uefi_raw branch from4796ad4 to4772497CompareJune 1, 2025 17:01
/// 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;

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.

@RenTrieu
Copy link
ContributorAuthor

Thank you for the review,@nicholasbishop . I've addressed your comments, but I'm unsure if I've correctly used thebitflags! macro. Would you mind taking a look at my usage?

nicholasbishop reacted with thumbs up emoji

@RenTrieu
Copy link
ContributorAuthor

I've revised the comments relating toShellDeviceNameFlags. Please let me know if there is anything else I should address.

Copy link
Member

@nicholasbishopnicholasbishop left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks!

@nicholasbishopnicholasbishop added this pull request to themerge queueJun 2, 2025
Merged via the queue intorust-osdev:main with commit462de06Jun 2, 2025
16 checks passed
@RenTrieuRenTrieu mentioned this pull requestJun 3, 2025
45 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@phip1611phip1611phip1611 left review comments

@nicholasbishopnicholasbishopnicholasbishop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@RenTrieu@phip1611@nicholasbishop@timrobertsdev

[8]ページ先頭

©2009-2025 Movatter.jp