-
-
Notifications
You must be signed in to change notification settings - Fork 180
uefi-raw: improve convenience of net types #1699
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
base: main
Are you sure you want to change the base?
Conversation
2b8f102
to
85838ce
Compare
For the record. Starting Friday, I'll be on vacation until July 30th. My plan is to program-detox for the whole period 😀 Restarting the work afterwards |
I'm unsure how I could fix the rustdoc error best. I want the documentation of the However: making the module public allows to import the same types from |
What if we removed the module doc and instead added this information to the individual types? The module doc is basically saying "you can convert between /// An IPv4 internet protocol address.
///
/// This type can be converted to and from [`core::net::Ipv4Addr`] using the [`From`] trait.
|
c2d44b0
to
24d5443
Compare
Yeah, good idea, why not :) What do you think about the latest state? |
411d16e
to
f2b5198
Compare
Generally looks good, but I haven't reviewed the details yet. Could you open a PR that just does the copy-paste move of stuff to a new module first? That should be very quick to review and merge, and then it won't get mixed in with the other changes. (I realize it's already broken down that way by commit, but there are still a lot of changes in commit 870aced that I would like to review more thoroughly, and it'll be easier for me if the module-move has already been merged.) |
aa1b362
to
4261a10
Compare
should be much easier to review now |
uefi-raw/src/net.rs
Outdated
@@ -83,19 +119,65 @@ pub union IpAddress { | |||
} | |||
|
|||
impl IpAddress { | |||
/// Construct a new zeroed address. | |||
#[must_use] | |||
pub const fn new_zeroed() -> Self { |
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.
How about an associated const
instead of a function?
pub const ZERO: Self = Self { addr: [0; 4] };
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 like the idea, thanks
uefi-raw/src/net.rs
Outdated
@@ -24,19 +32,32 @@ impl Ipv4Address { | |||
} | |||
} | |||
|
|||
impl From<core::net::Ipv4Addr> for Ipv4Address { | |||
fn from(ip: core::net::Ipv4Addr) -> Self { | |||
impl From<StdIpv4Addr> for Ipv4Address { |
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.
nit: I guess this is a personal preference thing, but I don't think it's clearer to introduce type aliases for the core::net types. core::net::
isn't that long to write out, and is more explicit.
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 think either way it doesn't look quite "nice", so I dropped the aliasing again. There is no clear benefit.
uefi-raw/src/net.rs
Outdated
v4: Ipv4Address(ip_addr), | ||
} | ||
pub const fn new_v4(octets: [u8; 4]) -> Self { | ||
// Initialize all bytes to zero first. |
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.
Can you say more about why this is desired? Perhaps a specific example of code where it would be useful to have this property.
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.
My thought here was that:
- make code safer if someone accidentally use the
v6
variant afterwards - this initialization overhead is negligible
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 just thought about this again, and I think we should drop. This only made sense in my previous design, which we discarded.
uefi-raw/src/net.rs
Outdated
|
||
/// Returns a raw pointer to the IP address. | ||
#[must_use] | ||
pub const fn as_ptr(&self) -> *const Self { |
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 don't think a method is needed when it's just returning a pointer to self
, instead the caller can write &raw addr
/&raw mut addr
(those operators were stabilized in 1.82)
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.
You are right, it is not needed. UEFI protocols consuming *mut IpAddress
can also consume a &mut IpAddress
and Rust does the right thing behind the scenes.
uefi-raw/src/net.rs
Outdated
@@ -158,9 +278,10 @@ impl From<[u8; 6]> for MacAddress { | |||
} | |||
} | |||
|
|||
impl From<MacAddress> for [u8; 6] { |
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.
Was removing MacAddress -> [u8; 6]
intentional?
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.
good catch; rebase glitch
|
||
/// Tries to interpret the MAC address as normal 6-byte MAC address, as used | ||
/// in ethernet. | ||
pub fn try_into_ethernet_mac_addr(self) -> Result<[u8; 6], [u8; 32]> { |
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.
This seems to make an assumption about zero being invalid, but is that true? If so, let's point to relevant documentation. If not, let's drop this method.
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.
This originates from my naive assumption that triviality applies here. Why would one have a fully non-null initialized byte array that takes more bytes than just a normal MAC address?
IMHO, this is a common use-case that will simplify things. In most cases, people want to work with Ethernet Mac addresses and they need a convenient conversion for that.
What do you think?
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.
How about an infallible conversion? Something like fn truncate_to_ethernet_mac_addr(self) -> [u8; 6]
. That way, we avoid making any assumptions about the meaning (or lack thereof) of the last 26 bytes, and the name makes it clear that this is truncating the input.
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.
simple but elegant, I like it, thanks!
uefi-raw/src/net.rs
Outdated
/// IPv4 address. | ||
/// | ||
/// # Safety | ||
/// Callers must be sure that all underlying bytes were initialized. |
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.
nit: This is a bit ambiguous. "all underlying bytes" could be read as "all bytes in the type", but that's not required for ipv4. Maybe something like: "Callers must ensure that the v4
field is valid if is_ipv6
is false, and that the v6
field is valid if is_ipv6
is true"
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 like this much better, thanks!
uefi-raw/src/net.rs
Outdated
/// # Safety | ||
/// Callers must be sure that all underlying bytes were initialized. | ||
#[must_use] | ||
pub unsafe fn into_std_ip_addr(self, is_ipv6: bool) -> StdIpAddr { |
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.
nit: Should this be into_core_ip_addr
? I'm not sure if there's an existing ecosystem convention either way, just want to make clear to end users that this method works in a no_std
env.
3bfe5a2
to
69b3116
Compare
Thanks for your valuable input :) I think we are getting closer to merging the additional convenience. |
684c89f
to
6e6a2a0
Compare
6e6a2a0
to
f7a4f2d
Compare
- added missing conversions between core::net types and the uefi-raw net types - added missing conversions for "typical byte arrays"
f7a4f2d
to
a123f60
Compare
a123f60
to
8fc2097
Compare
dc25f62
to
8fc2097
Compare
This prepares my vision for #1575, a split-out from #1645.
This adds much more convenience to the network types in
uefi-raw
, better integrating them withcore::net
types but also "typical" workflows. This "higher-level" logic is still low-level enough that I think it is perfectly fine to keep it. It will also not hinder Rust-based UEFI implementations.Steps to Undraft
Checklist