Page MenuHomeFreeBSD

savecore: add an option to save a live minidump
ClosedPublic

Authored by mhorne on Feb 22 2022, 8:04 PM.

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sbin/savecore/savecore.c
1154

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
111

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

sbin/savecore/savecore.8
111

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

pauamma requested changes to this revision.Apr 5 2022, 6:37 PM
pauamma added a subscriber: pauamma.
pauamma 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
116

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

773

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

775

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

785

Why does the file need to be truncated?

792

Should be indented by four spaces.

890
901

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

1535

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

sbin/savecore/savecore.c
116

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.

785

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
116

Nevermind, see D34821.

901
mhorne added inline comments.
sbin/savecore/savecore.c
803

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.