Overwrite the oldest coredump.
ClosedPublic
Actions

Authored by oshogbo on Jun 24 2018, 9:35 AM.

Details

Reviewers

jhb
kib

Commits

rS335844: core(5): overwrite the oldest core dump

Summary

If we are using %I option in the core dump fileformat we will create as many core dumps as we have CPUs.
The problem is that if we create those core dump the only core dump which will be overwrite will be the last one.
With this patch we will create a new core dump if there is a free slot or if all slot are taken we will overwrite the oldest one.

If you agree about this change I think I should also add the UPDATE note, right?

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

oshogbo created this revision.Jun 24 2018, 9:35 AM

In principle I am indifferent about this, but see the list of the implementation issues.

sys/kern/kern_sig.c
3326 ↗	(On Diff #44378)	Locals should be declared in the locals block.
3341 ↗	(On Diff #44378)	I would directly assigned to *vpp and returned there.
3344 ↗	(On Diff #44378)	Why not use VOP_GETATTR() directly ? You do not need anything except st_mtim.
3351 ↗	(On Diff #44378)	I believe you should compare whole st_mtim, not only the seconds part.
3354 ↗	(On Diff #44378)	It seems you need close_locked() local helper.
3479 ↗	(On Diff #44378)	Local vars should be grouped in the block at the start of the function. Why did you moved tham ?

oshogbo updated this revision to Diff 44515.Jun 27 2018, 10:34 AM

oshogbo marked 3 inline comments as done.

oshogbo added inline comments.

sys/kern/kern_sig.c
3341 ↗	(On Diff #44378)	You still need to unlock the oldvpp.
3344 ↗	(On Diff #44378)	Because I didn't know about it. Fixed.
3479 ↗	(On Diff #44378)	I prefer to declare the variables in the blocks that are really used, especially when we are moving code around and there is chance that we missed some use of variable. I believe that in OpenSolaris this is the standard. But I don't want to start any style discussion. I'm ok with moving that.

kib added inline comments.Jun 27 2018, 2:28 PM

sys/kern/kern_sig.c
3304 ↗	(On Diff #44515)	I would not use vn_ namespace in kern_sig.c. vnode_close_locked() is fine as well.
3358 ↗	(On Diff #44515)	This is wrong test. You should do lasttime.tv_sec > vattr.va_mtime.tv_sec \|\| (lasttime.tv_sec == vattr.va_mtime.tv_sec && lasttime_tv_nsec >= vattr.va_mtime.tv_nsec) Your test fails if A.tv_sec > B.tv_sec while A.tv_nsec < B.tv_nsec.