Page MenuHomeFreeBSD

savecore: add an option to save a live minidump
ClosedPublic

Authored by mhorne on Feb 22 2022, 8:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 11, 5:33 PM
Unknown Object (File)
Thu, Apr 11, 5:29 PM
Unknown Object (File)
Thu, Apr 11, 5:29 PM
Unknown Object (File)
Thu, Apr 11, 5:29 PM
Unknown Object (File)
Thu, Apr 11, 5:29 PM
Unknown Object (File)
Wed, Apr 10, 10:17 PM
Unknown Object (File)
Fri, Mar 29, 1:17 AM
Unknown Object (File)
Mar 3 2024, 3:26 AM

Details

Summary

e.g. savecore -L /var/crash

savecore(8) will invoke the livedump via ioctl on /dev/mem, and verify
the resulting core file. In the panic dump case we read the dump headers
and optional encryption key off of the dump device, then finally copy
the contents to a new core file. For the livedump we operate on the core
file itself, truncating the dump header and shifting the dump contents
to the beginning of the file if necessary.

Still a couple of TODO items, but putting this up for review now.

  • code cleanups, check error handling paths
  • find a better way to handle the temporary file/rename
  • update man page
  • test the !livecore case and make sure things aren't broken

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sbin/savecore/savecore.c
1155

I will find a better way to do this.

Rework the change. This splits the livedump handling into a separate routine,
resulting in some duplication but overall a much cleaner integration with the
existing code.

First cut at the man page changes.

debdrup added inline comments.
sbin/savecore/savecore.8
112

Are you sure this is correct syntax according to mandoc -Tlint?

sbin/savecore/savecore.8
112

Yes! No complaints from mandoc or igor, but I had forgotten to check this :) It is copied from the paragraph below.

pauamma_gundo.com added inline comments.
sbin/savecore/savecore.c
1422

While here, this is inconsistent with .Op Fl fkuvz in the manual page and handling of -u in main() below.

This revision now requires changes to proceed.Apr 5 2022, 6:37 PM
sbin/savecore/savecore.c
1422

This is a gratuitous use of 'Request Changes' but sure, I will commit this separately.

sbin/savecore/savecore.c
117

Do these really need to be global variables? It looks like they're only used in DoFile() or DoLiveFile()?

774

This can be moved down to just before the ioctl() call. Then the error paths below won't leak fddev.

776

Maybe explicitly note that we save it to a temporary file since we have to strip off the dump header later.

786

Why does the file need to be truncated?

793

Should be indented by four spaces.

891
902

The code from the xo_get_style() call to here seems to be duplicated - can it be factored out into a function?

1536

IMO this comment should either say why rename rights are needed, or not exist at all.

sbin/savecore/savecore.c
117

Previously they were declared as static in the scope of DoFile(), presumably to avoid allocating multiple large-ish arrays on the stack. While I could add the same to DoLiveFile(), I felt this was better than duplicating the number of large static arrays.

786

Heh, I suppose it no longer does. This was before the temp file was given a unique name.

Handle review comments from markj. Move the ftruncate() call earlier in the routine (before file rename).

mhorne added inline comments.
sbin/savecore/savecore.c
117

Nevermind, see D34821.

902
mhorne added inline comments.
sbin/savecore/savecore.c
804

Turns out this need to be -(off_t)sizeof(kdhl) to work properly on 32-bit arm :-|

This revision was not accepted when it landed; it landed in state Needs Review.Apr 18 2022, 3:57 PM
This revision was automatically updated to reflect the committed changes.