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

RFC: Move `std::net` types into `core::net`. #2832

Open
wants to merge 2 commits into
base: master
from

Conversation

@reitermarkus
Copy link

reitermarkus commented Dec 6, 2019

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

Rendered

@reitermarkus reitermarkus force-pushed the reitermarkus:core-net-types branch from 565c02d to c473501 Dec 6, 2019
@Nemo157

This comment has been minimized.

Copy link
Contributor

Nemo157 commented Dec 6, 2019

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

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Dec 6, 2019
…mulacrum

Remove boxed closures in address parser.

Simplify address parser by removing unnecessary boxed closures.

Also relevant for rust-lang/rfcs#2832.
@joshtriplett

This comment has been minimized.

Copy link
Member

joshtriplett commented Dec 6, 2019

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

This comment has been minimized.

Copy link
Author

reitermarkus commented Dec 7, 2019

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

This comment has been minimized.

Copy link
Author

reitermarkus commented Dec 7, 2019

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

This comment has been minimized.

Copy link
Member

joshtriplett commented Dec 7, 2019

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

@reitermarkus

This comment has been minimized.

Copy link
Author

reitermarkus commented Dec 9, 2019

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.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives

- Given the size of IP addresses is well defined by IETF RFCs, there is no

This comment has been minimized.

Copy link
@Manishearth

Manishearth Dec 9, 2019

Member

Alternative: move them into another library, like liballoc, that can be used without pulling in std.

This comment has been minimized.

Copy link
@reitermarkus

reitermarkus Dec 12, 2019

Author

Can you go into more detail about what benefits that alternative would bring?

This comment has been minimized.

Copy link
@tarcieri

tarcieri Dec 12, 2019

Is the a way for microcontroller users to use something in liballoc without defining a #[global_allocator]?

This comment has been minimized.

Copy link
@Lokathor

Lokathor Dec 12, 2019

Currently, only on Nightly.

This comment has been minimized.

Copy link
@bstrie

bstrie Dec 13, 2019

Contributor

I interpreted Manish's original comment as suggesting to split these out into a new library, like was done for liballoc, rather than suggesting to literally put them in liballoc. I suppose both approaches could be considered alternatives.

@JelteF

This comment has been minimized.

Copy link

JelteF commented Dec 13, 2019

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.

@reitermarkus reitermarkus changed the title RFC: Move `std::net` types into `core:.net`. RFC: Move `std::net` types into `core::net`. Dec 13, 2019
@dtolnay dtolnay added the T-libs label Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.