Add EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL and EFI_DEVICE_PATH_TO_TEXT_PROTOCOL #45
Conversation
Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>
Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>
| pub convert_device_node_to_text: DevicePathToTextNode, | ||
| pub convert_device_path_to_text: DevicePathToTextPath, |
There was a problem hiding this comment.
When I converted the stubs to proper prototypes, I chose to name them exactly like the member-names in the structure. You now stick to the names used in the spec (you called it DevicePathToTextNode, which matches the name in the spec rather than naming it ConvertDeviceNodeToText). I like it much more, but didn't spend the time to adjust all the other ones to their respective spec names.
Anyway, as a way forward, I prefer your approach of taking the names from the spec. But as an alternative, I am also fine with using the member-name and prefixing it with Protocol*, like we did before. We are a bit inconsistent then, but anyone who is bothered by that can easily put in the effort to get the names from the spec for the other prototypes and make the Protocol*-type a type-alias for it.
Long story short: I like your approach!
| *const crate::protocols::device_path::Protocol, | ||
| crate::base::Boolean, | ||
| crate::base::Boolean, | ||
| ) -> *const crate::base::Char16}; |
There was a problem hiding this comment.
This should be *mut, right? The value is allocated and free to the caller to use in any way they wish. They also have to release it. And the spec does not have any const annotation either. Same for the function below.
| ) -> *const crate::base::Char16}; | ||
|
|
||
| pub type DevicePathToTextPath = eficall! {fn( | ||
| *const crate::protocols::device_path::Protocol, |
There was a problem hiding this comment.
I converted this to *mut now, to be the same as the other protocols. We never applied *const annotations to function parameters so far, since they have been violated a lot in UEFI implementations, and it just makes rust miscompile when adding safe-abstractions that rely on those annotations. We always assumed *mut so far, and left it to safe abstractions to evaluate whether it actually requires mutable state, or whether they can fake the mut-pointer.
I am open to re-evaluating our position, but I want this as a separate discussion, not as part of this protocol addition. Note that the HII interfaces slipped through review and have some *const annotations. But that's mostly for historical reasons. If we want this, I think we should check whether they make sense and actually document where and when we use it.
Note that regarding UB both *const and *mut are equivalent in Rust. In fact, there have been thoughts on just providing one type of raw pointers in rust. The only real difference of both regarding FFI is their coercion from &v and &mut v. You can get *const v from &v automatically, but not from &mut v. In the latter case you explicitly need to as-cast from *const to *mut, which is completely legal and allowed.
There was a problem hiding this comment.
Yeah, I think a mutable pointer is better here. After some looking around, it seems the spec here means it's a pointer to a constant (which is different from a const pointer) according to the spec. I will open a separate issue where this can be discussed.
| pub type DevicePathFromTextNode = eficall! {fn( | ||
| *const crate::base::Char16, | ||
| ) -> *mut crate::protocols::device_path::Protocol}; | ||
|
|
||
| pub type DevicePathFromTextPath = eficall! {fn( | ||
| *const crate::base::Char16, | ||
| ) -> *mut crate::protocols::device_path::Protocol}; |
There was a problem hiding this comment.
I kept the *const annotations here, as I think for strings they are common enough and well defined.
No description provided.