Re: [PATCH v5 1/3] tests: Split ZFS ZSTD Test Into New File

On Wed, Nov 12, 2025 at 08:07:58PM -0600, Glenn Washburn wrote:
> On Wed, 12 Nov 2025 20:52:39 +0100 > Daniel Kiper <[email protected]> wrote:
> > On Sat, Nov 08, 2025 at 08:55:48PM -0600, Glenn Washburn wrote:
> > > On Thu,  6 Nov 2025 21:41:37 -0600 Andrew Hamilton <[email protected]> wrote:
> > > > Split ZFS ZSTD test into its own test script. Add a check
> > > > to the new test script to see if the zfs utility installed
> > > > on the host supports "zstd" compression before running the
> > > > test and skip the test if not. It seems at least some
> > > > zfs-fuse binaries do not support zstd compression and the current
> > > > test will fail in that case. Splitting into a new file will avoid
> > > > masking other test failures due to missing zstd support.
> > > >
> > > > Signed-off-by: Andrew Hamilton <[email protected]>
> > > > Reviewed-by: Daniel Kiper <[email protected]>
> > > > ---
> > > >  .gitignore             |  1 +
> > > >  Makefile.util.def      |  6 ++++++
> > > >  tests/zfs_test.in      |  1 -
> > > >  tests/zfs_zstd_test.in | 30 ++++++++++++++++++++++++++++++
> > > >  4 files changed, 37 insertions(+), 1 deletion(-)
> > > >  create mode 100644 tests/zfs_zstd_test.in
> > > >
> > > > diff --git a/.gitignore b/.gitignore
> > > > index 524f2e6d0..67ad2d26d 100644
> > > > --- a/.gitignore
> > > > +++ b/.gitignore
> > > > @@ -283,3 +283,4 @@ widthspec.bin
> > > >  /xfs_test
> > > >  /xzcompress_test
> > > >  /zfs_test
> > > > +/zfs_zstd_test
> > > > diff --git a/Makefile.util.def b/Makefile.util.def
> > > > index 91720e627..7b91c0b61 100644
> > > > --- a/Makefile.util.def
> > > > +++ b/Makefile.util.def
> > > > @@ -914,6 +914,12 @@ script = {
> > > >    common = tests/zfs_test.in;
> > > >  };
> > > >
> > > > +script = {
> > > > +  testcase = native;
> > > > +  name = zfs_zstd_test;
> > > > +  common = tests/zfs_zstd_test.in;
> > > > +};
> > > > +
> > > >  script = {
> > > >    testcase = native;
> > > >    name = cpio_test;
> > > > diff --git a/tests/zfs_test.in b/tests/zfs_test.in
> > > > index e1cb766a5..cd547b4d2 100644
> > > > --- a/tests/zfs_test.in
> > > > +++ b/tests/zfs_test.in
> > > > @@ -19,7 +19,6 @@ fi
> > > >  "@builddir@/grub-fs-tester" zfs_lzjb
> > > >  "@builddir@/grub-fs-tester" zfs_gzip
> > > >  "@builddir@/grub-fs-tester" zfs_zle
> > > > -"@builddir@/grub-fs-tester" zfs_zstd
> > > >  "@builddir@/grub-fs-tester" zfs_raidz3
> > > >  "@builddir@/grub-fs-tester" zfs_raidz2
> > > >  "@builddir@/grub-fs-tester" zfs_raidz
> > > > diff --git a/tests/zfs_zstd_test.in b/tests/zfs_zstd_test.in
> > > > new file mode 100644
> > > > index 000000000..1b8a20212
> > > > --- /dev/null
> > > > +++ b/tests/zfs_zstd_test.in
> > > > @@ -0,0 +1,30 @@
> > > > +#!@BUILD_SHEBANG@
> > > > +
> > > > +set -e
> > > > +
> > > > +if [ "x$EUID" = "x" ] ; then
> > > > +  EUID=`id -u`
> > > > +fi
> > > > +
> > > > +if [ "$EUID" != 0 ] ; then
> > > > +   exit 99
> > > > +fi
> > > > +
> > > > +if ! which zpool >/dev/null 2>&1; then
> > > > +   echo "zpool not installed; cannot test zfs."
> > > > +   exit 77
> > > > +fi
> > > > +
> > > > +if ! which zfs >/dev/null 2>&1; then
> > > > +   echo "zfs not installed; cannot test zfs."
> > > > +   exit 77
> > > > +fi
> > > > +
> > > > +# If OpenZFS is not installed (only zfs-fuse for example) then
> > > > +# skip ZSTD compression testing.
> > >
> > > This is kind of misleading as a comment. Its really, "If the ZFS
> > > implementation does not support ZSTD compression, for example zfs-fuse,
> > > then fail early." OpenZFS just happens to be the only ZFS
> > > implementation that currently supports ZSTD compression. And this might
> > > not even be true, do Solaris implementations support ZSTD? A quick
> > > search suggests yes. Now running tests is only officially on Debian (as
> > > of now), but I don't think this comment needs to take that as an
> > > assumption.
> >
> > Andrew, could you fix this?
> >
> > > > +if ! zfs get 2>&1 | grep -F "compression" | grep -F "zstd"; then
> > > > +   echo "zfs zstd compression not supported; cannot test zfs zstd."
> > > > +   exit 77
> > >
> > > These should be reverted to return 99. I explained in a reply to the v4
> > > patch. Apologize for not catching this sooner.
> >
> > Glenn, OK, so, AIUI you are OK with earlier two 77's but this one should
> > be reverted to 99. Right?
>
> They really should all be 99, which means the test failed for a reason
> other than "GRUB has a bug that needs to be fixed". This typically is
> because of a misconfigured environment, which is what all the above
> tests check for. A 77 means, skip this test because it tests for
> something not relevant to this target.

OK... May I ask you to document all these codes somewhere?
docs/grub-dev.texi?

Daniel

_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel