Thanks for the review, Chris! On Mon, Feb 03, 2025 at 11:52:08PM +0100, Chris Hofstaedtler wrote: > ISTM this is a lot of new, potentially fragile sh code, possibly > with undefined semantics. Indeed shellcheck has a lot of things to > say, including: > > | In prune-logs line 9: > | set -o pipefail > | ^------^ > | SC3040 (warning): In POSIX sh, set option pipefail is undefined. > > I'm not an expert in sh scripting, but I'd try to make it shellcheck > clean.
Thanks for the prompt. Partly I made the mistake of treating "dash has it" as a proxy for "POSIX has it"... Now cleaned, trimmed and safer on my 'log-pruning' branch. > Generally I think whatever Debian ends up shipping should > also go upstream. Surely it depends - some thing are highly distro-specific. The amount of detritus that accumulates in upstream contrib directories over the years... scripts that new integrators would surely rather rewrite for their own context. (As for the tool itself, however, upstreaming is going well so far...!) In this case I'd so much rather use logrotate (locking concerns acknowledged) because it has all the semantics you want, configurable in a standard way. But that doesn't fit with wtmpdb's rotation scheme and I didn't imagine dumping that entirely would go down well. > > One nicety of the old wtmp logrotate config is that it wouldn't even > > rotate until the log hit 1MB. I did add a size gate to the cron job. The unit file could be conditional too? > > What do we think of that idea? It seems logical - no point in rotating > > tiny files! > > It's a trade off to what should be kept. If you have strict > requirements on how many days can be kept, then also small files > must be treated. Not sure what you're suggesting there, I mean that there's no point in splitting the master log file into tiny fragments in the first place, not that they shouldn't be kept - to the contrary! I don't think we have to be too tight on the storage, just to be bounded, not be pathological and not leave a dog's breakfast on the user's filesystem! > Maybe a find call with -mtime <something> would be enough. I see what you mean, I did think of that of course, but I wasn't convinced that relying on file metadata wasn't itself fragile; I am concerned that we need to fail safe by deleting less, hence looking at age and count. (Size is an additional gate.) By the time you've glued those things together you're going to be growing that wrapper too. I also wondered about shipping with the pruner disabled - we only really need a way to make sure it can't grow endlessly. With nothing it rather looks like it's not been considered. Again, thanks for taking the trouble to feed back. I'm keen for users to have a good, least-surprises experience.
