-
-
Notifications
You must be signed in to change notification settings - Fork 177
Integration test for boot service function load_image #826
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
Integration test for boot service function load_image #826
Conversation
9ea69f2
to
c748974
Compare
c748974
to
88f46be
Compare
As to why strategy A fails: get_image_file_system()
tries to exclusively open LoadedImage protocol on the handle, which you already have open in this test function.
Personally, I am not a fan of seeing this crate throwing exclusive opening of protocols down everyone's throat.
This is what the spec says about exclusive opening:
EXCLUSIVE Used by applications to gain exclusive access to a protocol interface. If any drivers have the protocol
interface opened with an attribute of BY_DRIVER, then an attempt will be made to remove them by calling the driver’s
Stop() function.
This does not sound like something you'd wanna be doing willy-nilly with any protocol you may wanna open. I can also see child EFI apps not being able to open their own LoadedImage protocol just because the parent didn't bother to put their use of it inside a scope to force dropping it before starting the child.
There is also the matter of firmware/drivers already having an exclusive handle to something (I cannot get a GOP handle exclusively on my ASUS firmware + nvidia combo for example). So nudging everyone to use exclusive mode is just gonna result in a lot of annoyance.
Yes, the spec is annoyingly vague about all these protocol opening things. But the fact is, everyone on the C-side uses the old HandleProtocol
or OpenProtocol+GET_PROTOCOL
and it seems to work in practice.
e5b6b3e
to
fd1a9ca
Compare
Thanks very much @medhefgo for your help and advices! I've updated the PR.
Also what you've mentioned about open_protocol_exclusive
is a very good point. Thanks for the input!
Personally, I am not a fan of seeing this crate throwing exclusive opening of protocols down everyone's throat.
Well, that's a bit extreme :) For cases where you don't want an exclusive protocol, open_protocol can be used. We don't force anyone to open in exclusive mode.
One note, there will be overlap between this test and https://github.com/rust-osdev/uefi-rs/pull/793/files for testing shell protocols. I'm not necessarily opposed to having a bit of duplication in the tests though, just pointing it out.
fd1a9ca
to
6739187
Compare
Well, that's a bit extreme :) For cases where you don't want an exclusive protocol, open_protocol can be used. We don't force anyone to open in exclusive mode.
Any rust programmer worth their salt will use the non-unsafe interface, that's as good as forcing them (especially when the unsafe version is also unergonomic wrt to parameters to pass). Whether that interface is better/does the right thing is the question.
Any rust programmer worth their salt will use the non-unsafe interface, that's as good as forcing them
I agree, that's a valid point. Cognitive load is already relatively high (削除) in this crate (削除ここまで) when working with UEFI. So users will refrain from using more "unsafety" when there are "safe" interfaces
6739187
to
1463fc5
Compare
8df663b
to
8a9888a
Compare
Needs a rebase onto #849
8a9888a
to
a3f2c56
Compare
Are we good to go now? @nicholasbishop
545cf23
to
bb18eba
Compare
aca00d5
to
1d46a39
Compare
1d46a39
to
e9c69cb
Compare
Uh oh!
There was an error while loading. Please reload this page.
A colleague of mine (ping @scholzp) was about to use the
load_image
function of UEFI boot services to load an image into memory. While working on a minimal example, he encountered an unintuitive bug. As a consequence, I thought it's useful to have a valid integration test for that function which others can use as template.(削除) The test currently fails. What's wrong there with the chosen approach, @nicholasbishop ? UEFI fails withNOT_FOUND
whenload_image
is executed. (削除ここまで)Checklist