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

Add octet-oriented interface to std::net::Ipv6Addr #1498

Merged
merged 3 commits into from
Mar 17, 2016

Conversation

AGWA
Copy link
Contributor

@AGWA AGWA commented Feb 13, 2016

Summary: Add constructor and getter functions to std::net::Ipv6Addr that are oriented around octets.

Rendered

@SimonSapin
Copy link
Contributor

I’d call the methods bytes and from_bytes rather than octets and from_octets.

As far as I understand, in English, the word octet is used to be explicit about having 8 bits in contexts where a byte may have a different length. In the Rust standard library however, there is widespread precedent of the word "bytes" being used to designate [u8], and u8 by definition has exactly 8 bits.

Other than this naming bikeshed, +1 on the feature.

@sfackler
Copy link
Member

I agree with @SimonSapin. It's pretty annoying to work with IPv6 addresses right now in comparison to IPv4 ones: https://github.com/sfackler/rust-socks/blob/master/src/v5.rs#L12-L29

@sfackler sfackler self-assigned this Feb 15, 2016
@AGWA
Copy link
Contributor Author

AGWA commented Feb 15, 2016

@SimonSapin I see your point about "bytes," but note that std::net::Ipv4Addr's corresponding function is already called octets(). I think the inconsistency with Ipv4Addr would be more awkward than the inconsistency with the rest of the standard library, although I don't really mind one way or the other.

@SimonSapin
Copy link
Contributor

Ah, ok. Given Ipv4Addr::octets, never mind my comment.

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Feb 18, 2016
@alexcrichton
Copy link
Member

🔔 This RFC is now entering its week-long final comment period 🔔

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Mar 3, 2016
@alexcrichton
Copy link
Member

I personally like the motivation for this RFC and I think we should definitely have these methods. Some nitpicks about the API I might have are:

  • The octets method here returns an internal reference, but on Ipv4Addr it returns the octets by value. We've unfortunately had a bad time in the past returning internal references to structures defined in libc, so I might recommend returning the array by value.
  • The from_octets method may not be too useful unfortunately as it's very difficult to get your hands on a &[u8; 16]. The Ipv4Addr method just takes 4 arguments, and maybe we could just have a 16 argument method for Ipv6Addr? It's somewhat unergonomic, but it is probably more easily callable than getting a &[u8; 16].
  • Another possible option would be a fallible conversion from &[u8] to Ipv6Addr (verifying the length is exactly 16), but that plays into the more general "generic fallible conversions" story, so we may want to hold off on that.

@sfackler
Copy link
Member

sfackler commented Mar 3, 2016

@alexcrichton on the contrary, for use cases that involve getting IP addresses out of a network protocol, it's very reasonable to get a [u8; 16], and better to work with:

let mut buf = [0; 16];
try!(reader.read_all(&mut buf));
let ip = Ipv6Addr::from_octets(&buf);

vs

let ip = Ipv6Addr::from_octets(try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()),
                               try!(reader.read_u8()));

@AGWA
Copy link
Contributor Author

AGWA commented Mar 3, 2016

Agreed with @sfackler - I have not had trouble getting a &[u8; 16] when writing network code, and it would be a real downer to have to pass 16 arguments.

@alexcrichton
Copy link
Member

Hm ok, if you end up having [u8; 16] lying around a lot, perhaps we could add this via impl From<[u8; 16]> for Ipv6Addr?

@sfackler
Copy link
Member

sfackler commented Mar 3, 2016

That'd work as well. I currently use the From<u32> impl for Ipv4Addr.

@alexcrichton
Copy link
Member

I suppose while we're at it we could also add From<[u8; 4]> for Ipv4Addr, but that's more optional to me I think.

@sfackler
Copy link
Member

sfackler commented Mar 3, 2016

It's a bit easier to get at a [u8; 4] than a u32 from a reader, so I'd be on board with adding that.

@tbu-
Copy link
Contributor

tbu- commented Mar 8, 2016

I think vital information is missing: What byte-order should get exposed: The host byte order (little endian most of the time) or network byte order (big endian)?

(If that's already in the RFC, then I can't find it and it should probably be made more visible.)

