Support AIX signal handler types #136
Conversation
There was a problem hiding this comment.
Hello
There are some style issues that would be nice to get addressed. Nevertheless, there's this bigger concern I have.
You mention that AIX is not yet a tier-3 Rust target. When trying to look at the libc crate, it doesn't seem to offer any aix target officially and grepping it for the word aix yields nothing, so is it even supported by libc? Or is it some kind of unofficial fork of the libc crate?
So I'd like to know how stable that target is, if it's likely to change (like, if the libc crate might paper-over the differences in the names, for example, in the future, which would break the currently proposed code). If it's not going to change, having it included in the CI in some way would be nice, because I have no other way to tell if it has any chance to work or if some future changes would break it.
signal-hook-registry/src/lib.rs
Outdated
| #[cfg(target_os = "aix")] | ||
| let set_handler = | ||
| |action: &mut libc::sigaction, handler| action.sa_union.__su_sigaction = handler; | ||
| set_handler(&mut new, handler); |
There was a problem hiding this comment.
Why in this super-complicated way and not just having conditionally-compiled blocks?
src/low_level/pipe.rs
Outdated
| @@ -141,6 +141,9 @@ pub(crate) fn wake(pipe: RawFd, method: WakeMethod) { | |||
| let data = b"X" as *const _ as *const _; | |||
| match method { | |||
| WakeMethod::Write => libc::write(pipe, data, 1), | |||
| #[cfg(target_os = "aix")] | |||
| WakeMethod::Send => libc::send(pipe, data, 1, libc::MSG_NONBLOCK), | |||
| #[cfg(not(target_os = "aix"))] | |||
There was a problem hiding this comment.
I would prefer the same nowait_flag trick here too, to avoid two „same“ calls to duplicate in the code.
src/low_level/pipe.rs
Outdated
| let res = unsafe { libc::send(pipe, &[] as *const _, 0, libc::MSG_DONTWAIT) }; | ||
| #[cfg(target_os = "aix")] | ||
| let res = unsafe { libc::send(pipe, &[] as *const _, 0, libc::MSG_NONBLOCK) }; |
src/low_level/signal_details.rs
Outdated
| usize, | ||
| extern "C" fn(libc::c_int, *mut libc::siginfo_t, *mut libc::c_void), | ||
| >(sigaction) | ||
| }; |
There was a problem hiding this comment.
Doesn't the same sigaction as _ work, only assigning it to a different field?
There was a problem hiding this comment.
Seems direct casting from usize to extern fn is not allowed:
error[E0605]: non-primitive cast: `usize` as `extern "C" fn(i32, *mut siginfo_t, *mut c_void)`
--> src/low_level/signal_details.rs:120:46
|
120 | action.sa_union.__su_sigaction = libc::SIG_DFL as _;
| ^^^^^^^^^^^^^^^^^^ invalid cast
AIX has not been an official Rust target yet. We have a proposal here rust-lang/compiler-team#553 and multiple patches like rust-lang/libc#2894. So this PR might need to be in draft status. As for CI, we also plan to gradually set up CI for Rust toolchain after AIX target got accepted, but since it seems no public CI system supports AIX, we need to figure out how to support CI of these critical crates (also FYI @bzEq). Is that the necessity for now? |
Not necessarily. Considering the state of the proposal, etc, I'd be OK merging it with the knowledge this is an experimental target, not guaranteed to work or compile despite the crate being past 1.0. That is, I'm OK accepting it and even publishing a new version if it helps you, but until there's some CI and such, I can't promise not to accidentally break it in some future version 🤷. If that's fine for you, we can go forward with this. |
Thanks. Now |
|
Thank you, released… |
|
Thanks! |
The main differences for AIX:
MSG_DONTWAIT, instead, useMSG_NONBLOCK(AIX socket don't have MSG_DONTWAIT tag stephane/libmodbus#294)sa_union { __su_handler: extern fn(c: ::c_int), __su_sigaction: extern fn(c: ::c_int, info: *mut siginfo_t, ptr: *mut ::c_void) }Note that AIX hasn't been an official Tier-3 Rust target. (rust-lang/compiler-team#553)