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

Jump to bottom

Open

phip1611 wants to merge 5 commits into main

from core-net-v2

Open

uefi-raw: improve convenience of net types #1699

phip1611 wants to merge 5 commits into main from core-net-v2

+252 −13

Conversation

phip1611

Copy link

Member

@phip1611 phip1611 commented Jun 15, 2025 •

edited

Loading

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 with core::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

merge and rebase onto uefi-raw: move types to net module #1747
merge and rebase onto uefi-raw: various small net improvements #1748

Checklist

Sensible git history (for example, squash "typo" or "fix" commits). See the Rewriting History guide for help.
Update the changelog (if necessary)

@phip1611 phip1611 requested a review from nicholasbishop

June 15, 2025 19:25

@phip1611 phip1611 force-pushed the core-net-v2 branch 3 times, most recently from 2b8f102 to 85838ce Compare

June 15, 2025 19:36

nicholasbishop

nicholasbishop reviewed

Jun 16, 2025

View reviewed changes

uefi-raw/CHANGELOG.md Outdated Show resolved Hide resolved

@phip1611 phip1611 marked this pull request as draft

July 9, 2025 10:53

@phip1611 phip1611 changed the title ~~(削除) uefi-raw: move types to new net module, improve convenience (削除ここまで)~~ (追記) [WIP] uefi-raw: move types to new net module, improve convenience (追記ここまで)

Jul 9, 2025

@phip1611

Copy link

Member Author

phip1611 commented Jul 9, 2025 •

edited

Loading

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

@phip1611 phip1611 marked this pull request as ready for review

August 15, 2025 13:32

@phip1611 phip1611 force-pushed the core-net-v2 branch from 85838ce to 1b7a7be Compare

August 15, 2025 13:38

@phip1611 phip1611 mentioned this pull request

Aug 15, 2025

cargo xtask improvements #1741

Closed

2 tasks

@phip1611 phip1611 force-pushed the core-net-v2 branch from 1b7a7be to 2ecc76c Compare

August 15, 2025 13:53

@phip1611 phip1611 changed the title ~~(削除) [WIP] uefi-raw: move types to new net module, improve convenience (削除ここまで)~~ (追記) uefi-raw: move types to new net module, improve convenience (追記ここまで)

Aug 16, 2025

@phip1611 phip1611 force-pushed the core-net-v2 branch from 2ecc76c to a76f20e Compare

August 16, 2025 11:40

@phip1611 phip1611 requested a review from nicholasbishop

August 16, 2025 11:42

@phip1611 phip1611 force-pushed the core-net-v2 branch from a76f20e to 1f7cd1b Compare

August 16, 2025 11:43

@phip1611

Copy link

Member Author

phip1611 commented Aug 17, 2025

I'm unsure how I could fix the rustdoc error best. I want the documentation of the net module to be publicly available (it is a clear value-add), but earlier you suggested exporting its exports in a flat way, i.e., directly from the top-level.

However: making the module public allows to import the same types from uefi_raw::Type and uefi_raw::net::Type, which is not ideal. Perhaps we should export all net::* types also publicly?

@nicholasbishop

Copy link

Member

nicholasbishop commented Aug 18, 2025

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 core::net types and uefi_raw net type". E.g. for Ipv4Address it could be something like:

/// An IPv4 internet protocol address.
///
/// This type can be converted to and from [`core::net::Ipv4Addr`] using the [`From`] trait.

@phip1611 phip1611 force-pushed the core-net-v2 branch 2 times, most recently from c2d44b0 to 24d5443 Compare

August 19, 2025 08:30

@phip1611

Copy link

Member Author

phip1611 commented Aug 19, 2025

What if we removed the module doc and instead added this information to the individual types?

Yeah, good idea, why not :) What do you think about the latest state?

@phip1611 phip1611 force-pushed the core-net-v2 branch 8 times, most recently from 411d16e to f2b5198 Compare

