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

[Dave Chinner <david@fromorbit.com>]

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.

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@fromorbit.com