Re: [PATCH v2] tests: Don't run f2fs test on systems with PAGE_SIZE more than 4096 bytes

[Hans Ulrich Niedermann <hun@n-dimensional.de>]

On Wed, 29 Apr 2020 19:25:57 +0200
Hans Ulrich Niedermann <hun@n-dimensional.de> wrote:

> On Wed, 29 Apr 2020 17:06:36 +0300
> Anatoly Pugachev <matorola@gmail.com> wrote:
> 
> > Don't run f2fs test on systems with PAGE_SIZE > 4096 bytes, since
> > f2fs is not supported on these systems and trying to mount a f2fs
> > filesystem would fail.  
> 
> "Skip the f2fs test on ..." might be better wording, both in this
> paragraph and the Subject.
> 
> Exit code 77 is certainly documented with the word "skip", and "exit
> 77" will show up in the "make check" output as "SKIP" as well.
> 
> > v2 changes:
> > 
> > - fix compare
> > - quotes around variable expansion
> > 
> > Signed-off-by: Anatoly Pugachev <matorola@gmail.com>
> > Reviewed-by: Mike Gilbert <floppym@gentoo.org>
> > 
> > ---
> >  tests/f2fs_test.in | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/f2fs_test.in b/tests/f2fs_test.in
> > index 1ea77c826..3da1dad57 100644
> > --- a/tests/f2fs_test.in
> > +++ b/tests/f2fs_test.in
> > @@ -15,5 +15,11 @@ if ! which mkfs.f2fs >/dev/null 2>&1; then
> >   exit 77
> >  fi
> >  
> > +PAGE_SIZE=$(getconf PAGE_SIZE)
> > +F2FS_BLKSIZE=4096
> > +if [ "$PAGE_SIZE" -gt "$F2FS_BLKSIZE" ]; then
> > + printf "f2fs is not supported on PAGE_SIZE(%d) != %d\n" $PAGE_SIZE
> > $F2FS_BLKSIZE
> > + exit 77
> > +fi  
> 
> This confusing to me. You are skipping the test when
> 
>     PAGE_SIZE > F2FS_BLKSIZE
> 
> but the corresponding message says
> 
>     PAGE_SIZE != F2FS_BLKSIZE
> 
> Now... which condition is it supposed to be? ">" or "!="?
> 
> I know from the Linux kernel's ext2 driver that it is very well
> possible that PAGE_SIZE != EXT2_BLOCK_SIZE can work
> unless EXT2_BLOCK_SIZE > PAGE_SIZE.
> 
> So ">" and "!=" are not necessarily the same thing, and IMHO the check
> and the message use the same condition.

The commit title and the commit message are also talking about
"PAGE_SIZE > 4096" instead of "PAGE_SIZE != 4096".

This adds more confusion over whether F2FS_BLKSIZE just happens to be
4096 somewhere where PAGE_SIZE is 4096, whether F2FS_BLKSIZE is defined
to be 4096 everywhere, and whether the whole thing is about the number
4096 or about the value of F2FS_BLKSIZE.

(Again, in the ext2 example, filesystems can have many block sizes, but
the default is the Linux PAGE_SIZE, so all x86 PCs will create
filesystems with block size 4096 but a MIPS system like the Western
Digital SOHO external HDD NAS will create a filesystem with block
size 65536. The kernel driver just needs PAGE_SIZE >= block size to
work, so a MIPS created ext2 filesystem will not mount on a x86 PC.)

Mentioning the same condition in commit title, commit message, the
actual "if" branch, and then the "printf" message would certainly reduce
my confusion.

Uli