August 19, 2025 08:58

@nicholasbishop

Copy link

Member

nicholasbishop commented Aug 21, 2025

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.)

@phip1611 phip1611 mentioned this pull request

Aug 22, 2025

uefi-raw: move types to net module #1747

Merged

2 tasks

@phip1611 phip1611 marked this pull request as draft

August 22, 2025 07:41

@phip1611 phip1611 force-pushed the core-net-v2 branch 2 times, most recently from aa1b362 to 4261a10 Compare

August 23, 2025 12:52

Aug 23, 2025

@phip1611 phip1611 mentioned this pull request

Aug 23, 2025

uefi-raw: various small net improvements #1748

Merged

2 tasks

@phip1611 phip1611 self-assigned this

Aug 23, 2025

@phip1611 phip1611 force-pushed the core-net-v2 branch from 4261a10 to 0d4fe50 Compare

August 25, 2025 15:56

@phip1611 phip1611 changed the title ~~(削除) [WIP] uefi-raw: move types to new net module, improve convenience (削除ここまで)~~ (追記) uefi-raw: improve convenience of net types (追記ここまで)

Aug 25, 2025

@phip1611 phip1611 marked this pull request as ready for review

August 25, 2025 15:57

@phip1611

Copy link

Member Author

phip1611 commented Aug 25, 2025

should be much easier to review now

@phip1611 phip1611 force-pushed the core-net-v2 branch from 0d4fe50 to 3bfe5a2 Compare

August 25, 2025 16:03

nicholasbishop

nicholasbishop reviewed

Aug 26, 2025

View reviewed changes

uefi-raw/src/net.rs Outdated

impl IpAddress {

/// Construct a new zeroed address.

#[must_use]

pub const fn new_zeroed() -> Self {

Copy link

Member

@nicholasbishop nicholasbishop Aug 25, 2025

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] };

Copy link

Member Author

@phip1611 phip1611 Sep 7, 2025

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

