Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bootmisc: use find ... -delete for all cleanup in tmp directories #467

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

williamh
Copy link
Contributor

This is for #458

@williamh
Copy link
Contributor Author

@kaniini @vapier @richfelker Any comments on this?

@kaniini
Copy link
Contributor

kaniini commented Oct 10, 2021

I think doing it that way is going to be safer, but I also think that we should still have an option to turn off the wiping entirely, like Debian's TMPTIME=-1.

@williamh
Copy link
Contributor Author

@kaniini Does setting wipe_tmp to no in /etc/conf.d/bootmisc do what you need?

@kaniini
Copy link
Contributor

kaniini commented Oct 10, 2021

Setting wipe_tmp to no, still wipes some files. It is more selective, but would be nice to disable entirely in favor of more robust solutions like tmpreaper.

@vapier
Copy link
Member

vapier commented Oct 11, 2021

if you set cleanup_tmp_dir=" ", then no path will be walked. we still initialize X files which is needed for anyone using X. putting that behind a knob feels like overkill, but i don't think we can do any sort of reasonable dynamic checks on it.

as i noted in the bug, using find unconditionally negatively impacts just about everyone just to hypothetically make a corrupted system less corrupt. this change is the wrong direction to go.

@williamh
Copy link
Contributor Author

@robbat2 I'll give a few more hours before i merge this, but I was chatting with @kaniini tonight and we were talking about using the commands from Debian again. That's what the secomd commit on this pr does, and it seems that it would also allow removing the last find. Please comment on whether it is reasonable to squash and merge these or whether I should just merge the first commit into master.

@ncopa
Copy link
Contributor

ncopa commented Apr 27, 2023

I think this is fine 👍

Would be nice if this should be merged.

Thanks!

@vapier
Copy link
Member

vapier commented May 2, 2023

except none of the problems highlighted have been addressed

# Blah, too many files
find . -maxdepth 1 -name '[!ajlq\.]*' -exec rm -rf -- {} +
fi
find "${dir}" -xdev -maxdepth 1 -name '[!ajlq\.]*' -exec rm -rf -- {} +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't actually ignore submounts because rm -rf is still being invoked on the topdir. e.g. /tmp/foo/bar mountpoints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants