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

Change Handle representation to be non-nullable so that Option<Handle> is FFI-safe #309

Merged
merged 1 commit into from
Nov 24, 2021

Conversation

necauqua
Copy link
Contributor

@necauqua necauqua commented Nov 8, 2021

While I rewrote the OUT pointers in UEFI methods to use MaybeUninit (which actually looks very clean and logical) to avoid ever having null handles, the reason for the repr change is so that whenever an UEFI method accepts a handle - by the docs it can be optional or not, and this can be expressed by just having either Handle or Option<Handle> in the fn-pointer args, which is very clean and removes the need for null handles (Handle::unitialized()) to ever exist on Rust side.

I spent a night debugging my impls of (dis)connect_controller (that I will make a PR for too at some point) only to realize that I automatically assumed that whatever I did in the PR was already there ¯_(ツ)_/¯

Copy link
Contributor

@nicholasbishop nicholasbishop left a comment

Choose a reason for hiding this comment

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

Looks like a good change to me, left a couple comments. Please also rebase the fixup commits into the first commit.

src/data_types/mod.rs Outdated Show resolved Hide resolved
src/table/boot.rs Show resolved Hide resolved
@necauqua
Copy link
Contributor Author

Also I've just realised that an identical change can be made for Event as well, there are optional (nullable) event parameters in some functions in some protocols, I'll need to look into that I guess

@necauqua
Copy link
Contributor Author

Oh wow on ut/doctest stage it does not know that such feature exists, no idea how to fix it besides reverting back to using ManuallyDrop and stuff

@nicholasbishop
Copy link
Contributor

nicholasbishop commented Nov 23, 2021

Ah that is confusing. I think it's because that feature is part of alloc but not core, since Vec isn't in core. But those tests are running with all feature disabled, so the alloc crate isn't available.

To fix, gate the feature behind exts:

diff --git a/src/lib.rs b/src/lib.rs
index b12602b..035aee3 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -23,13 +23,12 @@
 //! For example, a PC with no network card might not contain a network driver,
 //! therefore all the network protocols will be unavailable.
 
-#![cfg_attr(feature = "exts", feature(allocator_api, alloc_layout_extra))]
+#![cfg_attr(feature = "exts", feature(allocator_api, alloc_layout_extra, vec_into_raw_parts))]
 #![feature(auto_traits)]
 #![feature(control_flow_enum)]
 #![feature(try_trait_v2)]
 #![feature(abi_efiapi)]
 #![feature(negative_impls)]
-#![feature(vec_into_raw_parts)]
 #![no_std]
 // Enable some additional warnings and lints.
 #![warn(missing_docs, unused)]

@nicholasbishop
Copy link
Contributor

Please rebase against latest master to resolve the conflict in boot.rs, and then I think it'll be good to merge.

@necauqua
Copy link
Contributor Author

Right, totally forgot about the other MR

@nicholasbishop
Copy link
Contributor

Thanks, merging now.

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.

2 participants