Page MenuHomeFreeBSD
Differential D27245

savecore: uncompress cores
ClosedPublic
Actions

Authored by glebius on Nov 16 2020, 10:22 PM.
Tags
None
Referenced Files
F103239694: D27245.id79733.diff
Fri, Nov 22, 1:07 PM
F103227081: D27245.id79684.diff
Fri, Nov 22, 10:26 AM
F103225811: D27245.diff
Fri, Nov 22, 10:10 AM
Unknown Object (File)
Mon, Nov 18, 11:37 AM
Unknown Object (File)
Mon, Nov 18, 8:11 AM
Unknown Object (File)
Sun, Nov 17, 9:54 AM
Unknown Object (File)
Sun, Nov 17, 6:55 AM
Unknown Object (File)
Sun, Nov 3, 11:39 AM
View All 88 Files
Subscribers
bcr
imp
trasz

Details

Reviewers
markj
gallatin
Group Reviewers
manpages
Commits
rS367824: Add '-u' switch that would uncompress cores that were compressed by
Summary

A real life scenario is that cores are compressed to reduce
size of dumpon partition, but we either don't care about space
in the /var/crash or we have a filesystem level compression of
/var/crash. And we want cores to be uncompressed in /var/crash
because we'd like to instantily read them with kgdb.

In this case we want kernel to write cores compressed, but
savecore(1) write them uncompressed.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

glebius created this revision.Nov 16 2020, 10:22 PM
Herald added a reviewer: manpages. · View Herald TranscriptNov 16 2020, 10:22 PM
Herald added a subscriber: imp. · View Herald Transcript
glebius requested review of this revision.Nov 16 2020, 10:22 PM
Harbormaster completed remote builds in B34840: Diff 79630.Nov 16 2020, 10:22 PM
gallatin accepted this revision.Nov 16 2020, 10:53 PM
This revision is now accepted and ready to land.Nov 16 2020, 10:53 PM
bcr accepted this revision as: manpages.Nov 17 2020, 9:21 AM
bcr added a subscriber: bcr.

OK from manpages.

trasz added a subscriber: trasz.Nov 17 2020, 12:01 PM

Shouldn't this also include a change to default savecore_flags in /etc/defaults/rc.conf?

markj added a comment.Nov 17 2020, 3:08 PM

This patch doesn't support ZSTD yet, since the library isn't yet installed in the base system and I'm reluctant to statically compile it into savecore(1).

It's there, it's just a private lib: /usr/lib/libprivatezstd.so.5

sbin/savecore/savecore.c
750 ↗(On Diff #79630)

Why not set uncompress = false here too?

glebius updated this revision to Diff 79684.Nov 17 2020, 9:02 PM

Add ZSTD support and address comments.

This revision now requires review to proceed.Nov 17 2020, 9:02 PM
Harbormaster completed remote builds in B34866: Diff 79684.Nov 17 2020, 9:02 PM
glebius marked an inline comment as done.Nov 17 2020, 9:03 PM
glebius added a comment.Nov 17 2020, 9:06 PM
In D27245#608500, @trasz wrote:

Shouldn't this also include a change to default savecore_flags in /etc/defaults/rc.conf?

I don't want to change the default behavior.

markj added a comment.Nov 17 2020, 9:14 PM

Looks fine to except some nits.

sbin/savecore/savecore.c
594 ↗(On Diff #79684)

The compression == KERNELDUMP_COMP_* cases should be lifted into their own subroutines IMO.

1024 ↗(On Diff #79684)

It is easier to understand this when written as (iscompressed && !uncompress) || compress).

glebius updated this revision to Diff 79733.Nov 19 2020, 1:58 AM

Separate decompression loops into their own subroutines. Reword
logical statement.

Harbormaster completed remote builds in B34896: Diff 79733.Nov 19 2020, 1:58 AM
glebius updated this revision to Diff 79734.Nov 19 2020, 1:58 AM

Some extra cruft slept in previous update.

Harbormaster completed remote builds in B34897: Diff 79734.Nov 19 2020, 1:58 AM
glebius edited the summary of this revision. (Show Details)Nov 19 2020, 1:59 AM
glebius marked an inline comment as done.
markj accepted this revision.Nov 19 2020, 2:13 AM
markj added inline comments.
sbin/savecore/savecore.c
569 ↗(On Diff #79734)

sizeof(z)

585 ↗(On Diff #79734)

Static analyzers will complain that this buffer is leaked. I know it's not a real problem, but it's also easy to fix.

586 ↗(On Diff #79734)

There's a missing newline here.

612 ↗(On Diff #79734)

I think compression != KERNELDUMP_COMP_NONE is preferable.

This revision is now accepted and ready to land.Nov 19 2020, 2:13 AM
glebius marked 2 inline comments as done.Nov 19 2020, 2:18 AM
glebius added inline comments.
sbin/savecore/savecore.c
585 ↗(On Diff #79734)

Yes. But function has several returns with different allocation conditions. Make it "right" everywhere would make it really hairy.

Closed by commit rS367824: Add '-u' switch that would uncompress cores that were compressed by (authored by glebius). · Explain WhyNov 19 2020, 2:21 AM
This revision was automatically updated to reflect the committed changes.
glebius added a commit: rS367824: Add '-u' switch that would uncompress cores that were compressed by.
markj added inline comments.Nov 19 2020, 2:23 AM
sbin/savecore/savecore.c
585 ↗(On Diff #79734)

You would have to add three free(zbuf) calls, relying on the fact that zbuf is initialized to NULL and that free(NULL) is valid. Not really that hairy. Someone else is inevitably going to come along and fix it anyway, for better or worse, so IMO it's better to just do it now.

glebius added inline comments.Nov 19 2020, 2:50 AM
sbin/savecore/savecore.c
585 ↗(On Diff #79734)

To do it properly the zstd context also needs to be freed, and this is where it gets hairy.

Revision Contents
Changeset List

PathSize
head/
sbin/
savecore/
4 lines
6 lines
219 lines

Diff 79735

View Options

head/sbin/savecore/Makefile

Loading...
View Options

head/sbin/savecore/savecore.8

Loading...
View Options

head/sbin/savecore/savecore.c

Loading...