-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add missing definitions for QNX8 for several BPF-related structures and constants #4769
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
base: main
Are you sure you want to change the base?
Conversation
f1b2a57 to
7d27d60
Compare
|
This looks fine to me, cc target maintainers @flba-eb @gh-tr @jonathanpallant @japaric to verify things build |
tgross35
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you actually mind moving this to the src/new module? We're slowly starting to migrate things there since it's easier to match up with what a user can #include. I believe this would be src/new/nto/net/if.h and src/new/nto/net/bpf.h, unless nto has a different structure. You can either move the existing BPF items over or not, your call.
Then this update to src/new/nto/mod.rs:
pub(crate) mod net {
pub(crate) mod route;
}And at
Lines 171 to 186 in 564e0ef
| // Per-OS headers we export | |
| cfg_if! { | |
| if #[cfg(target_os = "android")] { | |
| pub use sys::socket::*; | |
| } else if #[cfg(target_os = "linux")] { | |
| pub use linux::can::bcm::*; | |
| pub use linux::can::j1939::*; | |
| pub use linux::can::raw::*; | |
| pub use linux::can::*; | |
| pub use linux::keyctl::*; | |
| #[cfg(target_env = "gnu")] | |
| pub use net::route::*; | |
| } else if #[cfg(target_vendor = "apple")] { | |
| pub use signal::*; | |
| } | |
| } |
After that and the __c_anonymous_ifc_ifcu removal I'll merge this given we haven't heard back from the maintainers but this is straightforward.
Can you usually |
7d27d60 to
f62f2d4
Compare
This comment has been minimized.
This comment has been minimized.
@tgross35 Moved structs to Sorry for the delay in fixing this! I was away for a bit. |
879b73f to
1687b82
Compare
tgross35
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few small requests and one question. Would you be able to rebase? I think the test errors should be fixed
| pub const SCM_TIMESTAMP: c_int = 0x02; | ||
| cfg_if! { | ||
| if #[cfg(not(target_env = "nto71_iosock"))] { | ||
| if #[cfg(not(any(target_env = "nto71_iosock", target_env = "nto80")))] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these not available on 8.0 either?
If this change is related to making tests pass on 8.0 rather than as part of the BPF, would you be able to put it in a separate commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, these are not available on 8.0, so this is part of the BPF topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't follow the connection - why are sysctl things like HW_IOSTATS related to BPF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion.
I was just referring specifically to the BIOC* constants needed for BPF. I initially assumed that all of the constants in that section were BPF-related, but that turns out not to be the case. After checking the actual BIOC* values used for BPF on QNX 8, I added the new flag because the build was incorrectly picking up the constants from the else branch (intended for QNX 7.1/7.0 I think).
There are many other constants defined there that are unrelated to BPF. My assumption is that nto71_iosock and nto80 share the same constant layout, but I dont work for QNX, so I had to assume a few things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes more sense to me, but I'd like the maintainers to double check since it's a bit surprising.
@flba-eb @gh-tr @jonathanpallant @japaric could you review and/or test the changes to these constants on QNX 8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks okay, but I suggest to inverse the logic:
| if #[cfg(not(any(target_env = "nto71_iosock", target_env = "nto80")))] { | |
| if #[cfg(any(target_env = "nto70", target_env = "nto71"))] { |
- Positive logic is easier to understand
- It is more likely that the list of QNX versions using the older network stack will not need to be modified in the future, but the versions with the new stack will probably need to be adjusted with each new QNX version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these constants all only for the old network stack then? A comment would be nice then if you could add one @rafaeling.
|
Reminder, once the PR becomes ready for a review, use |
1687b82 to
400b741
Compare
|
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. |
src/new/nto/net/if_.rs
Outdated
| pub struct ifreq { | ||
| /// if name, e.g. "en0" | ||
| pub ifr_name: [c_char; crate::IFNAMSIZ], | ||
| pub ifr_ifru: __c_anonymous_ifr_ifru, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably has to go in s_no_extra_traits since the union doesn't have them (you can check that things work with --feature extra_traits)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the structs into s_no_extra_traits (as linux does) but building with --features extra_traitsstill fails because the union would need Eq, Hash, and PartialEq.
On the BSD targets (Apple/FreeBSD/OpenBSD) these unions do implement Eq, Hash, and PartialEq and struct ifreq is into s! macro, should I mirror that behavior here as well? (I tried it and also fails with same error, not sure how to proceed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is failing? If __c_anonymous_ifr_ifru and ifreq are both in s_no_extra_traits, that macro shouldn't try to derive them.
Don't handwrite trait impls, we do have them in some places but we're trying to move away from that because of the easy accidental unsoundness with unions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont know why, but retried again and works now, maybe did something wrong before.
Check last commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I'm going to need to wait for somebody to look at #4769 (comment) though.
This will need a squash before merge.
Description
These structures are required for working with BPF filters and network interfaces on the QNX8 platform. Their absence currently prevents building or using BPF-related functionality with libc on QNX8 targets.
Sources
Updated
src/unix/nto/mod.rsaccordinglyChecklist
libc-test/semverhave been updated-> No tests for QNX?
*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget); especially relevant for platforms that may not be checked in CI.-> even main branch does not work.
Error: could not execute process
/home/user/libc/target/x86_64-pc-nto-qnx800/debug/deps/libc-67e46525c5118905(never executed)