Details

Reviewers

markj
imp

Commits

Summary

This can be disabled by sysctl kern.core_dump_can_intr

Tested by: pho

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

kib requested review of this revision.Oct 5 2021, 11:59 AM

It would be good to document this behaviour in core.5, though it is logically not quite the right place. Or at least add a release note.

sys/kern/kern_exec.c
1956	At this point we will needlessly call vn_truncate_locked() before breaking out of the outer loop. Why not break out immediately?
sys/kern/kern_sig.c
3280	Maybe curproc_has_sigkill_pending()? I think this name is not very clear.
3288	Perhaps atomic_load, since there is no synchronization.

Exit core write loop earlier.
Rename function.
Add man page update.

share/man/man5/core.5
55	limited by the .Dv RLIMIT_CORE .Xr setrlimit 2 limit.
60
61
63
65	Or s/later/latter/, but I think this would be an unusual use of "latter".
sys/kern/kern_exec.c
155
1954	No need to set `success`.

sys/kern/kern_sig.c
3280	I'm not sure what "9" is supposed to mean, or why it is preferable to spelling out "sigkill".

Sorry, I forgot to submit two answers to the notes in previous patch update.

sys/kern/kern_sig.c
3280	Pending usually means that signal was not delivered yet due to userspace blocking or something similar. SIGKILL cannot be pending (at least from userspace PoV). I changed it to curproc_is_killed_9, it should be vivid enough.
3280	9 is SIGKILL, but ok
3288	We do unlocked read this way everywhere. Also I do not think that the load through volatile actually improves anything there.

Man page fixes
RWTUN
remove setting of success
rename function to curproc_sigkilled()

imp added inline comments.Oct 6 2021, 2:33 PM

share/man/man5/core.5
60	s/With a large enough limit, processes that had/With a large limit, a process that had/ I think reads better and conveys the same meaning. And it isn't processes, in aggregate, that causes a problem. It's the mapped address space of a single process that's a problem when that process dumps core.
63	I think this sentence is awkward. "The system ignores all signals sent to a process writing a core file, except .Dv SIGKILL which terminates the writing of the core file."
68	What's the normal way to indicate that this sysctl is also a tunable? If there's not a standard one, I'd suggest adding the words "the tunable" after "by setting".
sys/kern/kern_exec.c
154	"intr" seems too concise here. I'd be tempted to call the core_dump_can_intr or core_dump_interrupible (with a preference for the former) mostly because I can read core_dump_intr as some async signal to interrupt the core dump writing.
1943	I'd move interrupted up a line. It doesn't control the loop at all and just needs to be initialized.
1954	What does setting interrupted and breaking a couple of times offer over a 'return (EINTR)' here?

kib marked 6 inline comments as done.Oct 6 2021, 4:00 PM

kib added inline comments.

share/man/man5/core.5
63	I somewhat reworded this, adding more information in the second part of the sentence.
sys/kern/kern_exec.c
1954	So far this function has single return. I tried to keep this (sometimes useful) property.