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.

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

[clayton615]

Sorry again to take so long to get back to this. My open source contributions at work are relegated to my "free time" which I seem to have less and less of lately. So after testing this on x86, despite having the header file, it always results in an Err when creating the socket. It only works on s390x. I don't have access to an LPAR (Logical Partition, closes you get to running on bare metal on IBM Z) so I can't test it there, but the documentation for IUCV says it should work with Hypersockets, so it should be fine in an LPAR. Under z/VM it works as expected, I'm already using my fork of socket2 to do work.

That does make adding some tests difficult. How would you like me to proceed?

[Thomasdezeeuw]

Then we'll have to do without tests. Can you fix up the last couple of comments? Then we can merge this

[clayton615]

Sorry, how do you mean? I thought that I was using Fixup, so it should still just say "adds support for AF_IUCV sockets". Looking at the other commits, it's uppercased and in a different tense "Add" instead of "adds". Is that what you're asking me to change?

[Thomasdezeeuw]

I meant addressing the review comments I've made like #668 (comment).

[clayton615]

Okay, hopefully I understood your meaning correctly. I've added them as part of the commit description so as not to go past the 50/72 rule.

Derp. I somehow read comments as commits no less than 3 times. Sorry about that.