Factor out common kerneldump code into dump_start()/dump_finish().
ClosedPublic
Actions

Authored by markj on Jul 13 2017, 8:51 PM.

Details

Reviewers

cem
def
asomers
rgrimes

Commits

rS322644: Factor out duplicated kernel dump code into dump_{start,finish}().

Summary

This is a step towards generic support for kernel dump compression.
dump_start() and dump_finish() are responsible for writing kernel dump
headers, optionally writing the crypto key, and initializing "dumplo",
i.e. the offset into the dump device at which we start writing the
dump's contents.

Also remove the unused dump_write_pad(), and make some functions
static now that they're no longer used outside of kern_shutdown.c.

No functional change intended.

Test Plan

minidumps, full dumps, encrypted minidumps, all on amd64

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Jul 13 2017, 8:51 PM

Herald added subscribers: emaste, andrew. · View Herald TranscriptJul 13 2017, 8:51 PM

Harbormaster completed remote builds in B10478: Diff 30745.Jul 13 2017, 8:51 PM

markj edited the test plan for this revision. (Show Details)Jul 13 2017, 8:54 PM

markj added reviewers: cem, def, asomers, rgrimes.

cem accepted this revision.Jul 14 2017, 9:36 PM

This revision is now accepted and ready to land.Jul 14 2017, 9:36 PM

def added inline comments.Jul 31 2017, 8:14 PM

sys/kern/kern_shutdown.c
1238 ↗	(On Diff #30745)	dump_start() returns ENOSPC now while in the past we set error to E2BIG in case of a lack of space on amd64 and arm64. On these architectures in the ENOSPC case we try to create a minidump again. We should remove E2BIG cases as it's dead code and check in case of ENOSPC if the first kernel dump header was successfully written. I think it doesn't make sense to try to write a crash dump again if we fail at the beginning.

markj added inline comments.Jul 31 2017, 9:37 PM

sys/kern/kern_shutdown.c
1238 ↗	(On Diff #30745)	Oops, thanks for catching that. Note that dump_check_bounds() returns ENOSPC. I think it would be simpler to change this to return E2BIG instead?

def added inline comments.Aug 2 2017, 3:06 PM

sys/kern/kern_shutdown.c
1218 ↗	(On Diff #30745)	Can we move it to sys/sys/kerneldump.h and rename to KERNELDUMP_METADATA_SIZE so we can use it also in sys/ddb/ddb/db_textdump.c?
1238 ↗	(On Diff #30745)	Do you mean to change dump_start() to return E2BIG if this check fails? This would be a good idea. Note that this also requires changes in MD code.
1240 ↗	(On Diff #30745)	Currently dump_start() modifies dumplop even if it fails. I would introduce a temporary dumplo variable to update dumplop if dump_start() is successful. It would be less bug prone if we'll want to reuse dumplop in case of a failure in the future. It would be also consistent with dumplo being updated after each dump_write() call in MD code.
1260 ↗	(On Diff #30745)	For consistency with dump_start() I would pass a pointer to dumplo to dump_finish() and update it after a kernel dump header is successfully written.

markj marked 3 inline comments as done.Aug 2 2017, 10:33 PM

markj added inline comments.

sys/kern/kern_shutdown.c
1238 ↗	(On Diff #30745)	Indeed.
1260 ↗	(On Diff #30745)	I'm not sure why it's important to be consistent in this way. A dumplop argument would point into a stack frame that's about to be freed anyway. Further, I'll note that the dumplo/dumplo arguments are removed in a later revision.

Address review feedback.

This revision now requires review to proceed.Aug 2 2017, 10:39 PM

def added inline comments.Aug 3 2017, 9:22 AM

sys/arm/arm/minidump_machdep.c
345 ↗	(On Diff #31501)	I was thinking about else if (error == E2BIG \|\| error == ENOSPC) here and in other architectures as dump_write() can still return ENOSPC which might happen when a number of pages being dumped has increased during dumping.
sys/kern/kern_shutdown.c
1260 ↗	(On Diff #30745)	OK.