impl From<core::net::Ipv4Addr> for Ipv4Address {

fn from(ip: core::net::Ipv4Addr) -> Self {

impl From<StdIpv4Addr> for Ipv4Address {

Copy link

Member

@nicholasbishop nicholasbishop Aug 26, 2025

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.

Copy link

Member Author

@phip1611 phip1611 Sep 16, 2025 •

edited

Loading

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.

Copy link

Member

@nicholasbishop nicholasbishop Aug 26, 2025

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.

Copy link

Member Author

@phip1611 phip1611 Sep 7, 2025

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

Copy link

Member Author

@phip1611 phip1611 Sep 7, 2025 •

edited

Loading

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 {

Copy link

Member

@nicholasbishop nicholasbishop Aug 26, 2025

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)

Copy link

Member Author

@phip1611 phip1611 Sep 7, 2025 •

edited

Loading

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

}

impl From<MacAddress> for [u8; 6] {

Copy link

Member

@nicholasbishop nicholasbishop Aug 26, 2025

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?

Copy link

Member Author

@phip1611 phip1611 Sep 7, 2025

Choose a reason for hiding this comment

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

good catch; rebase glitch

uefi-raw/src/net.rs

/// 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]> {

Copy link

Member

@nicholasbishop nicholasbishop Aug 26, 2025

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.

Copy link

Member Author

@phip1611 phip1611 Sep 7, 2025

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?

Copy link

Member

@nicholasbishop nicholasbishop Sep 17, 2025

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.

Copy link

Member Author

@phip1611 phip1611 Sep 17, 2025

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.

Copy link

Member

@nicholasbishop nicholasbishop Aug 26, 2025

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"

Copy link

Member Author

@phip1611 phip1611 Sep 7, 2025

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 {

Copy link

Member

@nicholasbishop nicholasbishop Aug 26, 2025

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.

@phip1611 phip1611 force-pushed the core-net-v2 branch from 3bfe5a2 to 69b3116 Compare

September 7, 2025 14:47

@phip1611

Copy link

Member Author

phip1611 commented Sep 7, 2025

Thanks for your valuable input :) I think we are getting closer to merging the additional convenience.

@phip1611 phip1611 requested a review from nicholasbishop

September 7, 2025 14:48

@phip1611 phip1611 force-pushed the core-net-v2 branch 3 times, most recently from 684c89f to 6e6a2a0 Compare

September 7, 2025 15:02

@phip1611 phip1611 force-pushed the core-net-v2 branch from 6e6a2a0 to f7a4f2d Compare

September 16, 2025 05:44

phip1611 added 2 commits

September 16, 2025 07:45

@phip1611


 uefi-raw: small code improvements for IpAddress

ff424b5

@phip1611


 uefi-raw: add convenient From impls + integration with core::net

9fa22d9

- added missing conversions between core::net types and the uefi-raw
 net types
- added missing conversions for "typical byte arrays"

@phip1611 phip1611 force-pushed the core-net-v2 branch from f7a4f2d to a123f60 Compare

September 16, 2025 05:45

phip1611 added 2 commits

September 16, 2025 07:48

@phip1611


 uefi-raw: add convenient into_* helpers

a984cca

@phip1611


 uefi-raw: add unit test for typical high-level net API usage

8fc2097

@phip1611 phip1611 force-pushed the core-net-v2 branch from a123f60 to 8fc2097 Compare

September 16, 2025 05:48

@phip1611 phip1611 mentioned this pull request

Jul 6, 2025

uefi: release 0.36 #1655

Open

8 tasks

@phip1611 phip1611 force-pushed the core-net-v2 branch from dc25f62 to 8fc2097 Compare

September 16, 2025 06:33

@phip1611


 uefi-raw: implement Display for Ipv4Address and Ipv6Address

cede821

@phip1611 phip1611 mentioned this pull request

Sep 16, 2025

[WIP] uefi: use core::net types in public API, remove net type duplicates, use uefi-raw's net types #1645

Draft

3 tasks

Labels

None yet

2 participants

@phip1611 @nicholasbishop

Uh oh!

uefi-raw: improve convenience of net types #1699

Are you sure you want to change the base?

uefi-raw: improve convenience of net types #1699

Conversation

@phip1611 phip1611 commented Jun 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps to Undraft

Checklist

Uh oh!

Uh oh!

phip1611 commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phip1611 commented Aug 17, 2025

Uh oh!

nicholasbishop commented Aug 18, 2025

Uh oh!

phip1611 commented Aug 19, 2025

Uh oh!

nicholasbishop commented Aug 21, 2025

Uh oh!

phip1611 commented Aug 25, 2025

Uh oh!

@nicholasbishop nicholasbishop Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

@phip1611 phip1611 Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

@nicholasbishop nicholasbishop Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

@phip1611 phip1611 Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@nicholasbishop nicholasbishop Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

@phip1611 phip1611 Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

@phip1611 phip1611 Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@nicholasbishop nicholasbishop Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

@phip1611 phip1611 Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@nicholasbishop nicholasbishop Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

@phip1611 phip1611 Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

@nicholasbishop nicholasbishop Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

@phip1611 phip1611 Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

@nicholasbishop nicholasbishop Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

@phip1611 phip1611 Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

@nicholasbishop nicholasbishop Aug 26, 2025

Choose a reason for hiding this comment

Uh oh!

@phip1611 phip1611 Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

@nicholasbishop nicholasbishop Aug 26, 2025

Choose a reason for hiding this comment

@phip1611 phip1611 commented Jun 15, 2025 •

edited

Loading

phip1611 commented Jul 9, 2025 •

edited

Loading

@phip1611 phip1611 Sep 16, 2025 •

edited

Loading

@phip1611 phip1611 Sep 7, 2025 •

edited

Loading

@phip1611 phip1611 Sep 7, 2025 •

edited

Loading