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 private helper method in_network for Ipv{4,6}Addr #86969

Closed
wants to merge 1 commit into from

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Jul 8, 2021

As I proposed in #86634 (comment), this PR introduces a private helper method in_network to check if an IP address is in a network. Checking if address is in 224.0.0.0/4 would be written as:

address.in_network(&Ipv4Addr::new(224, 0, 0, 0), 4)

This improves readability and possibly performance (based on https://rust.godbolt.org/z/6KEGWcd5a) over the various other ways this is implemented right now:

self.octets()[0] == 192 && self.octets()[1] == 0 && self.octets()[2] == 0
self.octets()[0] == 198 && (self.octets()[1] & 0xfe) == 18
self.octets()[0] >= 224 && self.octets()[0] <= 239
matches!(self.octets(), [169, 254, ..])

I think it would be a good idea to eventually offer this kind of functionality publically, although with a different API. Maybe Rust could get dedicated Ipv{4,6}Network types like Python?

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added the A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` label Jul 8, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2021
@the8472
Copy link
Member

the8472 commented Jul 8, 2021

Wouldn't a full mask be more efficient? if (addr & mask) == reference that way some of the more unusual ranges which aren't prefix-based could also be expressed. There are plans to change the IPv4 storage to u32 so this could boil down to two instructions, same for ipv6 but as a u128 so that shouldn't be more than a handful either, especially with simd operations.

By that I mean the ability to specify a mask, not just a prefix.

@CDirkx
Copy link
Contributor Author

CDirkx commented Jul 8, 2021

What would be non-prefix based ranges? Currently the method is only intended to be used as a helper for the implementation of the other methods (is_multicast etc. which are all prefix based). An eventually public API could indeed be more flexible.

@the8472
Copy link
Member

the8472 commented Jul 8, 2021

I think this check from another PR of yours could be expressed as a mask with a gap

(matches!(self.segments(), [0x2001, b, 0, 0, _, _, _, _] if b < 0x200)

@CDirkx
Copy link
Contributor Author

CDirkx commented Jul 8, 2021

Ah from #86634. That code should check if the address is in 2001::/23 however, so still a prefix (I'll double check to make sure it is correct). Code like that is exactly why I want to introduce this helper method, since it is hard to see the mapping.

Edit: the code is incorrect, as written it would indeed be a mask with a gap. It should be (matches!(self.segments(), [0x2001, b, _, _, _, _, _, _] if b < 0x200) instead. I fixed it in the other PR, thanks!

@joshtriplett
Copy link
Member

I like the concept of this, but if we're going to have functionality like this, I think we should have a dedicated type for a network, with ways to construct a network with either address and mask or address and number of bits.

@CDirkx
Copy link
Contributor Author

CDirkx commented Jul 8, 2021

I can additionally work on a public API version of this functionality. However I see this PR more as a refactoring of all the is_ methods to improve the readability and correctness (as shown by the mistake in #86969 (comment)) and think the private helper is still valuable on its own for now.

@CDirkx
Copy link
Contributor Author

CDirkx commented Jul 9, 2021

I opened #86992 to introduce public types representing IP prefixes.

@faern
Copy link
Contributor

faern commented Jul 19, 2021

address.in_network(&Ipv4Addr::new(224, 0, 0, 0), 4)

You can do exactly this with Ipv4Network::contains. Why does it need to be in std? If any functionality on IP network computation is missing we can probably add it to ipnetwork.

@bors
Copy link
Contributor

bors commented Aug 2, 2021

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

@camelid camelid 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 Aug 20, 2021
@inquisitivecrystal inquisitivecrystal added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 24, 2021
@JohnCSimon
Copy link
Member

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

@JohnCSimon
Copy link
Member

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

@rustbot label: +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot added 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 Sep 28, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@CDirkx I'm closing this as inactive, please feel free to reopen when you're ready to continue.

@rustbot label: S-inactive

@JohnCSimon JohnCSimon closed this Oct 19, 2021
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Oct 19, 2021
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 Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants