On Wed, Feb 21, 2024 at 03:09:51PM +0100, David Sterba wrote: > Hi, > > reading [1] and how late it was found that effectively a "rm -rf /" can > happen makes me worried about what I can expect from fstests after git > pull. Many people contribute and the number for custom _cleanup() > functions with unquoted 'rm' commands is just asking for more problems. > > [1] https://lore.kernel.org/all/20240205060016.7fgiyafbnrvf5chj@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ I started down the _cleanup() path a couple of years ago and one of the reasons for that was getting rid of all the open coded rm commands that were often just plain wrong. That start was here: https://lore.kernel.org/fstests/20220524073411.1943480-1-david@xxxxxxxxxxxxx/ But I got little interest except for one person picking at irrelevant details and wanting unnecessary API and naming changes that did nothing to really further the cleanup work. It did seem like anyone was interested in having this code cleaned up and so I basically couldn't find the motivation to slog through hundreds of tests trying do stuff that nobody really seemed to care about.... Shame, this whole problem would have not existed if that work sort of infrastructure technical debt reduction was encouraged, and if it did there'd only be one line of code to change... :/ > Unquoted arguments in shell scripts is IMO a big anti-pattern, > unfortunately present everywhere in xfstests since the beginning. > Rewriting all scripts would be quite a lot of work, could you at least > provide safe versions of the cleanup helpers? > > For example: > > _rm_tmp() { > rm -rf -- $tmp > } > > and used as > > _cleanup() { > _rm_tmp > } > > I can send patches at least for btrfs and generic as this affects > me but first I'd like to know that this will become standard > coding style requirement in fstests. I think it would adress this specific issue, but I think it doesn't address the bigger problem that fixing cleanup behaviour requires touching a couple of thousand tests. i.e. it doesn't reduce the maintenance burden of this code at all. The vast majority of cleanup functions are identical and/or unnecessary, so the right thing to do is to only have cleanup functions for tests that need them, and for those that do need to clean up to only have to clean up their own mess. i.e. the test harness itself should be responsible for cleaning up $tmp stuff and doing stuff like returning to the correct directory after the test completes, not require every test to duplicate the same cargo-culted behaviour... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
