Skip to content
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.

Sign up for GitHub

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

Jump to bottom

uefi: Implement getcwd and chdir #129794

Merged
merged 1 commit into from
Oct 15, 2024
Merged

uefi: Implement getcwd and chdir #129794

merged 1 commit into from
Oct 15, 2024

Conversation

Ayush1325
Copy link
Contributor

  • Using EFI Shell Protocol. These functions do not make much sense unless a shell is present.
  • Return the exe dir in case shell protocol is missing.

r? @joboet

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 30, 2024
@Ayush1325 Ayush1325 changed the title Implement getcwd and chdir uefi: Implement getcwd and chdir Aug 30, 2024
@Ayush1325 Ayush1325 force-pushed the uefi-os-expand branch 2 times, most recently from 0d0e44f to 48dd6a3 Compare August 31, 2024 08:14
@bors
Copy link
Contributor

bors commented Sep 23, 2024

☔ The latest upstream changes (presumably #130724) made this pull request unmergeable. Please resolve the merge conflicts.

@Ayush1325 Ayush1325 requested a review from bjorn3 October 8, 2024 06:42
@Ayush1325
Copy link
Contributor Author

ping @joboet

@joboet
Copy link
Member

joboet commented Oct 10, 2024

I'm very sorry for not getting back to you sooner.

The problem I see here is that GetCurDir returns a mapping, not a device path and will therefore not interoperate with our current filesystem implementation. I think there are two options we could take here:

  1. Normalize the mapping to a device path using GetDevicePathFromMap
  2. Use the filesystem functions from UEFI shell when available, which support both mappings and device paths.

Option 2 is more work, but has the advantage that File would support opening paths stored in environment variables, which will probably be mappings, not device paths, so I think that is the best option in the long run. If you want to get this PR merged first, then option 1 would also be fine for now.

@Ayush1325
Copy link
Contributor Author

Ayush1325 commented Oct 10, 2024 •
edited
Loading

I'm very sorry for not getting back to you sooner.

It's fine. Everyone is doing stuff in their free time after all.

The problem I see here is that GetCurDir returns a mapping, not a device path and will therefore not interoperate with our current filesystem implementation. I think there are two options we could take here:

  1. Normalize the mapping to a device path using GetDevicePathFromMap
  2. Use the filesystem functions from UEFI shell when available, which support both mappings and device paths.

Option 2 is more work, but has the advantage that File would support opening paths stored in environment variables, which will probably be mappings, not device paths, so I think that is the best option in the long run. If you want to get this PR merged first, then option 1 would also be fine for now.

Thanks for pointing it out. I will add a TODO to implement it in the fs PR. Having support for mappings in file i/o is essential.

Personally, I would like to stick to the normal UEFI file APIs, and add plumbing in Rust side to handle the mapping to device path. It is fairly simple to do since we have edk2 as reference.

@joboet
Copy link
Member

joboet commented Oct 13, 2024

Right, then let's ship this.
@bors r+

@bors
Copy link
Contributor

bors commented Oct 13, 2024

📌 Commit 1f718f9 has been approved by joboet

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 13, 2024
@matthiaskrgr
Rollup merge of rust-lang#129794 - Ayush1325:uefi-os-expand, r=joboet
c3c3000 
uefi: Implement getcwd and chdir

- Using EFI Shell Protocol. These functions do not make much sense unless a shell is present.
- Return the exe dir in case shell protocol is missing.

r? `@joboet`
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 13, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2024
@bors
Auto merge of rust-lang#131665 - matthiaskrgr:rollup-i5hluna, r=matth…
dbec5c4 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128967 (std::fs::get_path freebsd update.)
 - rust-lang#129794 (uefi: Implement getcwd and chdir)
 - rust-lang#130629 (core/net: add Ipv[46]Addr::from_octets, Ipv6Addr::from_segments.)
 - rust-lang#131274 (library: Const-stabilize `MaybeUninit::assume_init_mut`)
 - rust-lang#131473 (compiler: `{TyAnd,}Layout` comes home)
 - rust-lang#131533 (emscripten: Use the latest emsdk 3.1.68)
 - rust-lang#131593 (miri: avoid cloning AllocExtra)
 - rust-lang#131616 (merge const_ipv4 / const_ipv6 feature gate into 'ip' feature gate)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr
Copy link
Member

@bors r-
#131665 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 14, 2024
@Ayush1325
uefi: Implement getcwd and chdir
f8ac1c4 
- Using EFI Shell Protocol. These functions do not make much sense
  unless a shell is present.
- Return the exe dir in case shell protocol is missing.

Signed-off-by: Ayush Singh <[email protected]>
@bjorn3 bjorn3 removed their request for review October 14, 2024 09:53
@Ayush1325
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 14, 2024
@Ayush1325
Copy link
Contributor Author

@joboet @matthiaskrgr I think this should be ready to merge now.

@joboet
Copy link
Member

joboet commented Oct 15, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Oct 15, 2024

📌 Commit f8ac1c4 has been approved by joboet

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 15, 2024
@compiler-errors compiler-errors mentioned this pull request Oct 15, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2024
@bors
Auto merge of rust-lang#131747 - compiler-errors:rollup-0fnymws, r=co…
e7c0d27 
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#129794 (uefi: Implement getcwd and chdir)
 - rust-lang#130568 (Make some float methods unstable `const fn`)
 - rust-lang#131521 (rename RcBox to RcInner for consistency)
 - rust-lang#131701 (Don't report `on_unimplemented` message for negative traits)
 - rust-lang#131705 (Fix most ui tests on emscripten target)
 - rust-lang#131733 (Fix uninlined_format_args in stable_mir)
 - rust-lang#131734 (Update `arm64e-apple-tvos` maintainer)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 34636e6 into rust-lang:master Oct 15, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Oct 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 15, 2024
@rust-timer
Unrolled build for rust-lang#129794
ea82c64 
Rollup merge of rust-lang#129794 - Ayush1325:uefi-os-expand, r=joboet

uefi: Implement getcwd and chdir

- Using EFI Shell Protocol. These functions do not make much sense unless a shell is present.
- Return the exe dir in case shell protocol is missing.

r? `@joboet`
@Ayush1325 Ayush1325 deleted the uefi-os-expand branch October 16, 2024 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@joboet joboet

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Ayush1325 @bors @joboet @matthiaskrgr @bjorn3 @rustbot