RFC: Move `std::net::IpAddr` types into `core::net`.

[reitermarkus]

Make the IpAddr, Ipv4Addr and Ipv6Addr types available in no_std contexts by moving them into a core::net module.

Rendered

[Nemo157]

Given that most IP based networking is done over UDP/TCP, even for IoT devices, I think it would make sense to move the SocketAddr types at the same time. This also makes the string parser easier to move (in my branch I initially only intended to move the IP types, but I think I ran into issues with interdependencies between the parsers which would have needed a larger rewrite of them).

[joshtriplett]

This seems entirely reasonable to me.

One thing the RFC should discuss, though: will all platforms that use a given target have the same interpretation of the type? Is the inner storage expected to be in network-endian byte order or native-endian byte order?

[reitermarkus]

The inner octets should always be stored in network/big-endian order. The current Ipv4Addr implementation already ensures this by constructing the libc type with u32::to_be and the libc type for Ipv6Addr already uses a [u8; 16] internally on all platforms.

[reitermarkus]

I agree that it makes sense to move the SocketAddr types at the same time.

Now there are two options on how to represent them:

The Rust Way:

pub struct SocketAddrV4 {
    ip: Ipv4Addr,
    port: u16,
}

pub struct SocketAddrV6 {
    ip: Ipv6Addr,
    port: u16,
    flowinfo: u32,
    scope_id: u32,
}

This makes these types as portable as possible but we have to construct the corresponding libc type when interfacing with libc.

The C Way:

pub struct SocketAddrV4 {
    __padding: [u8; 2],
    port: u16,
    ip: Ipv4Addr,
    __zero: [u8; 8]
}

pub struct SocketAddrV6 {
    __padding: [u8; 2],
    port: u16,
    flowinfo: u32,
    ip: Ipv6Addr,
    scope_id: u32,
}

According to RFC 2553, __padding can either be

len: u8,
family: u8,

or just

family: u16,

and __zero should be [u8; 8], but there are some platforms that don't have it at all and some that use [u8; 24].

Note that in the current implementation we never set len, even if the libc type has that field.

This way we can cast directly to the libc type, however there are a few drawbacks:

• family must be still be set to libc::AF_INET/libc::AF_INET6 before interfacing with libc which would mutate an immutable reference. Also the type itself already encodes whether it is IPv4 or IPv6.
• Size of __zero conflicts with the libc type on some platforms.

[joshtriplett]

@reitermarkus If the socketaddr types aren't identical across platforms, I don't think we want them in core.

[reitermarkus]

They are definitely identical with the pure Rust representation. Also SocketAddrV6 is identical in the C representation on all platforms.

Now for the C representation of SocketAddrV4, after looking further into the __zero issue, I found that all targets using no such field at all or [u8; 0] either simply don't support sockets at all or are using a different socket API .

RFC 2553 also states the following:

Each protocol-specific data structure is designed so it can be cast into a protocol-independent data structure — the sockaddr structure.

So even if there were platforms actually using [u8; 0], it would still be safe to cast to the libc type.

RFC 2553 also mentions:

The sockaddr_in structure is the protocol-specific data structure for IPv4. This data structure actually includes 8-octets of unused space, …

That means that __zero: [u8; 8] is definitely correct according to the RFC. And in fact all platforms except Haiku define it this way.

That means we will have to provide a shim for Haiku, since it will try to zero a [u8; 24].

This way libcore will be in spec with the platform-agnostic RFC while libstd will provide the platform-specific shim for Haiku.

[JelteF]

Small nitpick, both the title of the PR and the commit message say: core::.net, not core::net

This would be a funny module name, given that .NET Core is the name of the .NET compiler. But I think it's both a bit too confusing, since std::.net does not exist, and it's actually a typo because the rfc itself only mentions core::net

So I suggest changing it to core::net in the PR and commit title.

[scottmcm]

:+1: the IpAddr-Ipv4Addr-and-Ipv6Addr version of this. I like that the version-specific ones have well-defined byte orders and sizes, so there are no real surpises or choices here.

Harder questions like SocketAddr* can always be a future RFC.

(P.S. The title of this RFC surprised me -- it makes it seem like everything, not a subset.)

[reitermarkus]

