adds support for AF_IUCV sockets

[clayton615]

This is my first attempt to add IUCV support. This references issue #667 .

The way I've tried to restrict this to only platforms with IUCV headers (as far as I can tell, this is GNU Linux on x86_64 and s390x) is with the following:

#[cfg(all(
    feature = "iucv",
    any(target_arch = "x86_64", target_arch = "s390x"),
    target_env = "gnu"
))]

Let me know if that can be improved.

I've also added an is_iucv and as_socket_iucv, doing my best to model that off of the existing functions.

I've tested this on x86_64 Linux, and s390x Linux and it's all working as I hoped. I'm hoping that like libc you'll have a suite of tests to make sure I'm not introducing any issues in any platforms I'm not able test.

Scrutiny welcome, always looking to improve.

[clayton615]

Off to a good start. Clippy I can fix (doing so now), but I need some advice for the other two.

For the external types checking, does that mean I'm not allowed to return a libc::sockaddr_iucv? It was the easiest way I could think of to get to the user_id and name fields.

For 3ds, I'm surprised my cfg clause didn't prevent that, unless the runner is just busted?

[Thomasdezeeuw]

For the external types checking, does that mean I'm not allowed to return a libc::sockaddr_iucv? It was the easiest way I could think of to get to the user_id and name fields.

That's correct, we don't want to depend on any external libraries in our API otherwise we can change that dependency version (e.g. going from libc 0.2 to libc 1.0 would be a breaking change, even though the structure would likely not change).

For 3ds, I'm surprised my cfg clause didn't prevent that, unless the runner is just busted?

You can ignore that one, it's a problem with the standard library

[clayton615]

That's correct, we don't want to depend on any external libraries in our API otherwise we can change that dependency version (e.g. going from libc 0.2 to libc 1.0 would be a breaking change, even though the structure would likely not change).

Okay, that makes sense, but I'm less sure how to proceed now. Since there's no SockAddrIucv in the standard library to mirror SockAddrV4 and SockAddrV6 I can't just return one. I see two options:

1. Add a SockAddrIucv in socket2 and basically just wrap sockaddr_iucv
2. Instead of using a as_socket_iucv function, I could just add two functions like get_iucv_user and get_iucv_name
   Do you have a preference?

[Thomasdezeeuw]

Okay, that makes sense, but I'm less sure how to proceed now. Since there's no SockAddrIucv in the standard library to mirror SockAddrV4 and SockAddrV6 I can't just return one. I see two options:

1. Add a SockAddrIucv in socket2 and basically just wrap sockaddr_iucv
2. Instead of using a as_socket_iucv function, I could just add two functions like get_iucv_user and get_iucv_name
   Do you have a preference?

Have you looked at SockAddrStorage, specifically SockAddrStorage::view_as? It allows you to view a SockAddr (via SockAddr::as_storage) as any storage type. Could you try that to get the sockaddr_iucv address, see if that is ergonomical enough?

[clayton615]

I've been messing with this off and on over the week, and I must admit I'm struggling a bit. Since I can't return the sockaddr_iucv, I think the thing to do is just have a sock_iucv_userid and sock_iucv_name that just return the array of bytes. If you'd rather me not clutter up your crate with a bunch of functions, I'm fine with just adding them as a trait in my own crate.

Also, not really sure how to hold the SockAddrStorage::view_as() function. Since it wants an &mut Self, and in theory this function just be fine as just &self, I'm a little lost on how to accomplish that other than to clone it to get a mutable copy. I feel like that's not the best idea though. Mind you, I'm a little lower level than I usually go, so this could just be inexperience.

[Thomasdezeeuw]

@clayton615 here is an example of using the API:

fn iucv_socket_example() {
    // NOTE: I don't use IUCV, so I have no idea what the type or protocol
    // should be set to.
    let socket = Socket::new(Domain::IUCV, Type::STREAM, None).unwrap();

    // Create a new address.
    let mut storage = SockAddrStorage::zeroed();
    let uicv_address: &mut libc::sockaddr_iucv =
        unsafe { storage.view_as::<libc::sockaddr_iucv>() };
    // Set the correct family.
    uicv_address.siucv_family = libc::AF_IUCV;
    // Se the other fields.
    uicv_address.siucv_nodeid = [1; _];
    uicv_address.siucv_user_id = [2; _];
    uicv_address.siucv_name = [3; 8];

    // Create the SockAddr type with the correct length.
    let address_length = size_of::<libc::sockaddr_iucv>() as socklen_t;
    let address = unsafe { SockAddr::new(storage, address_length) };

    // Use the IUCV address to connect the socket.
    socket.connect(&address).unwrap();

    // Get the local (or peer address).
    let address = socket.local_addr().unwrap();

    // Check the family (can also use SockAddr::family) and the length.
    assert!(address.domain() == Domain::IUCV);
    assert!(address.len() == size_of::<libc::sockaddr_iucv>() as socklen_t);
    // Cast the address to the IUCV address type now that we know the family and
    // length is correct.
    let uicv_address: &libc::sockaddr_iucv = unsafe { &*address.as_ptr().cast() };
    // Use the address
    dbg!(&uicv_address.siucv_name);
}

Let me know if this help, if you have more questions let me know.

[clayton615]

Thank you, that's very helpful. That looks pretty similar to what I came up with to return the sockaddr_iucv from libc, just using as_ptr() (which is probably better than my raw cast). I think I will just remove the as_socket_iucv() then and not bother with the names in this PR, since it'll be easy to add that in my own crate. Thanks!

[Thomasdezeeuw]

These changes LGTM. Is it possible to add a test of some kind? Even a test to create a socket would be enough.

[Thomasdezeeuw]

Don't think this is needed as the function isn't unsafe (anymore?).

[clayton615]

You're right, and I can't even remember why I thought I needed that.

[Thomasdezeeuw]

Nit: documentation should come before the cfg attributes.

[clayton615]

Not a problem, I'll change it. It made more sense in my head to put everything after the cfg since all of it should be blocked, but I should have looked at the rest of the code to see how you're doing it for consistency.

[clayton615]

I'm not really sure how to add a test in this case. I guess I could use a similar cfg statement to only run the test on s390x? But unless you're running under z/VM or have a hypersocket defined, it's going to fail... Let me go do some testing on platforms like x86 where the header is there but the underlying mechanism is not, and see what the behavior is.