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

Modified return type for ipaddr #2151

Merged

Conversation

carlosb1
Copy link
Contributor

@carlosb1 carlosb1 commented Oct 3, 2023

What does this PR do

Fixes #2003

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

@carlosb1 carlosb1 changed the title WIP: Modified return type for ipaddr Modified return type for ipaddr Oct 3, 2023
@carlosb1 carlosb1 marked this pull request as ready for review October 3, 2023 17:21
@@ -1006,8 +1006,8 @@ pub struct SockaddrIn(libc::sockaddr_in);
impl SockaddrIn {
/// Returns the IP address associated with this socket address, in native
/// endian.
pub const fn ip(&self) -> libc::in_addr_t {
u32::from_be(self.0.sin_addr.s_addr)
pub fn ip(&self) -> net::Ipv4Addr {
Copy link
Member

@SteveLauC SteveLauC Oct 4, 2023

Choose a reason for hiding this comment

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

It seems that we can also retain the const here if we choose the following impl:

    pub const fn ip(&self) -> Ipv4Addr {
        let bytes = self.0.sin_addr.s_addr.to_ne_bytes();
        let (a, b, c, d) = (bytes[0], bytes[1], bytes[2], bytes[3]);
        Ipv4Addr::new(a, b, c, d)
    }

@SteveLauC
Copy link
Member

And for SockaddrIn6::ip(), we can also make it const with a similar impl:

    pub const fn ip(&self) -> Ipv6Addr {
        let bytes = self.0.sin6_addr.s6_addr;
        let [a, b, c, d, e, f, g, h] =
            unsafe { transmute::<_, [u16; 8]>(bytes) };
        
        // Ipv6Addr::new() takes segments in native endian, convert them here
        let [a, b, c, d, e, f, g, h] = [
            u16::from_be(a),
            u16::from_be(b),
            u16::from_be(c),
            u16::from_be(d),
            u16::from_be(e),
            u16::from_be(f),
            u16::from_be(g),
            u16::from_be(h),
        ];

        Ipv6Addr::new(a, b, c, d, e, f, g, h)
    }

@SteveLauC
Copy link
Member

cc @asomers, these two types were added in #1684

@carlosb1 carlosb1 force-pushed the feat/2003-fix-return-correct-ipaddr branch from f8213aa to 870459f Compare October 4, 2023 10:02
@asomers
Copy link
Member

asomers commented Oct 4, 2023

And for SockaddrIn6::ip(), we can also make it const with a similar impl:

    pub const fn ip(&self) -> Ipv6Addr {
        let bytes = self.0.sin6_addr.s6_addr;
        let [a, b, c, d, e, f, g, h] =
            unsafe { transmute::<_, [u16; 8]>(bytes) };
        
        // Ipv6Addr::new() takes segments in native endian, convert them here
        let [a, b, c, d, e, f, g, h] = [
            u16::from_be(a),
            u16::from_be(b),
            u16::from_be(c),
            u16::from_be(d),
            u16::from_be(e),
            u16::from_be(f),
            u16::from_be(g),
            u16::from_be(h),
        ];

        Ipv6Addr::new(a, b, c, d, e, f, g, h)
    }

mem::transmute should be a last resort. I don't think we need it here. We could do the conversion more verbosely just by addressing the individual octets directly.

@carlosb1
Copy link
Contributor Author

And for SockaddrIn6::ip(), we can also make it const with a similar impl:

    pub const fn ip(&self) -> Ipv6Addr {
        let bytes = self.0.sin6_addr.s6_addr;
        let [a, b, c, d, e, f, g, h] =
            unsafe { transmute::<_, [u16; 8]>(bytes) };
        
        // Ipv6Addr::new() takes segments in native endian, convert them here
        let [a, b, c, d, e, f, g, h] = [
            u16::from_be(a),
            u16::from_be(b),
            u16::from_be(c),
            u16::from_be(d),
            u16::from_be(e),
            u16::from_be(f),
            u16::from_be(g),
            u16::from_be(h),
        ];

        Ipv6Addr::new(a, b, c, d, e, f, g, h)
    }

mem::transmute should be a last resort. I don't think we need it here. We could do the conversion more verbosely just by addressing the individual octets directly.

I reimplemented it

@SteveLauC
Copy link
Member

LGTM! Sorry for the late review, thanks for doing this!

@SteveLauC SteveLauC added this pull request to the merge queue Nov 5, 2023
Merged via the queue into nix-rust:master with commit ed0f7ff Nov 5, 2023
35 checks passed
@SteveLauC
Copy link
Member

I forgot to mention that we have changed the CHANGELOG mode in #2149, let me fix this later

SteveLauC added a commit to SteveLauC/nix that referenced this pull request Nov 5, 2023
@SteveLauC SteveLauC mentioned this pull request Nov 5, 2023
3 tasks
SteveLauC added a commit that referenced this pull request Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Illogical return value for SockaddrIn::ip() when compared with SockaddrIn6::ip()
3 participants