On Fri, Feb 23, 2024 at 02:53:27PM +1100, Dave Chinner wrote: > 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. The patches and the direction is along what I had in mind and would be a good start for sure. > 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... :/ Agreed and that's too bad it did not go anywhere, from the replies here I think we all know we need it. I don't know how feasible it is given your previous attempts and how it was received upstream, I'm kind of disnclined despite my previous enthusiasm. > > 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... Yeah, I picked one specific issue, the bigger problems would emerge once I'd try to fix it.
