Page MenuHomeFreeBSD

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

Authored by markj on Jul 13 2017, 8:51 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 23 2022, 3:43 PM
Unknown Object (File)
Dec 18 2022, 6:42 AM
Subscribers

Details

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 added reviewers: cem, def, asomers, rgrimes.
This revision is now accepted and ready to land.Jul 14 2017, 9:36 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.

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?

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 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.

markj edited edge metadata.
markj marked an inline comment as done.
  • Address review feedback.
This revision now requires review to proceed.Aug 2 2017, 10:39 PM
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.

  • Combine error handling for E2BIG and ENOSPC.
def added inline comments.
sys/amd64/amd64/minidump_machdep.c
444 ↗(On Diff #31602)

ENOSPC case is handled before. :)

sys/arm64/arm64/minidump_machdep.c
420 ↗(On Diff #31602)

ENOSPC case is handled before.

This revision is now accepted and ready to land.Aug 6 2017, 3:50 PM
This revision was automatically updated to reflect the committed changes.