Re: Dangerous commands (was:[ANNOUNCE] fstests: for-next branch updated to v2024.02.04)

[David Sterba <dsterba@suse.cz>]

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@dell-per750-06-vm-08.rhts.eng.pek2.redhat.com/
> 
> 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@fromorbit.com/
> 
> 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.