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

Introduce unstable Ipv{4,6}AddrPrefix #86992

Closed
wants to merge 4 commits into from
Closed

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Jul 9, 2021

This PR introduces types representing IP address prefixes such as 192.0.2.0/24, as suggested in #86969 (comment).

Feature gate: #![feature(ip_prefix)]
Tracking issue: #86991

Overview

Ipv4AddrPrefix

#[derive(Copy, PartialEq, Eq, Clone, Hash)]
pub struct Ipv4AddrPrefix {
    address_raw: u32,
    len: u8,
}

impl Ipv4AddrPrefix {
    pub const fn new(address: Ipv4AddrPrefix, len: u32) -> Result<Ipv4AddrPrefix, InvalidPrefixError>;
    pub const fn address(&self) -> Ipv4Addr;
    pub const fn len(&self) -> u32;
    pub const fn contains(&self, address: &Ipv4Addr) -> bool;
}

impl Debug for Ipv4AddrPrefix  {}
impl Display for Ipv4AddrPrefix  {}
impl From<Ipv4Addr> for Ipv4AddrPrefix  {}
impl FromStr for Ipv4AddrPrefix  {}

Ipv6AddrPrefix

#[derive(Copy, PartialEq, Eq, Clone, Hash)]
pub struct Ipv6AddrPrefix {
    address_raw: u128,
    len: u8,
}

impl Ipv6AddrPrefix {
    pub const fn new(address: Ipv6AddrPrefix, len: u32) -> Result<Ipv6AddrPrefix, InvalidPrefixError>;
    pub const fn address(&self) -> Ipv6Addr;
    pub const fn len(&self) -> u32;
    pub const fn contains(&self, address: &Ipv6Addr) -> bool;
}

impl Debug for Ipv6AddrPrefix  {}
impl Display for Ipv6AddrPrefix  {}
impl From<Ipv6Addr> for Ipv6AddrPrefix  {}
impl FromStr for Ipv6AddrPrefix  {}

This PR doesn't make any changes yet to the Ipv{4,6}Addr types themselves.

Unresolved Questions

Is the naming clear? Is this the correct abstraction?

@rustbot rustbot added A-io Area: std::io, std::fs, std::net and std::path T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 9, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 9, 2021
/// [IPv4 address]: Ipv4Addr
#[derive(Copy, PartialEq, Eq, Clone, Hash)]
#[unstable(feature = "ip_prefix", issue = "86991")]
pub struct Ipv4AddrPrefix {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python uses the name IPv6Network, but I chose the less generic name Ipv4AddrPrefix.

#[unstable(feature = "ip_prefix", issue = "86991")]
pub struct Ipv4AddrPrefix {
address: Ipv4Addr,
len: u8,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

len stored as an u8 to reduce size, however new and len use/return a u32 (chosen that way to have the same type as {u32, u128}::BITS).

Copy link
Member

Choose a reason for hiding this comment

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

Well, it reduces size but not alignment. So if you put into an array there wouldn't be any savings.

/// ```
#[unstable(feature = "ip_prefix", issue = "86991")]
#[inline]
pub const fn new(address: Ipv4Addr, len: u32) -> Result<Ipv4AddrPrefix, InvalidPrefixError> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Constructor is fallible, unlike the IpvAddr and SocketAddr constructors, because the prefix length can not be longer than there are bits.

/// Ipv4AddrPrefix::new(Ipv4Addr::new(192, 0, 2, 0), 35).unwrap_err();
/// ```
#[unstable(feature = "ip_prefix", issue = "86991")]
#[inline]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure which methods should or shouldn't be inlined.

@the8472
Copy link
Member

the8472 commented Jul 9, 2021

As mentioned in the other PR, having a mask instead of a prefix would be more powerful. The prefix could still be passed as a convenience constructor.
One example case where this is useful is dynamic ipv6 prefix delegation where each machine still has a fixed suffix. So you can identify a machine by matching a suffix instead of a prefix.

#[derive(Copy, PartialEq, Eq, Clone, Hash)]
#[unstable(feature = "ip_prefix", issue = "86991")]
pub struct Ipv4AddrPrefix {
address_raw: u32,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed from Ipv4Addr to a u32 to avoid storing potential extra fields of c::in_addr. Same thing for Ipv6AddrPrefix. This could be reverted if #78802 ever gets merged.

@CDirkx
Copy link
Contributor Author

CDirkx commented Jul 9, 2021

I agree that the ability to do computations with address masks should be added, like you said comparing addresses and using a subnet mask (255.255.255.0) or wildcard mask (0.0.0.255) would be very useful. However I don't think that functionality should be included in these IpAddrPrefix types, but instead as methods on the IpAddr types (or a separate IpAddrMask type, see CDirkx@2e17cd8 for what I think that could look like).

An address + prefix (1.2.3.0/24) to describe a range is a fundamental concept enough to warrant its own type in my opinion, in a way address + arbitrary mask isn't. Python's IPNetwork classes also only represent address + prefix, but I can look into what other languages are doing regarding prefixes, masks etc.

@the8472
Copy link
Member

the8472 commented Jul 9, 2021

An address + prefix (1.2.3.0/24) to describe a range is a fundamental concept enough to warrant its own type in my opinion, in a way address + arbitrary mask isn't.

But a mask can everything a prefix can, so why not implement the more powerful concept?

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2021
@bors
Copy link
Contributor

bors commented Jul 28, 2021

☔ The latest upstream changes (presumably #85769) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@CDirkx can you please address the merge conflict?

@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Sep 6, 2021
@JohnCSimon
Copy link
Member

Triage:
@the8472 - seems that this PR has been inactive for over two months. I'm closing this as inactive. Please feel free to reopen if you want to continue on this. Thank you.

@JohnCSimon JohnCSimon closed this Sep 6, 2021
@the8472
Copy link
Member

the8472 commented Sep 6, 2021

@JohnCSimon I'm not the PR author.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: std::io, std::fs, std::net and std::path S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants