Skip to content
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

config: add HAVE_DEQUEUE_SIGNAL_3ARG_SIGINFO #16662

Closed
wants to merge 1 commit into from

Conversation

Finix1979
Copy link
Contributor

When I use the latest stable version of ZFS(2.3-release), I find that it cannot compile on my Kylin OS, which is based on the 4.19 kernel version.

Motivation and Context

I have figured out that both 4.18 and 4.19 still use siginfo_t as the third parameter of the dequeue_signal function, which takes three parameters.

https://elixir.bootlin.com/linux/v4.18/source/include/linux/sched/signal.h
https://elixir.bootlin.com/linux/v4.19.322/source/include/linux/sched/signal.h

Description

Besides the current 3arg version checking, I add kthread_dequeue_signal_3arg_siginfo.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

The dequeue_signal API accepts siginfo_t as the third parameter
instead of kernel_siginfo_t in 4.18 and 4.19.

Signed-off-by: Finix <[email protected]>
@amotin amotin requested a review from robn October 18, 2024 13:57
@robn
Copy link
Member

robn commented Oct 18, 2024

Ooh, this is subtle, and I'm annoyed I missed it.

I'll write an explanation out in full, though I know you know this. It helps me make sure I'm understanding right :)

Problem was actually introduced in d6b8c17, which I did a poor job of documenting properly, and is how we got here.

6.12 makes dequeue_signal three-args again, but different to the pre 5.17. So now we have:

  • < 5.17: int dequeue_signal(struct task_struct *task, sigset_t *mask, kernel_siginfo_t *info)
  • 5.17-6.11: int dequeue_signal(struct task_struct *task, sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)
  • 6.12: int dequeue_signal(sigset_t *mask, kernel_siginfo_t *info, enum pid_type *type)

So I introduced the _3ARG_TASK test, looking for the old version. But there's a subtlety. In 4.19, the signature was:

  • int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)

4.20 (torvalds/linux@ae7795bc6187) introduced kernel_siginfo_t as a smaller siginfo_t for inside the kernel. And, we already knew this: ZFS_AC_KERNEL_SIGINFO checks and defines HAVE_SIGINFO appropriately, and we typedef `spl_kernel_siginfo_t appropriately.

And that makes this annoying, because had we defined HAVE_DEQUEUE_SIGNAL_3ARG_TASK correctly on 4.19, it would have compiled just fine!

So, I think the better thing to do is what I should have done in the first place: check for the 6.12 version directly, and define for that, and always fallback to the old one. That way we never have to know the correct name for siginfo_t in configure, and once we're compiling the code proper, we'll have spl_kernel_siginfo_t and it'll all fall out right, and we won't need an additional check.

I'm not sure why I didn't do it that way in the first place, to be honest. My guess is that it was a bit more complicated within the existing structure, but also maybe I just got distracted.

So yeah, that's what I reckon. I'll take a swing at that later today, unless you get there first. Let me know! Cheers!

@behlendorf
Copy link
Contributor

Thanks for catching this and posting a fix! I'd like to go with Rob's alternate change for this though which is more consistent with how we handle this kind of thing in the other autoconf checks. @Finix1979 it'd be great if you could look over #16666 and test it so we're certain it does resolve your build issue.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Oct 20, 2024
@Finix1979
Copy link
Contributor Author

Thanks for catching this and posting a fix! I'd like to go with Rob's alternate change for this though which is more consistent with how we handle this kind of thing in the other autoconf checks. @Finix1979 it'd be great if you could look over #16666 and test it so we're certain it does resolve your build issue.

Thank you Rob. I just tested it on my Kylin10 OS and it works.

@robn
Copy link
Member

robn commented Oct 21, 2024

@Finix1979 great to hear, thank you for the fantastic report and patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants