zfs icon indicating copy to clipboard operation
zfs copied to clipboard

config: add HAVE_DEQUEUE_SIGNAL_3ARG_SIGINFO

Open Finix1979 opened this issue 1 year ago • 1 comments

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

  • [x] 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:

  • [x] My code follows the OpenZFS code style requirements.
  • [ ] I have updated the documentation accordingly.
  • [x] I have read the contributing document.
  • [ ] I have added tests to cover my changes.
  • [ ] I have run the ZFS Test Suite with this change applied.
  • [x] All commit messages are properly formatted and contain Signed-off-by.

Finix1979 avatar Oct 18 '24 03:10 Finix1979

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 d6b8c17f1d81b64512ba87be4aff886243cbb8ad, 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!

robn avatar Oct 18 '24 23:10 robn

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 avatar Oct 20 '24 16:10 behlendorf

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.

Finix1979 avatar Oct 21 '24 01:10 Finix1979

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

robn avatar Oct 21 '24 02:10 robn