Page MenuHomeFreeBSD

signal: Add SIG_FOREACH and refactor issignal()
ClosedPublic

Authored by markj on Tue, Oct 12, 9:10 PM.

Details

Summary

Add a SIG_FOREACH macro that can be used to iterate over a signal set.
This is a bit cleaner and more efficient than calling sig_ffs() in a
loop. The implementation is based on BIT_FOREACH_ISSET(), except
that the bitset limbs are always 32 bits wide, and signal sets are
1-indexed rather than 0-indexed like bitset(9) sets.

issignal() cannot really be modified to use SIG_FOREACH() directly.
Take this opportunity to split the function into two explicit loops.
I've always found this function hard to read and think that this change
is an improvement. The inner body of issignal() is unchanged, except
that sigprop() is only called when needed now.

Remove sig_ffs(), nothing uses it now.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Tue, Oct 12, 9:10 PM
sys/kern/kern_sig.c
3158

Isn't break equivalent to goto next there. Then this switch is same as the switch inside P_TRACED block above. Then, could you replace sigpending with a mask consisting only of SIGSTOP and use one switch?

sys/kern/kern_sig.c
3158

Oops, I had such a change stashed and forgot to add it. Yes, it is possible to merge the switch statements.

sys/kern/kern_sig.c
3158

Still please either use break in HANDLED case, or goto next in NEXT.

sys/kern/kern_sig.c
3158

break and goto next are not equivalent here. goto next reloads the sigpending set from the thread+process. "break" continues iteration within the already loaded sigpending set. SIGSTATUS_NEXT is returned only when nothing was done and the proc lock was not dropped.

sys/kern/kern_sig.c
3083

I suggest to move both sigqueue_delete calls to getsignal(), and replace all 'goto next' in this function by return (SIGSTATUS_NEXT);. Also I think that SIGSTATUS_NEXT is better named SIGSTATUS_IGNORE

3096

Why not call this function issignal? Then you do not need to change cursig() and it makes one less thing to track between older and newer branches.

markj marked 2 inline comments as done.

Apply feedback:

  • Remove the signal from td/proc sets in the outer function
  • SIGSTATUS_NEXT -> _IGNORE
  • Rename back to issignal(), the helper function is called sigprocess()
This revision is now accepted and ready to land.Wed, Oct 13, 3:11 PM