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.

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

Implement Deref and DerefMut for ScopedProtocol #434

Merged
merged 3 commits into from
May 30, 2022
Merged

Implement Deref and DerefMut for ScopedProtocol #434

merged 3 commits into from
May 30, 2022

Conversation

ctrlaltmilk
Copy link
Contributor

Since ScopedProtocol's only use is to be dereferenced, it makes sense to add a Deref impl so users don't have to do it manually. This also improves safety, since with this change it's harder to accidentally keep a reference to a protocol alive after exiting boot services.

@nicholasbishop
Copy link
Member

Thanks for the PR! I like the change to add Deref and DerefMut, but I also want to think more about whether it's OK that the final call sites no longer have any unsafe. The current convention that accessing a protocol always requires the call site to use unsafe predates my time working on this project, so also want to hear what @GabrielMajeri thinks. It's possible that the specific issues the unsafe was guarding against are more of a problem with handle_protocol than with open_protocol, but I'm not sure.

@GabrielMajeri
Copy link
Collaborator

The current convention that accessing a protocol always requires the call site to use unsafe predates my time working on this project, so also want to hear what @GabrielMajeri thinks.

The logic was as follows: in the early stages of uefi-rs, we didn't want to mark as "safe" functions which could in fact be unsafe (but not explicitly marked as such due to our lack of foresight). So the safe approach was to mark the creation of protocols as "unsafe", as a disclaimer to indicate to the caller that it's their responsibility to check that their usage of the protocol doesn't lead to potential UB.

I think it's OK now to make these methods safe, and fix any unsafety bugs which we encounter along the way.

@ctrlaltmilk
Copy link
Contributor Author

In my personal opinion, I think the constant use of unsafe blocks "devalues" the idea of unsafety when using this library. While I do agree that it's fundamentally important to mark unsafe behavior, I also think that if there's a core abstraction to the library designed to prevent unsafety, it should be as user-friendly as possible. To that end, I'm open to doing work on ScopedProtocol myself to make it safer - if there's a specific holdup to this, just let me know and I can start work on fixing it.

@GabrielMajeri
Copy link
Collaborator

@verticalegg I agree with you. Let's try merging and using Deref and DerefMut as is, and we'll take steps later to eliminate unsafe uses of it -- if it's the case.

@nicholasbishop
Copy link
Member

Great! I think that's a very nice usability improvement :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants