Rework the gzio API.
ClosedPublic
Actions

Authored by markj on Dec 26 2017, 3:09 AM.

Details

Reviewers

allanjude
cem
def

Group Reviewers

manpages

Commits

rS327707: Generalize the gzio API.

Summary

A long time ago I rewrote kern_gzio.c since it contained some really
questionable code, and I wanted to use it for kernel dumps (it had
existed solely for user process core dumps). It works ok, but now that
we're adding support for zstd dumps, it's become preferable to use a
wrapper API for different compression formats.

This change adds such an API in subr_compressor.c and reworks the user
and kernel core dump code to use it instead. I did it in a way that
minimizes the number of ifdefs needed, and I think kern_shutdown.c is
left neater than it was.

The change is still a bit of a WIP; suggestions for improvements,
especially related to naming, are welcome.

Test Plan

My hacky kernel dump test suite still passes.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj created this revision.Dec 26 2017, 3:09 AM

Harbormaster completed remote builds in B13815: Diff 37025.Dec 26 2017, 3:09 AM

markj edited the test plan for this revision. (Show Details)Dec 26 2017, 3:14 AM

markj added reviewers: allanjude, cem, def.

markj mentioned this in D13101: Add support for Zstd-compressed kernel and userspace dumps.Dec 26 2017, 3:16 AM

cem accepted this revision.Jan 6 2018, 1:47 AM

cem added inline comments.

sys/kern/kern_sig.c
3258 ↗	(On Diff #37025)	Seems like this change effectively disables compressed user cores by default.
sys/kern/subr_compressor.c
46–50 ↗	(On Diff #37025)	Can all of these be const?
54 ↗	(On Diff #37025)	const?
sys/sys/compressor.h
38 ↗	(On Diff #37025)	No need for ZSTD in this patch.

This revision is now accepted and ready to land.Jan 6 2018, 1:47 AM

markj marked 3 inline comments as done.Jan 8 2018, 4:56 PM

markj added inline comments.

sys/kern/kern_sig.c
3258 ↗	(On Diff #37025)	Indeed. It seemed a bit weird to make a kernel option automatically configure a feature by default, and I didn't want to deal with the precedence issue that comes up when we have multiple compression algorithms available. I don't plan to MFC this (at least, not without preserving the current behaviour), so I don't think it's a big deal to change the default behaviour.

Sprinkle const
Remove unintended reference to zstd

This revision now requires review to proceed.Jan 8 2018, 5:00 PM

Add a SPDX identifier to subr_compressor.c

LGTM, modulo disagreement about default-compress disable. Ok to commit — we can discuss later if and how default-compress would be implemented.

sys/kern/kern_sig.c
3258 ↗	(On Diff #37025)	I think some form of compression, as well as at least one compression algorithm feature, should be on by default. It's fine if disabling all compression features disables compression, but I think we should have relatively sane defaults. Forcing everyone to add an rc.conf or loader.conf line for compressed cores is just another thing unnecessarily wrong out of the box.