it makes it seem like everything, not a subset

Well, the idea was to move most types, but I think scoping this RFC to just IP address types might be best to get things moving.

[reitermarkus]

Any more concerns or suggestions?

[ryankurte]

it would be super great to have net::IpAddr* moved to core, it's a definite improvement, and it makes sense not to block on things that require more discussion...

however, without the net::SocketAddr* types we still have all the overhead of managing alternate SockAddr types for no_std applications, or finding types to substitute if we support both, i'd love to see both of em in core so we can remove this whole pain point.

[Manishearth]

Please stop.

I am not on the libs team, I don't know why you want my review here. If someone undoes an action you've taken you should at most ask why, not immediately do it again.

[reitermarkus]

@Manishearth, I'm sorry, that second request was a misclick. I added the alternative you suggested of moving these types into a crate other than core which is why I initially requested a review from you. Also, I don't seem to be able to request another review from anyone else.

[Manishearth]

It's up to the libs team to decide when to discuss and review this.

[ryankurte]

hey @rust-lang/libs folks, sorry if this is already in the queue but, have y'all had a chance to look at this yet?

i am on the edge of writing a std/no_std facade to manage this but, would rather spend the time fixing the actual problem if there's a path forwards ^_^

[Ericson2314]

I feel like it would be better to just have a crates.io create using core that std can also use, but I suppose core has gotten quite big already.

[Nemo157]

@Ericson2314 is there precedent of std publicly exposing types from other crates? As far as I'm aware all current crate usage is hidden behind new-types so you can't do something like let _: std::collections::HashMap = hashbrown::HashMap::new();. Interoperability between crates using this no-std supporting IpAddr type and crates using std::net::IpAddr is crucial.

If this isn't "core", then maybe there needs to be another public crate within the "standard libraries" alongside alloc which includes other OS-independent data types. (EDIT: And rereading the discussion, this was suggested as an alternative already 😁)

[Ericson2314]

Oh good.

[lygstate]

Oh, it's here, I didn't found it in rust issues

[madsmtm]

For those who don't know, the no-std-net crate exists to fill this gap until the RFC gets further.

[clarfonthey]

I very much agree with this RFC and also second the idea that we should add SocketAddr to libcore as well, since the socket is well-defined to always be a u16, and the ability to parse a string like 127.0.0.1:80 into SocketAddrV4([127, 0, 0, 1], 80) would be nice. That said, I understand entirely that it's desired to keep this outside the scope of the RFC.

[sunfishcode]

The main concern about adding socket addresses to core seems to be:

If the socketaddr types aren't identical across platforms, I don't think we want them in core.

I have an idea for how we could add socket address types to core, while preserving the important properties of core:

core already has types that aren't completely identical across platforms; eg. Duration has a platform-specific amount of padding. So, what if we add the socketaddr types to core, with platform-specific layouts, but encapsulated, similar to Duration, so that the public API remains platform-independent?

For FFI, what if we introduced a new core::os module, taking ideas from both core::arch and std::os? It could contain extension traits like core::os::unix::net::SocketAddrV4Ext, similar to std::os::unix::ffi::OsStrExt, which would provide methods to access the internal representation for doing FFI.

This would preserve the essential properties of core: it would continue to do no I/O itself, it'd continue to have a platform-independent public API outside of core::os and core::arch, and it'd continue to be trivial to compile on new platforms. A platform with no extension traits defined yet could always use a common platform-indepenent socketaddr layout because the layout wouldn't be observable.

Would something like this be within the spirit of core?

[Nemo157]

https://github.com/rust-lang/rust/pull/78802 is going to remove platform differences in the relevant types, so once that has been merged it seems like there should be no objections against include SocketAddr here.

[nacaclanga]

The main concern about adding socket addresses to core seems to be:

If the socketaddr types aren't identical across platforms, I don't think we want them in core.

I have an idea for how we could add socket address types to core, while preserving the important properties of core:

core already has types that aren't completely identical across platforms; eg. Duration has a platform-specific amount of padding. So, what if we add the socketaddr types to core, with platform-specific layouts, but encapsulated, similar to Duration, so that the public API remains platform-independent?

