stderr open
ClosedPublic
Actions

Authored by kevans on Nov 22 2023, 5:12 AM.

Details

Reviewers

0mp
allanjude
des

Group Reviewers

Klara

Commits

rG18425c19cae0: lockf: don't hold stdin/stdout/stderr open

Summary

None of these are essential in the lockf monitor (parent post-fork), so
close them to maintain the illusion that lockf hasn't been inserted into
the pipeline. This ensures that the correct effects happen on other
programs in the pipeline if the locked command closes or redirects these
elsewhere.

The original patch used -s to close stdout/stderr rather than closing
them unconditionally, but it's not clear that we really care that much.

PR: 112379
Sponsored by: Klara, Inc.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kevans created this revision.Nov 22 2023, 5:12 AM

Herald added a subscriber: imp. · View Herald TranscriptNov 22 2023, 5:12 AM

kevans requested review of this revision.Nov 22 2023, 5:12 AM

Harbormaster completed remote builds in B54561: Diff 130417.Nov 22 2023, 5:12 AM

kevans added a parent revision: D42712: lockf: correct omission in the usage statement.Nov 22 2023, 5:12 AM

kevans added a child revision: D42714: lockf: add some tests.

It seems like the commit message should contain "Reported by". The reference to "the original patch" is a bit unclear as well. It took me a moment to figure out that this is pointing at the patch from the PR.

Perhaps the commit message could contain the example from the Bugzilla PR, i.e., (script1.sh | lockf foo script2.sh 2>&1 &) | mail -s something somewhere.

usr.bin/lockf/lockf.c
164	Didn't the old `err` contain more details?

This revision is now accepted and ready to land.Nov 22 2023, 10:35 AM

reviewed-by: allanjude

des added a subscriber: des.Nov 22 2023, 2:37 PM

des added inline comments.

usr.bin/lockf/lockf.c
164	sure but there is no `stderr` at this point.

des requested changes to this revision.Nov 22 2023, 2:39 PM

des added inline comments.

usr.bin/lockf/lockf.c
216	`stderr` is almost certainly closed at this point.

This revision now requires changes to proceed.Nov 22 2023, 2:39 PM

stderr will almost always be closed when killed is called, so just exit.

Harbormaster completed remote builds in B54572: Diff 130432.Nov 22 2023, 4:47 PM

kevans added inline comments.Nov 22 2023, 4:47 PM

usr.bin/lockf/lockf.c
164	I note that there's very little failure mode left with waitpid() anyways when used in this manner, so I wasn't too concerned about working out some gymnastics to try and make it more debuggable looking.