kern: move the vnode user coredumper out into its own file
AcceptedPublic
Actions

Authored by kevans on Wed, Jul 16, 4:11 PM.

Details

Reviewers

markj
kib

Summary

This more cleanly contains the bits that are specifically relevant to
dumping coredumps out to a vnode, which will make future changes a bit
easier to review. This also makes the scope of the relevant sysctls
easier to reason about in the process, as they're not visible outside of
the vnode dumper file -- this will mostly become relevant when we allow
pluggable dumpers.

While we're here, move all of the coredump-related stuff out into its
own sys/coredump.h. We have enough that it's useful to separate it out
and de-clutter sys/exec.h a bit.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 65501
Build 62384: arc lint + arc unit

Event Timeline

kevans created this revision.Wed, Jul 16, 4:11 PM

Herald added subscribers: olce, imp. · View Herald TranscriptWed, Jul 16, 4:11 PM

kevans requested review of this revision.Wed, Jul 16, 4:11 PM

Harbormaster completed remote builds in B65487: Diff 158615.Wed, Jul 16, 4:11 PM

kevans added a parent revision: D51337: kern: factor out the 'writing' bits of user process coredumping.Wed, Jul 16, 4:12 PM

kevans added a child revision: D51338: kern: abstract away the vnode coredumper to allow pluggable dumpers.

I would also move rest of generic core dump bits into e.g. kern_ucoredump.c (from kern_sig.c).

sys/kern/coredump_vnode.c
30	No need in param.h it is provided by systm.h
59	Use _Static_assert?
62	and below
77	I suggest getting rid of this define, it does not add any value, only obfuscates the real use of the lock.
79

Address review commentary, and split further bits out of kern_sig.c

sys/coredump.h -> sys/ucoredump.h for symmetry with kern_ucoredump.c. The vnode
dumper remains in coredump_vnode.c for the time being to mirror the historical
naming of coredump().

Herald added a subscriber: ziaee. · View Herald TranscriptThu, Jul 17, 3:08 AM

Harbormaster completed remote builds in B65501: Diff 158648.Thu, Jul 17, 3:08 AM

kevans added inline comments.Thu, Jul 17, 3:11 AM

share/man/man5/core.5
119	This could probably use a rewrite in general... the below options correspond directly to values of kern.compress_uesr_cores below being usable, but it's not really tied in that well. I have a draft re-flow of some of the above text prepared for the next review in the stack, because it's a little messy: we talk about the core filename, segue over to signals blocked, then return back to discussing filename configuration. My edit will move the middle bit down to keep all of the naming stuff together, particularly because it's more useful as those two or three paragraphs around and including it are describing the default vnode dumper.

kib added inline comments.Thu, Jul 17, 3:44 AM

sys/kern/kern_sig.c
3636	I would move sigexit() to kern_ucoredump.c as well.

Take sigexit() with us

sig_do_core() is added for the one conditional in sigexit() that wants to check
a sigprop, rather than opening up sigprops more widely -- this is the only
change to the function in the move. Blank line at the beginning of
sig_do_core() is retained for consistency with the rest of the file.

Harbormaster completed remote builds in B65502: Diff 158651.Thu, Jul 17, 4:10 AM

kib accepted this revision.Thu, Jul 17, 4:14 AM

This revision is now accepted and ready to land.Thu, Jul 17, 4:14 AM

markj accepted this revision.Fri, Jul 18, 2:00 PM

markj added inline comments.