@AGWA
Copy link
Contributor Author

AGWA commented Mar 8, 2016

I think vital information is missing: What byte-order should get exposed: The host byte order (little endian most of the time) or network byte order (big endian)?

I'm afraid I don't understand your question - the interface exposes an array of 16 single bytes, so there isn't any question of byte order. If the interface exposed an array of 8 16-bit integers, or a single 128-bit integer (if Rust had such a type), then the byte order of those integers would need to be specified, but that's not the case here.

@SimonSapin
Copy link
Contributor

It should be the same order as Ipv6Addr::segments and Ipv6Addr::octets. Which order that is is not explicit in the docs, but assuming they match the order in the ::new() constructors it’s the same order as in the hex/decimal serialization.

@AGWA
Copy link
Contributor Author

AGWA commented Mar 8, 2016

@alexcrichton @sfackler I like your suggestions of adding an impl From<[u8; 16]> for Ipv6Addr and an impl From<[u8; 4]> for Ipv4Addr. Should I update the RFC accordingly?

I think I'll also change octets to not return an internal reference, given the bad experiences with returning internal references.

@alexcrichton
Copy link
Member

@AGWA yeah it's fine to just push an update to the RFC.

@tbu-
Copy link
Contributor

tbu- commented Mar 8, 2016

@AGWA It's interesting in which order the bytes are, for an address starting with fe80::, are the first two bytes you output [0xfe, 0x80] (big endian, "network byte order") or [0x80, 0xfe] (little endian)? Could you add that (whichever endian you want) to the RFC?

@SimonSapin
Copy link
Contributor

For an address starting with fe80::1::, big endian would be starting with [0xfe, 0x80, 0x00, 0x01], little-endian would be ending with [0x01, 0x00, 0x80, 0xfe].

Starting with [0x80, 0xfe, 0x01, 0x00] would be big-endian for pairs of bytes and little-endian with each pair, which makes no sense.

@tbu-
Copy link
Contributor

tbu- commented Mar 8, 2016

Sorry if it is clear in what order the bytes are, however it's not that clear to me. For me, an IPv6 address is composed of eight unsigned 16-bit integers. Now my question was whether these 16-bit integers are themselves in big-endian (as they're sent over the network) or little-endian order (how most hosts represent 16-bit integers).

@SimonSapin
Copy link
Contributor

For me, an IPv6 address is composed of eight unsigned 16-bit integers.

https://tools.ietf.org/html/rfc2460 only talks about IPv6 addresses being made of 128 bits. The grouping by 16 bits only exists for the purpose of hex serialization.

@tbu-
Copy link
Contributor

tbu- commented Mar 8, 2016

Thank you, that clears it up!

AGWA added 2 commits March 9, 2016 21:01
* `octets` no longer returns a reference, due to bad experiences
  with returning internal references to libc structures in the past.

* Replace `from_octets` with an implementation of `From<[u8; 16]>`
  for `Ipv6Addr`.

* For consistency, also implement `From<[u8; 4]>` for `Ipv4Addr`.
@AGWA
Copy link
Contributor Author

AGWA commented Mar 10, 2016

I just pushed an update to the RFC that reflects the feedback so far.

@alexcrichton
Copy link
Member

Looks good to me, thanks @AGWA!

@alexcrichton
Copy link
Member

The libs team discussed this RFC during triage yesterday and the conclusion was to merge. It was brought up that it's relatively odd to consume fixed-size arrays in the standard library, but as pointed out by @sfackler this is tenable even today in some situations. It was also brought up that we would likely inevitably want this functionality regardless as it's just an extra implementation of From.

Tracking issue: rust-lang/rust#32313

@alexcrichton
Copy link
Member

Ah and of course, thanks again for the RFC @AGWA!

@alexcrichton alexcrichton merged commit 0fede96 into rust-lang:master Mar 17, 2016
bors added a commit to rust-lang/rust that referenced this pull request Mar 19, 2016
Add an impl for From trait

Converts a u8 slice to a Ipv4Addr
More discussion on this here: rust-lang/rfcs#1498 (comment)
@Centril Centril added the A-net Proposals relating to networking. label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-net Proposals relating to networking. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants