Skip to content

Fix siginfo_t for Illumos #2696

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

Closed
wants to merge 1 commit into from
Closed

Fix siginfo_t for Illumos #2696

wants to merge 1 commit into from

Conversation

dylni
Copy link
Contributor

@dylni dylni commented Feb 22, 2022

The siginfo_t struct appears to be incorrect for Illumos. It currently has si_code before si_errno and appears to be too large. I'm not familiar with this platform, so please let me know if this fix is inaccurate.

siginfo_t:
https://github.com/illumos/illumos-gate/blob/74e98f497ae878ae481ccba1f91fd8dc26fa6627/usr/src/boot/sys/sys/signal.h#L196

siginfo.h:
https://github.com/illumos/illumos-gate/blob/74e98f497ae878ae481ccba1f91fd8dc26fa6627/usr/src/uts/common/sys/siginfo.h#L164

I am not sure if the packed() attribute should remain, but I copied it from the Solaris definition.

@rust-highfive
Copy link

Some changes occurred in solarish module

cc @jclulow,@pfmooney

@rust-highfive
Copy link

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

@jclulow
Copy link
Contributor

jclulow commented Feb 22, 2022

siginfo_t: https://github.com/illumos/illumos-gate/blob/74e98f497ae878ae481ccba1f91fd8dc26fa6627/usr/src/boot/sys/sys/signal.h#L196

That's not the right header. Note that it's under usr/src/boot which was imported with our copy of the FreeBSD boot loader.

@jclulow
Copy link
Contributor

jclulow commented Feb 22, 2022

Can I ask what lead you to make this change? Did something fail to compile or work correctly on an illumos system?

@dylni
Copy link
Contributor Author

dylni commented Feb 22, 2022

@jclulow Yes, the motivation is this issue:
dylni/process_control#12

libc currently does not define siginfo_t::si_status on Illumos, which is required by process_control. The signal codes are also required so were included with this change.

If you let me know the correct definition, I can certainly update this PR.

@dylni dylni marked this pull request as draft February 22, 2022 02:34
@pfmooney
Copy link
Contributor

@jclulow Yes, the motivation is this issue: dylni/process_control#12

libc currently does not define siginfo_t::si_status on Illumos, which is required by process_control. The signal codes are also required so were included with this change.

If you let me know the correct definition, I can certainly update this PR.

I'm working on an update to expose the siginfo_t fields you need using function accessors like the other unix-y platforms have done. I should have it posted in the next day or so.

@dylni
Copy link
Contributor Author

dylni commented Feb 22, 2022

Thank you @pfmooney! Please also include CLD_EXITED and the related constants from this PR if possible.

@dylni dylni closed this Feb 22, 2022
@dylni dylni deleted the fix-siginfo_t-for-illumos branch February 22, 2022 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants