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

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

[David Sterba]

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/

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
}

or at least mandate the "--" separator and quoting arguments in new code
and gradually fix the existing code.

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.

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

[Darrick J. Wong]

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/
> 
> Unquoted arguments in shell scripts is IMO a big anti-pattern,

That wasn't even the problem there.  I temporarily forgot my shellfu and
did the equivalent of:

<never set FUBAR>

rm -rf "$FUBAR"*

The shell helpfully expands the never-set FUBAR to an empty string, then
globs the star to everything in the current directory.  rm has
safeguards to prevent you from "rm -rf /" but that offers no protection
from what happens above.

The argument was quoted "properly", but what I think you're really
asking for is "set -u":

#!/bin/bash -u

unset FUBAR
echo -- rm -f "$FUBAR"*

$ /tmp/a.sh
/tmp/a.sh: line 8: FUBAR: unbound variable

Unfortunately you can't just set that unconditionally for all of fstests
and call it a day because now you have to test every line of shellcode
to make sure nothing breaks on unset variables that would have been fine
otherwise.  That turns into a mess of:

test -n "$FUBAR" && rm -f "$FUBAR"*

Oh but then there's the other bad shell habit of setting variables to
the empty string -- sometimes you really want that empty string, other
times authors seem to have used that in place of unset.  We've gotten
away with that bit of bad hygiene for ages, likely due to -u being a
nondefault option.

> 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?

I worried about this a long time ago, and tried running shellcheck on
the entire codebase.  Thousands of error messages about sloppy quoting
later I gave up.  Later I turned that into a patch in djwong-wtf that
runs it only on the files changed by the head commit.

It **didn't notice** the cleanup error!  So that wouldn't have saved me.

Long term we ought to rewrite fstests in any language that isn't as much
of a foot gun.  Or someone starts a project to set -e -u and deals with
the massive treewide change that's going to be.

> For example:
> 
> _rm_tmp() {
>     rm -rf -- $tmp

Um, isn't this /also/ an unquoted variable?

I guess one could make safe(r) wrappers so at least rm won't get mixed
up by dashes at the start of filenames:

_rm_files() {
	rm -f -- "$@"
}

_rm_dirtree() {
	rm -r -- "$@"
}

But then you do

moopath="foo bar"
_rm_files $moopath

and we've lost the battle yet again.

(On the plus side, that's four separate "f*king bash" invocations in one
email! :))

--D

> }
> 
> and used as
> 
> _cleanup() {
>     _rm_tmp
> }
> 
> or at least mandate the "--" separator and quoting arguments in new code
> and gradually fix the existing code.
> 
> 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.
> 

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

[Dave Chinner]

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

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

[Zorro Lang]

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/
> 
> 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?

Hi David,

Thanks for taking care about it :)

> 
> For example:
> 
> _rm_tmp() {
>     rm -rf -- $tmp

It's "$tmp.*"

May I ask what problem does the "--" hope to avoid? If the "$tmp" is empty,
"rm -rf" and "rm -rf --"" looks like both doing nothing. So what kind
of situation does the "--" hope to fix?

The root problem in above [1] is about "${FOO}*". If someone does "rm -rf ${FOO}*"
in its custom _cleanup_xxxxx function, then it's dangerous if "$FOO" is empty.

I thought some ways to avoid that:
1) Try to avoid doing rm -rf ${FOO}*, if not necessary.
2) Must checks [ -n "$FOO" ] before doing any rm -rf ${FOO}*
3) Someone's custom _cleanup_xxxxx better to be called before default _cleanup
does "cd /".
4) Think about bringing in someone "Static program analysis" tool about bash
script, but I don't know if there're someone good, feel free to give me
suggestions.

> }
> 
> and used as
> 
> _cleanup() {
>     _rm_tmp
> }
> 
> or at least mandate the "--" separator and quoting arguments in new code
> and gradually fix the existing code.
> 
> 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.

If you'd like to send a patch to cleanup the cleanup things, that's welcome.
(just don't bring in regression issue, please:)

> 

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

[Zorro Lang]

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/

Hi Dave,

Thanks for taking care about this issue. Sure I've waited for that patchset V2
long time, after V1 got enough RVB but you hoped to rebase and change it a bit.

If you still hope to do that, I can accept a pull request directly from you,
and I can merge your patchset in a single fstests release version, to avoid
code conflicts.

Hmm... or maybe you hope I can take that job, due to I can always rebase the
changes onto my latest local fstests (which's for next release), and fix code
conflicts locally ?

Thanks,
Zorro

> 
> 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
> 

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

[Eric Biggers]

On Sun, Feb 25, 2024 at 11:16:16PM +0800, Zorro Lang 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/
> > 
> > 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?
> 
> Hi David,
> 
> Thanks for taking care about it :)
> 
> > 
> > For example:
> > 
> > _rm_tmp() {
> >     rm -rf -- $tmp
> 
> It's "$tmp.*"
> 
> May I ask what problem does the "--" hope to avoid? If the "$tmp" is empty,
> "rm -rf" and "rm -rf --"" looks like both doing nothing. So what kind
> of situation does the "--" hope to fix?
> 
> The root problem in above [1] is about "${FOO}*". If someone does "rm -rf ${FOO}*"
> in its custom _cleanup_xxxxx function, then it's dangerous if "$FOO" is empty.
> 
> I thought some ways to avoid that:
> 1) Try to avoid doing rm -rf ${FOO}*, if not necessary.
> 2) Must checks [ -n "$FOO" ] before doing any rm -rf ${FOO}*
> 3) Someone's custom _cleanup_xxxxx better to be called before default _cleanup
> does "cd /".
> 4) Think about bringing in someone "Static program analysis" tool about bash
> script, but I don't know if there're someone good, feel free to give me
> suggestions.

"--" prevents the following arguments from being interpreted as options if they
begin with "-".  That's a good practice, but it doesn't help with ${FOO} being
empty.  To cause the script to exit if ${FOO} is empty, it can be written as
${FOO:?}.  Alternatively, 'set -u' can be used.

- Eric

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

[Darrick J. Wong]

On Sun, Feb 25, 2024 at 08:51:28AM -0800, Eric Biggers wrote:
> On Sun, Feb 25, 2024 at 11:16:16PM +0800, Zorro Lang 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/
> > > 
> > > 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?
> > 
> > Hi David,
> > 
> > Thanks for taking care about it :)
> > 
> > > 
> > > For example:
> > > 
> > > _rm_tmp() {
> > >     rm -rf -- $tmp
> > 
> > It's "$tmp.*"
> > 
> > May I ask what problem does the "--" hope to avoid? If the "$tmp" is empty,
> > "rm -rf" and "rm -rf --"" looks like both doing nothing. So what kind
> > of situation does the "--" hope to fix?
> > 
> > The root problem in above [1] is about "${FOO}*". If someone does "rm -rf ${FOO}*"
> > in its custom _cleanup_xxxxx function, then it's dangerous if "$FOO" is empty.
> > 
> > I thought some ways to avoid that:
> > 1) Try to avoid doing rm -rf ${FOO}*, if not necessary.
> > 2) Must checks [ -n "$FOO" ] before doing any rm -rf ${FOO}*
> > 3) Someone's custom _cleanup_xxxxx better to be called before default _cleanup
> > does "cd /".
> > 4) Think about bringing in someone "Static program analysis" tool about bash
> > script, but I don't know if there're someone good, feel free to give me
> > suggestions.
> 
> "--" prevents the following arguments from being interpreted as options if they
> begin with "-".  That's a good practice, but it doesn't help with ${FOO} being
> empty.  To cause the script to exit if ${FOO} is empty, it can be written as
> ${FOO:?}.  Alternatively, 'set -u' can be used.

I said that four days ago.  Did nobody receive that reply?

https://lore.kernel.org/fstests/20240225165128.GA1128@sol.localdomain/T/#m0efd851c5a1fb0dbe418f4aff818d20f4355638b

--D

> - Eric
> 

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

[Eric Biggers]

On Sun, Feb 25, 2024 at 09:03:04AM -0800, Darrick J. Wong wrote:
> On Sun, Feb 25, 2024 at 08:51:28AM -0800, Eric Biggers wrote:
> > On Sun, Feb 25, 2024 at 11:16:16PM +0800, Zorro Lang 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/
> > > > 
> > > > 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?
> > > 
> > > Hi David,
> > > 
> > > Thanks for taking care about it :)
> > > 
> > > > 
> > > > For example:
> > > > 
> > > > _rm_tmp() {
> > > >     rm -rf -- $tmp
> > > 
> > > It's "$tmp.*"
> > > 
> > > May I ask what problem does the "--" hope to avoid? If the "$tmp" is empty,
> > > "rm -rf" and "rm -rf --"" looks like both doing nothing. So what kind
> > > of situation does the "--" hope to fix?
> > > 
> > > The root problem in above [1] is about "${FOO}*". If someone does "rm -rf ${FOO}*"
> > > in its custom _cleanup_xxxxx function, then it's dangerous if "$FOO" is empty.
> > > 
> > > I thought some ways to avoid that:
> > > 1) Try to avoid doing rm -rf ${FOO}*, if not necessary.
> > > 2) Must checks [ -n "$FOO" ] before doing any rm -rf ${FOO}*
> > > 3) Someone's custom _cleanup_xxxxx better to be called before default _cleanup
> > > does "cd /".
> > > 4) Think about bringing in someone "Static program analysis" tool about bash
> > > script, but I don't know if there're someone good, feel free to give me
> > > suggestions.
> > 
> > "--" prevents the following arguments from being interpreted as options if they
> > begin with "-".  That's a good practice, but it doesn't help with ${FOO} being
> > empty.  To cause the script to exit if ${FOO} is empty, it can be written as
> > ${FOO:?}.  Alternatively, 'set -u' can be used.
> 
> I said that four days ago.  Did nobody receive that reply?
> 
> https://lore.kernel.org/fstests/20240225165128.GA1128@sol.localdomain/T/#m0efd851c5a1fb0dbe418f4aff818d20f4355638b
> 

You didn't mention the :? option, and I thought that would be worth mentioning.

Of course ideally -u would be used everywhere, as you said.

- Eric

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

[Zorro Lang]

On Sun, Feb 25, 2024 at 09:03:04AM -0800, Darrick J. Wong wrote:
> On Sun, Feb 25, 2024 at 08:51:28AM -0800, Eric Biggers wrote:
> > On Sun, Feb 25, 2024 at 11:16:16PM +0800, Zorro Lang 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/
> > > > 
> > > > 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?
> > > 
> > > Hi David,
> > > 
> > > Thanks for taking care about it :)
> > > 
> > > > 
> > > > For example:
> > > > 
> > > > _rm_tmp() {
> > > >     rm -rf -- $tmp
> > > 
> > > It's "$tmp.*"
> > > 
> > > May I ask what problem does the "--" hope to avoid? If the "$tmp" is empty,
> > > "rm -rf" and "rm -rf --"" looks like both doing nothing. So what kind
> > > of situation does the "--" hope to fix?
> > > 
> > > The root problem in above [1] is about "${FOO}*". If someone does "rm -rf ${FOO}*"
> > > in its custom _cleanup_xxxxx function, then it's dangerous if "$FOO" is empty.
> > > 
> > > I thought some ways to avoid that:
> > > 1) Try to avoid doing rm -rf ${FOO}*, if not necessary.
> > > 2) Must checks [ -n "$FOO" ] before doing any rm -rf ${FOO}*
> > > 3) Someone's custom _cleanup_xxxxx better to be called before default _cleanup
> > > does "cd /".
> > > 4) Think about bringing in someone "Static program analysis" tool about bash
> > > script, but I don't know if there're someone good, feel free to give me
> > > suggestions.
> > 
> > "--" prevents the following arguments from being interpreted as options if they
> > begin with "-".  That's a good practice, but it doesn't help with ${FOO} being
> > empty.  To cause the script to exit if ${FOO} is empty, it can be written as
> > ${FOO:?}.  Alternatively, 'set -u' can be used.
> 
> I said that four days ago.  Did nobody receive that reply?
> 
> https://lore.kernel.org/fstests/20240225165128.GA1128@sol.localdomain/T/#m0efd851c5a1fb0dbe418f4aff818d20f4355638b

Sorry Darrick, I've noticed your reply. About the "set -u", it helps to avoid
empty variables, but the question is when should we set it. If we set it at
the beginning of each case, won't it affect too much? Some cases might don't
mind empty variables, but this setting might cause some cases fail. The
programmers might want to decide if they accept empty variables by themselves?

Thanks,
Zorro

> 
> --D
> 
> > - Eric
> > 
> 

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

[Zorro Lang]

On Sun, Feb 25, 2024 at 09:45:27AM -0800, Eric Biggers wrote:
> On Sun, Feb 25, 2024 at 09:03:04AM -0800, Darrick J. Wong wrote:
> > On Sun, Feb 25, 2024 at 08:51:28AM -0800, Eric Biggers wrote:
> > > On Sun, Feb 25, 2024 at 11:16:16PM +0800, Zorro Lang 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/
> > > > > 
> > > > > 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?
> > > > 
> > > > Hi David,
> > > > 
> > > > Thanks for taking care about it :)
> > > > 
> > > > > 
> > > > > For example:
> > > > > 
> > > > > _rm_tmp() {
> > > > >     rm -rf -- $tmp
> > > > 
> > > > It's "$tmp.*"
> > > > 
> > > > May I ask what problem does the "--" hope to avoid? If the "$tmp" is empty,
> > > > "rm -rf" and "rm -rf --"" looks like both doing nothing. So what kind
> > > > of situation does the "--" hope to fix?
> > > > 
> > > > The root problem in above [1] is about "${FOO}*". If someone does "rm -rf ${FOO}*"
> > > > in its custom _cleanup_xxxxx function, then it's dangerous if "$FOO" is empty.
> > > > 
> > > > I thought some ways to avoid that:
> > > > 1) Try to avoid doing rm -rf ${FOO}*, if not necessary.
> > > > 2) Must checks [ -n "$FOO" ] before doing any rm -rf ${FOO}*
> > > > 3) Someone's custom _cleanup_xxxxx better to be called before default _cleanup
> > > > does "cd /".
> > > > 4) Think about bringing in someone "Static program analysis" tool about bash
> > > > script, but I don't know if there're someone good, feel free to give me
> > > > suggestions.
> > > 
> > > "--" prevents the following arguments from being interpreted as options if they
> > > begin with "-".  That's a good practice, but it doesn't help with ${FOO} being

Thanks Eric, I know "--" can do that, just didn't understand how it helps the
empty variable problem. So looks like it doesn't.

> > > empty.  To cause the script to exit if ${FOO} is empty, it can be written as
> > > ${FOO:?}.  Alternatively, 'set -u' can be used.
> > 
> > I said that four days ago.  Did nobody receive that reply?
> > 
> > https://lore.kernel.org/fstests/20240225165128.GA1128@sol.localdomain/T/#m0efd851c5a1fb0dbe418f4aff818d20f4355638b
> > 
> 
> You didn't mention the :? option, and I thought that would be worth mentioning.

Actually:

[ -n "$FOO" ] && rm -rf ${FOO}*

or

rm -rf ${FOO:?}*

Both of them are good to me, depends on what's expected. The 1st one ignore empty $FOO and
keep running, the 2nd one breaks case running if $FOO is empty.

But they all need the programer to realize that his variable might be dangerous if
it's empty, and write as that. So it still depends on the programer or the reviewers
to notice that.

Thanks,
Zorro

> 
> Of course ideally -u would be used everywhere, as you said.
> 
> - Eric
> 

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

[Darrick J. Wong]

On Mon, Feb 26, 2024 at 10:56:29AM +0800, Zorro Lang wrote:
> On Sun, Feb 25, 2024 at 09:45:27AM -0800, Eric Biggers wrote:
> > On Sun, Feb 25, 2024 at 09:03:04AM -0800, Darrick J. Wong wrote:
> > > On Sun, Feb 25, 2024 at 08:51:28AM -0800, Eric Biggers wrote:
> > > > On Sun, Feb 25, 2024 at 11:16:16PM +0800, Zorro Lang 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/
> > > > > > 
> > > > > > 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?
> > > > > 
> > > > > Hi David,
> > > > > 
> > > > > Thanks for taking care about it :)
> > > > > 
> > > > > > 
> > > > > > For example:
> > > > > > 
> > > > > > _rm_tmp() {
> > > > > >     rm -rf -- $tmp
> > > > > 
> > > > > It's "$tmp.*"
> > > > > 
> > > > > May I ask what problem does the "--" hope to avoid? If the "$tmp" is empty,
> > > > > "rm -rf" and "rm -rf --"" looks like both doing nothing. So what kind
> > > > > of situation does the "--" hope to fix?
> > > > > 
> > > > > The root problem in above [1] is about "${FOO}*". If someone does "rm -rf ${FOO}*"
> > > > > in its custom _cleanup_xxxxx function, then it's dangerous if "$FOO" is empty.
> > > > > 
> > > > > I thought some ways to avoid that:
> > > > > 1) Try to avoid doing rm -rf ${FOO}*, if not necessary.
> > > > > 2) Must checks [ -n "$FOO" ] before doing any rm -rf ${FOO}*
> > > > > 3) Someone's custom _cleanup_xxxxx better to be called before default _cleanup
> > > > > does "cd /".
> > > > > 4) Think about bringing in someone "Static program analysis" tool about bash
> > > > > script, but I don't know if there're someone good, feel free to give me
> > > > > suggestions.
> > > > 
> > > > "--" prevents the following arguments from being interpreted as options if they
> > > > begin with "-".  That's a good practice, but it doesn't help with ${FOO} being
> 
> Thanks Eric, I know "--" can do that, just didn't understand how it helps the
> empty variable problem. So looks like it doesn't.
> 
> > > > empty.  To cause the script to exit if ${FOO} is empty, it can be written as
> > > > ${FOO:?}.  Alternatively, 'set -u' can be used.
> > > 
> > > I said that four days ago.  Did nobody receive that reply?
> > > 
> > > https://lore.kernel.org/fstests/20240225165128.GA1128@sol.localdomain/T/#m0efd851c5a1fb0dbe418f4aff818d20f4355638b
> > > 
> > 
> > You didn't mention the :? option, and I thought that would be worth mentioning.

I wasn't even aware that existed.  It seems like a good way to enable
erroring on unset variable on a case by case basis.  Though TBH I
suspect that setting -u and using ${FOO:-} for the cases where we're
actually ok with unset variables is better practice.

Too bad it's going to be a lot of work to do /that/.

> Actually:
> 
> [ -n "$FOO" ] && rm -rf ${FOO}*
> 
> or
> 
> rm -rf ${FOO:?}*
> 
> Both of them are good to me, depends on what's expected. The 1st one ignore empty $FOO and
> keep running, the 2nd one breaks case running if $FOO is empty.
> 
> But they all need the programer to realize that his variable might be dangerous if
> it's empty, and write as that. So it still depends on the programer or the reviewers
> to notice that.

<nod>

--D

> Thanks,
> Zorro
> 
> > 
> > Of course ideally -u would be used everywhere, as you said.
> > 
> > - Eric
> > 
> 
> 

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

[Darrick J. Wong]

On Sun, Feb 25, 2024 at 09:45:27AM -0800, Eric Biggers wrote:
> On Sun, Feb 25, 2024 at 09:03:04AM -0800, Darrick J. Wong wrote:
> > On Sun, Feb 25, 2024 at 08:51:28AM -0800, Eric Biggers wrote:
> > > On Sun, Feb 25, 2024 at 11:16:16PM +0800, Zorro Lang 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/
> > > > > 
> > > > > 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?
> > > > 
> > > > Hi David,
> > > > 
> > > > Thanks for taking care about it :)
> > > > 
> > > > > 
> > > > > For example:
> > > > > 
> > > > > _rm_tmp() {
> > > > >     rm -rf -- $tmp
> > > > 
> > > > It's "$tmp.*"
> > > > 
> > > > May I ask what problem does the "--" hope to avoid? If the "$tmp" is empty,
> > > > "rm -rf" and "rm -rf --"" looks like both doing nothing. So what kind
> > > > of situation does the "--" hope to fix?
> > > > 
> > > > The root problem in above [1] is about "${FOO}*". If someone does "rm -rf ${FOO}*"
> > > > in its custom _cleanup_xxxxx function, then it's dangerous if "$FOO" is empty.
> > > > 
> > > > I thought some ways to avoid that:
> > > > 1) Try to avoid doing rm -rf ${FOO}*, if not necessary.
> > > > 2) Must checks [ -n "$FOO" ] before doing any rm -rf ${FOO}*
> > > > 3) Someone's custom _cleanup_xxxxx better to be called before default _cleanup
> > > > does "cd /".
> > > > 4) Think about bringing in someone "Static program analysis" tool about bash
> > > > script, but I don't know if there're someone good, feel free to give me
> > > > suggestions.
> > > 
> > > "--" prevents the following arguments from being interpreted as options if they
> > > begin with "-".  That's a good practice, but it doesn't help with ${FOO} being
> > > empty.  To cause the script to exit if ${FOO} is empty, it can be written as
> > > ${FOO:?}.  Alternatively, 'set -u' can be used.
> > 
> > I said that four days ago.  Did nobody receive that reply?
> > 
> > https://lore.kernel.org/fstests/20240225165128.GA1128@sol.localdomain/T/#m0efd851c5a1fb0dbe418f4aff818d20f4355638b
> > 
> 
> You didn't mention the :? option, and I thought that would be worth mentioning.
> 
> Of course ideally -u would be used everywhere, as you said.

Yes, but why not reply to my reply instead of replying to the original
patch as if I'd never said anything at all?

The background on that -- I've been noticing the last couple of years
that every now and then I'll reply to something; then a day or two go
by; and then someone else will say the exact same thing I said.

However, they don't simply reply to my email with "Yes, what Darrick
said".  Often the reply literally reiterates what I said.  That makes me
feel invisible, which isn't great.  Then I do some digging and usually
find out that actually no, it's that Microsoft or Google or vger smtp
servers are either (a) broken or (b) their AI have decided that I am a
spammer or (c) maybe I actually /am/ in everyone's killfiles due to the
sheer volume of patches that I send to the lists.

Regardless, every time I see that I start worrying that email is broken
yet again.

That said, I'm not complaining about your specific behavior, Eric; I'm
putting out there that I don't trust our review process at all anymore.
<sadface>

--D

> - Eric
> 

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

[Zorro Lang]

On Wed, Feb 21, 2024 at 08:13:58AM -0800, Darrick J. Wong 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/
> > 
> > Unquoted arguments in shell scripts is IMO a big anti-pattern,
> 
> That wasn't even the problem there.  I temporarily forgot my shellfu and
> did the equivalent of:
> 
> <never set FUBAR>
> 
> rm -rf "$FUBAR"*
> 
> The shell helpfully expands the never-set FUBAR to an empty string, then
> globs the star to everything in the current directory.  rm has
> safeguards to prevent you from "rm -rf /" but that offers no protection
> from what happens above.
> 
> The argument was quoted "properly", but what I think you're really
> asking for is "set -u":
> 
> #!/bin/bash -u
> 
> unset FUBAR
> echo -- rm -f "$FUBAR"*
> 
> $ /tmp/a.sh
> /tmp/a.sh: line 8: FUBAR: unbound variable
> 
> Unfortunately you can't just set that unconditionally for all of fstests
> and call it a day because now you have to test every line of shellcode
> to make sure nothing breaks on unset variables that would have been fine
> otherwise.  That turns into a mess of:
> 
> test -n "$FUBAR" && rm -f "$FUBAR"*
> 
> Oh but then there's the other bad shell habit of setting variables to
> the empty string -- sometimes you really want that empty string, other
> times authors seem to have used that in place of unset.  We've gotten
> away with that bit of bad hygiene for ages, likely due to -u being a
> nondefault option.
> 
> > 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?
> 
> I worried about this a long time ago, and tried running shellcheck on
> the entire codebase.  Thousands of error messages about sloppy quoting
> later I gave up.  Later I turned that into a patch in djwong-wtf that
> runs it only on the files changed by the head commit.
> 
> It **didn't notice** the cleanup error!  So that wouldn't have saved me.
> 
> Long term we ought to rewrite fstests in any language that isn't as much
> of a foot gun.  Or someone starts a project to set -e -u and deals with
> the massive treewide change that's going to be.

I think the "set -e" isn't suitable for fstests, it might bring in more
troubles. About the "set -u", it's a good idea, but as I said in another
reply, I think we need to evaluate its effection on fstests running, especially
if we want to do it at the beginning of each case. There're too many
cases in fstests, I don't know if some cases keep running with empty
variables. What do you think?

> 
> > For example:
> > 
> > _rm_tmp() {
> >     rm -rf -- $tmp
> 
> Um, isn't this /also/ an unquoted variable?
> 
> I guess one could make safe(r) wrappers so at least rm won't get mixed
> up by dashes at the start of filenames:
> 
> _rm_files() {
> 	rm -f -- "$@"
> }
> 
> _rm_dirtree() {
> 	rm -r -- "$@"
> }
> 
> But then you do
> 
> moopath="foo bar"
> _rm_files $moopath
> 
> and we've lost the battle yet again.
> 
> (On the plus side, that's four separate "f*king bash" invocations in one
> email! :))

The cleanup things need more improvement/enhancement, we can't perfect it
in one patchset, so feel free to improve it bit by bit.

> 
> --D
> 
> > }
> > 
> > and used as
> > 
> > _cleanup() {
> >     _rm_tmp
> > }
> > 
> > or at least mandate the "--" separator and quoting arguments in new code
> > and gradually fix the existing code.
> > 
> > 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.
> > 
> 

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

[Eric Biggers]

On Mon, Feb 26, 2024 at 10:56:20AM -0800, Darrick J. Wong wrote:
> > > > "--" prevents the following arguments from being interpreted as options if they
> > > > begin with "-".  That's a good practice, but it doesn't help with ${FOO} being
> > > > empty.  To cause the script to exit if ${FOO} is empty, it can be written as
> > > > ${FOO:?}.  Alternatively, 'set -u' can be used.
> > > 
> > > I said that four days ago.  Did nobody receive that reply?
> > > 
> > > https://lore.kernel.org/fstests/20240225165128.GA1128@sol.localdomain/T/#m0efd851c5a1fb0dbe418f4aff818d20f4355638b
> > > 
> > 
> > You didn't mention the :? option, and I thought that would be worth mentioning.
> > 
> > Of course ideally -u would be used everywhere, as you said.
> 
> Yes, but why not reply to my reply instead of replying to the original
> patch as if I'd never said anything at all?
> 
> The background on that -- I've been noticing the last couple of years
> that every now and then I'll reply to something; then a day or two go
> by; and then someone else will say the exact same thing I said.
> 
> However, they don't simply reply to my email with "Yes, what Darrick
> said".  Often the reply literally reiterates what I said.  That makes me
> feel invisible, which isn't great.  Then I do some digging and usually
> find out that actually no, it's that Microsoft or Google or vger smtp
> servers are either (a) broken or (b) their AI have decided that I am a
> spammer or (c) maybe I actually /am/ in everyone's killfiles due to the
> sheer volume of patches that I send to the lists.
> 
> Regardless, every time I see that I start worrying that email is broken
> yet again.
> 
> That said, I'm not complaining about your specific behavior, Eric; I'm
> putting out there that I don't trust our review process at all anymore.
> <sadface>
> 

I did receive your email.  I just happened to reply to Zorro's reply instead
because it was more recent, was discussing the same topic, and I was adding some
new information anyway.  Sorry for trying to contribute.

- Eric

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

[David Sterba]

On Wed, Feb 21, 2024 at 08:13:58AM -0800, Darrick J. Wong 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/
> > 
> > Unquoted arguments in shell scripts is IMO a big anti-pattern,
> 
> That wasn't even the problem there.  I temporarily forgot my shellfu and
> did the equivalent of:
> 
> <never set FUBAR>
> 
> rm -rf "$FUBAR"*
> 
> The shell helpfully expands the never-set FUBAR to an empty string, then
> globs the star to everything in the current directory.  rm has
> safeguards to prevent you from "rm -rf /" but that offers no protection
> from what happens above.
> 
> The argument was quoted "properly", but what I think you're really
> asking for is "set -u":
> 
> #!/bin/bash -u
> 
> unset FUBAR
> echo -- rm -f "$FUBAR"*
> 
> $ /tmp/a.sh
> /tmp/a.sh: line 8: FUBAR: unbound variable
> 
> Unfortunately you can't just set that unconditionally for all of fstests
> and call it a day because now you have to test every line of shellcode
> to make sure nothing breaks on unset variables that would have been fine
> otherwise.  That turns into a mess of:
> 
> test -n "$FUBAR" && rm -f "$FUBAR"*
> 
> Oh but then there's the other bad shell habit of setting variables to
> the empty string -- sometimes you really want that empty string, other
> times authors seem to have used that in place of unset.  We've gotten
> away with that bit of bad hygiene for ages, likely due to -u being a
> nondefault option.
> 
> > 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?
> 
> I worried about this a long time ago, and tried running shellcheck on
> the entire codebase.  Thousands of error messages about sloppy quoting
> later I gave up.  Later I turned that into a patch in djwong-wtf that
> runs it only on the files changed by the head commit.
> 
> It **didn't notice** the cleanup error!  So that wouldn't have saved me.
> 
> Long term we ought to rewrite fstests in any language that isn't as much
> of a foot gun.  Or someone starts a project to set -e -u and deals with
> the massive treewide change that's going to be.

I see from your and Dave's response that the problems are known and that
everybody tried to fix it in some way. What I also see that it's kind of
futile so that people carry their patches in own branches for testing.
The amount of treewide changes is probably killing the idea or this
would have to be coordinated or done in sprints or idk.

> > For example:
> > 
> > _rm_tmp() {
> >     rm -rf -- $tmp
> 
> Um, isn't this /also/ an unquoted variable?

That was not the best example, I took the existing code and put a
wrapper but yeah quoting should have been there.

> I guess one could make safe(r) wrappers so at least rm won't get mixed
> up by dashes at the start of filenames:
> 
> _rm_files() {
> 	rm -f -- "$@"
> }
> 
> _rm_dirtree() {
> 	rm -r -- "$@"
> }
> 
> But then you do
> 
> moopath="foo bar"
> _rm_files $moopath
> 
> and we've lost the battle yet again.

Maybe if there are known patterns and helpers that avoid the unsafe
constructs this could be avoided. Then go trough all code, fix it one by
one and catch it in newly submitted code. It's too much handwaving now.
I think we'd know what to do but the questions are when and how to get
the changes merged.

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

[David Sterba]

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.

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

[Eric Biggers]

On Thu, Feb 29, 2024 at 07:44:52PM +0100, David Sterba wrote:
> > I worried about this a long time ago, and tried running shellcheck on
> > the entire codebase.  Thousands of error messages about sloppy quoting
> > later I gave up.  Later I turned that into a patch in djwong-wtf that
> > runs it only on the files changed by the head commit.
> > 
> > It **didn't notice** the cleanup error!  So that wouldn't have saved me.
> > 
> > Long term we ought to rewrite fstests in any language that isn't as much
> > of a foot gun.  Or someone starts a project to set -e -u and deals with
> > the massive treewide change that's going to be.
> 
> I see from your and Dave's response that the problems are known and that
> everybody tried to fix it in some way. What I also see that it's kind of
> futile so that people carry their patches in own branches for testing.
> The amount of treewide changes is probably killing the idea or this
> would have to be coordinated or done in sprints or idk.

At the risk of being blamed for posting a "redundant" response again:

What I tend to do in new shell scripts outside xfstests is use
'set -e -u -o pipefail', and also make sure they pass shellcheck.

For example, I did that for the tests I wrote for the fscrypt command-line tool:
https://github.com/google/fscrypt/tree/master/cli-tests.  And I made the
shellcheck be enforced by GitHub Actions, so new warnings don't get introduced.

So, the point is, I'm very much in favor of 'set -e -u -o pipefail' plus
shellcheck as a modern, less error-prone way to write shell scripts.

What can realistically be done with the existing 170000 (?) lines of shell
script in xfstests is another question.  Perhaps common/ could be updated first,
and then individual tests could opt into strict mode as they get updated too.
It would be a lot of work in any case.

- Eric