lockf: switch to a SIGCHLD model for reaping child
AcceptedPublic
Actions

Authored by kevans on Tue, Jun 24, 11:19 PM.

Details

Reviewers

allanjude
des
kib

Summary

A future change will add a -T flag to forward SIGTERM along to the
child before we cleanup and terminate ourselves. Using a SIGCHLD
handler to do that with SIGTERM blocked only while the child is actively
being collected will enable us to safely do so without having to worry
that our pid is potentially invalid.

Add a test that concisely checks that the child's error is properly
bubbled up to the caller.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 65095
Build 61978: arc lint + arc unit

Event Timeline

kevans created this revision.Tue, Jun 24, 11:19 PM

Herald added a subscriber: imp. · View Herald TranscriptTue, Jun 24, 11:19 PM

kevans requested review of this revision.Tue, Jun 24, 11:19 PM

Harbormaster completed remote builds in B65064: Diff 157554.Tue, Jun 24, 11:19 PM

kevans added a child revision: D51025: lockf: add a -T option to terminate the child upon early abort.Tue, Jun 24, 11:19 PM

Adding @kib to validate fence usage.

kib added inline comments.Tue, Jun 24, 11:45 PM

usr.bin/lockf/lockf.c
251	You fork before setting the handler for SIGCHLD. See aa8cdb7cae7b7550d5cb8 for the fix for the same bug.
290	BTW this method of getting the child status looses high 24bits of it.

kevans marked an inline comment as done.Wed, Jun 25, 12:56 AM

kevans added inline comments.

usr.bin/lockf/lockf.c
290	To make sure I understand: this is waitpid vs. collecting from a waitpid() siginfo, right? Since we have dedicated fields for signal and exit status rather than compressing it all into a single 32-bit value?

kevans added inline comments.Wed, Jun 25, 1:10 AM

usr.bin/lockf/lockf.c
290	that second one is apparently autocorrect from waitid, of course

kib added inline comments.Wed, Jun 25, 2:35 AM

usr.bin/lockf/lockf.c
290	Yes

des added inline comments.Wed, Jun 25, 10:31 AM

usr.bin/lockf/lockf.c
104	this line should be lower (sort by size then by name)
365

Address review feedback; waitpid -> waitid will happen in a near-future change,
following ATF changes in D51034 and D51035 to allow me to test exit status
preservation above 8-bits.

Harbormaster completed remote builds in B65095: Diff 157642.Thu, Jun 26, 3:14 AM

Actually push all of the review feedback

Local changes had moved part of the signal setup earlier to avoid potentially
losing the new child in the process, but they apparently got stashed rather than
folded in. Added a note that we should just switch to a signal pipe if we add
any more complexity that we need to unwind in the child.

Harbormaster completed remote builds in B65096: Diff 157643.Thu, Jun 26, 3:21 AM

kib accepted this revision.Thu, Jun 26, 4:41 AM

kib added inline comments.