-
Notifications
You must be signed in to change notification settings - Fork 159
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
uefi: add BootPolicy type #1326
Conversation
This type is used in three functions of the UEFI spec. Having an enum instead of a boolean simplifies the interface as the variants can be properly documented.
- public modules - private modules - public uses - private uses This is consistent with other parts of the code.
This adds the new `BootPolicy` type into `LoadImageSource`.
The existing code duplication is just temporary until the old API is deleted. Nevertheless, it is nicer to have this conversion close to the actual type.
/// various UEFI functions that load files (typically UEFI images). | ||
/// | ||
/// This type is not ABI compatible. On the ABI level, this is an UEFI | ||
/// boolean. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it could be made compatible: add repr(u8)
and BootSelection = 1
, ExactMatch = 0
.
(As long as this type is only passed into UEFI APIs, and not read from them; in that case newtype_enum
might be better.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to postpone that decision/discussion to #1307?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the type is only being passed into the API, UB shouldn't be a concern -- we'll always initialize BootPolicy
correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you just adding thoughts to the discussion or do you want this doccomment to be removed before we merge it? That's not clear to me right now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we should make changes before merging, but actually since there is no declared repr
right now, it wouldn't be a breaking change to add repr(u8)
in the future I think. So let's go ahead and merge.
This was decoupled from #1297 for better review-ability.
Checklist