linux: Qdd sockaddr_iucv - #5041
Conversation
|
I blocked all of 32bit PPC, it's only missing on ppc32le apparently, but since IUCV is only really useful on s390x I think it's fine. Everywhere else, the kernel headers exist, so I don't think it hurts to include it. |
|
Nevermind, I'm realizing I have no clue how to tell what archs have the header vs which don't. I know I want it on s390x to work, and at least x86 so that rust analyzer won't scream while I'm writing code around it. |
|
Huzzah, all tests pass and I think I've done this all correctly. |
| if #[cfg(all( | ||
| any(target_arch = "x86_64", target_arch = "s390x"), | ||
| not(any(target_env = "musl", target_os = "android")) | ||
| ))] { | ||
| s! { | ||
| pub struct sockaddr_iucv { |
There was a problem hiding this comment.
Put this in linux_like/linux/gnu; currently this is available on theoretical emscripten and l4re targets which I think probably doesn't make sense. I think you may also be able to drop the target_arch list too if you do that, and just leave it enabled everywhere.
(test config will need to be updated to match)
There was a problem hiding this comment.
Okay, I'll try it under linux/gnu and see if the tests pass. Thanks.
| pub siucv_family: crate::sa_family_t, | ||
| pub siucv_port: crate::in_port_t, | ||
| pub siucv_addr: crate::in_addr_t, | ||
| pub siucv_nodeid: [c_char; 8], | ||
| pub siucv_user_id: [c_char; 8], | ||
| pub siucv_name: [c_char; 8], |
There was a problem hiding this comment.
Per https://github.com/torvalds/linux/blob/7ca6d1cfec80ebe46cc063f3284c5896c344d9a1/include/net/iucv/af_iucv.h#L46-L50 all except for the first field are reserved. Does common access require using them or could we make them private in case things change?
There was a problem hiding this comment.
Port and Addr don't do anything as far as I know. Making them private should be fine. NodeID and UserID are used to specify your target z/VM ID to connect to, and Name is the application. However, my basic understanding of the C code is that NodeID is actually unused in the linux implementation, so I don't know if the right thing to do is make it private or leave it public in the hopes that some day it's fixed.
In other words, at least UserID and Name should be public. I'll defer to experience on NodeID.
There was a problem hiding this comment.
If you don't know of port and addr to do anything and they have a "reserved" comment, would you mind making them private? It's nice to be on the conservative side here because a private field also makes the struct effectively non-exhaustive so we can add fields in the future (just using #[non_exhaustive] would be great but it has a bug currently). And it's easy enough to undo that if we do need them on the future.
|
Looks like even when restricted to gnu, it still isn't present on 32bit systems. I'll update it to only look for 64bit. |
|
Can I just do |
It's much appreciated! Sorry things are a bit confusing. Based on this, moving to We have a new structure at |
|
Also the CI failure should be fixed if you can rebase. |
|
Whoops, was just trying to rebase, didn't mean to push changes yet. I need more practice with git. Since I'm hear, if I make the fields siucv_port and siucv_addr private, it causes the tests from cargo test to fail. Should I be trying to edit those as well to not do whatever it is that it's doing on them? |
|
Mind posting the failure? I think things should work. But if making them private turns out to be more effort than it's worth, feel free to just leave them as-is 🙂 |
|
There's about 10 or so errors, all like the following: I'm going to be honest, I'm not really looking through the test code. Since it picked up my changes and started testing them, I was hoping that was enough. If I should be digging through them and making changes there too, let me know, but honestly since it's clearly running some tests against my new struct I was hoping that was "good enough". |
This comment has been minimized.
This comment has been minimized.
|
Looks like all the tests are passing and I think I've rebased correctly. Is there any additional action necessary on my end to move this forward? |
|
@tgross35 sorry to pester, but I need to know if any additional action is required from me. |
|
@clayton615 You need to use @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I'm still pretty new at all of this, but I think I've done what you asked. It looks like it's squashed into one commit, and I rebased on Main. I've also used padding on the two unused fields. Can someone confirm this is correct? Thanks for all of your patience while I'm learning. |
|
Why did the android test just cancel itself? I don't see any obvious error in the log... Is there a way for me to restart it without having to push another commit? |
This adds the sockaddr_iucv define in netiucv/iucv. It is primarily used on the s390x platform when linux is running under z/VM, but is present on all the linux systems I've been able to test on. [ cleaned up commit message - Trevor ]
There was a problem hiding this comment.
Android CI is unfortunately flaky, see #4297. But this should be good, thank you for the updates!
I pushed your branch to clean up the commit message, the fixup messages that aren't relevant to the final patch don't need to be kept around. (If using interactive rebase, f/fixup rather than s/squash is probably what you actually want).
|
Thanks everyone. Will this also trickle into 0.2? Most packages rely on that branch, so I'm hoping it can be added there. I did mark it for stable nominated, so just checking if it will happen or not. |
|
It will, the |
This adds the sockaddr_iucv define in netiucv/iucv. It is primarily used on the s390x platform when linux is running under z/VM, but is present on all the linux systems I've been able to test on. [ cleaned up commit message - Trevor ] (backport <rust-lang/libc#5041>) (cherry picked from commit 7a0c435)
This adds the sockaddr_iucv define in netiucv/iucv. It is primarily used on the s390x platform when linux is running under z/VM, but is present on all the linux systems I've been able to test on.
Description
Adds support for sockaddr_iucv
Sources
https://github.com/torvalds/linux/blob/5619b098e2fbf3a23bf13d91897056a1fe238c6d/include/net/iucv/iucv.h
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI
@rustbot label +stable-nominated