I think there is a difference here: Duration itself is plattform independend. Only functions that happen to return one like std::time::SystemTime::elapsed() (witch are not defined in core) are plattform dependend.

Would something like this be within the spirit of core?

As far as I know, code that is specific to the architecture can live in core. Code that is dependend on the specific plattform gets rejected. (See e.g. https://github.com/rust-lang/rust/issues/36193)

Edit: Primitive, but plattform dependend C types are now found to be ok to live in core. However this likely won't effect the position for more complex types. You should be aware of Nemo157's comment

[faern]

https://github.com/rust-lang/rust/pull/78802 has now been merged. So the discussion here can continue. There should now not be anything stopping us from moving IpAddr* and SocketAddr* into core as far as I know :tada:

Personally I'd love to see this RFC go through. But with the addition of SocketAddr, SocketAddrV4 and SocketAddrV6, which is currently not part of the RFC text, albeit discussed a lot above.

An experimental branch with this move can probably be prepared right away, to verify that this is possible and does not have any unknown drawbacks.

[reitermarkus]

Nice work, @faern!

[mati865]

@faern I have tried to create such branch but got stuck on write!(..) calls to mutable slice like this one. AFAIK they are not supported in core:

The error:

error[E0599]: the method `write_fmt` exists for mutable reference `&mut [u8]`, but its trait bounds were not satisfied
    --> library/core/src/macros/mod.rs:500:14
     |
498  | / macro_rules! write {
499  | |     ($dst:expr, $($arg:tt)*) => {
500  | |         $dst.write_fmt($crate::format_args!($($arg)*))
     | |              ^^^^^^^^^ method cannot be called on `&mut [u8]` due to unsatisfied trait bounds
501  | |     };
502  | | }
     | |_- in this expansion of `write!`
     |
    ::: library/core/src/fmt/mod.rs:187:28
     |
187  |       fn write_fmt(mut self: &mut Self, args: Arguments<'_>) -> Result {
     |                              --------- the method might not be found because of this arbitrary self type
     |
    ::: library/core/src/net/ip.rs:1775:13
     |
1775 |               write!(buf_slice, "{}", self).unwrap();
     |               ----------------------------- in this macro invocation
     |
note: trait bound `[u8]: Write` was not satisfied
    --> library/core/src/fmt/mod.rs:193:9
     |
193  | impl<W: Write + ?Sized> Write for &mut W {
     |         ^^^^^           -----     ------
     |         |
     |         unsatisfied trait bound introduced here

[MabezDev]

@faern I have tried to create such branch but got stuck on write!(..) calls to mutable slice like this one. AFAIK they are not supported in core:

The error:

error[E0599]: the method `write_fmt` exists for mutable reference `&mut [u8]`, but its trait bounds were not satisfied
    --> library/core/src/macros/mod.rs:500:14
     |
498  | / macro_rules! write {
499  | |     ($dst:expr, $($arg:tt)*) => {
500  | |         $dst.write_fmt($crate::format_args!($($arg)*))
     | |              ^^^^^^^^^ method cannot be called on `&mut [u8]` due to unsatisfied trait bounds
501  | |     };
502  | | }
     | |_- in this expansion of `write!`
     |
    ::: library/core/src/fmt/mod.rs:187:28
     |
187  |       fn write_fmt(mut self: &mut Self, args: Arguments<'_>) -> Result {
     |                              --------- the method might not be found because of this arbitrary self type
     |
    ::: library/core/src/net/ip.rs:1775:13
     |
1775 |               write!(buf_slice, "{}", self).unwrap();
     |               ----------------------------- in this macro invocation
     |
note: trait bound `[u8]: Write` was not satisfied
    --> library/core/src/fmt/mod.rs:193:9
     |
193  | impl<W: Write + ?Sized> Write for &mut W {
     |         ^^^^^           -----     ------
     |         |
     |         unsatisfied trait bound introduced here

You could create a wrapper new type around the buffer that implements core::fmt::Write. The write_str method would write the bytes directly into the buffer :).

[reitermarkus]

I have opened https://github.com/rust-lang/rust/pull/100625 to remove the dependency on std::io::Write.

[faern]

Does anyone have a branch ready with this change? To prove that it's possible. I think it's time to move ahead with this change. Would be great with these types in core already.