Re: [PATCH v2] fstests: add support for hfsplus

["Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>]

On Tue, Sep 25, 2018 at 08:57:58AM +0300, Amir Goldstein wrote:
> On Tue, Sep 25, 2018 at 12:44 AM Ernesto A. Fernández
> <ernesto.mnd.fernandez@gmail.com> wrote:
> >
> > Running tests on hfsplus requires patched versions of the mkfs and fsck
> > tools [1] that support filesystems smaller than the device.
> 
> That's not accurate, is it? Wouldn't it be more accurate to say that the patched
> tools are required to run the _scratch_mkfs_sized tests?

I set this up to call _fatal early if the tools are not the patched
versions.

Using -X on mkfs.hfsplus forces the size to be a whole number of blocks,
and using -X on fsck makes it work under that assumption.  This is briefly
explained in the patched man page for the tools.

It's a workaround needed because hfsplus only stores the number of
allocation blocks in the volume header.  If the device size is not a
multiple of the block size, the backup header will be past the end, and
fsck will take that as corruption.

Of course I could let the patched fsck use the -X flag by default.  But
that could be dangerous if somebody accidentally uses it to check an
important filesystem.

> 
> >
> > [1] https://github.com/eafer/hfsprogs-linux.git
> >
> > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> > ---
> >  common/config | 10 ++++++++++
> >  common/rc     | 15 ++++++++++++++-
> >  2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/config b/common/config
> > index 1ba8d96c..315f8b1e 100644
> > --- a/common/config
> > +++ b/common/config
> > @@ -229,6 +229,7 @@ case "$HOSTOS" in
> >         export MKFS_CIFS_PROG="false"
> >         export MKFS_OVERLAY_PROG="false"
> >         export MKFS_REISER4_PROG=$(type -P mkfs.reiser4)
> > +       export MKFS_HFSPLUS_PROG=$(type -P mkfs.hfsplus)
> >         export E2FSCK_PROG=$(type -P e2fsck)
> >         export TUNE2FS_PROG=$(type -P tune2fs)
> >         export FSCK_OVERLAY_PROG=$(type -P fsck.overlay)
> > @@ -313,6 +314,9 @@ _mount_opts()
> >         ubifs)
> >                 export MOUNT_OPTIONS=$UBIFS_MOUNT_OPTIONS
> >                 ;;
> > +       hfsplus)
> > +               export MOUNT_OPTIONS=$HFSPLUS_MOUNT_OPTIONS
> > +               ;;
> >         *)
> >                 ;;
> >         esac
> > @@ -380,6 +384,9 @@ _mkfs_opts()
> >         f2fs)
> >                 export MKFS_OPTIONS="$F2FS_MKFS_OPTIONS"
> >                 ;;
> > +       hfsplus)
> > +               export MKFS_OPTIONS=$HFSPLUS_MKFS_OPTIONS
> > +               ;;
> >         *)
> >                 ;;
> >         esac
> > @@ -397,6 +404,9 @@ _fsck_opts()
> >         f2fs)
> >                 export FSCK_OPTIONS=""
> >                 ;;
> > +       hfsplus)
> > +               export FSCK_OPTIONS="-nX"
> > +               ;;
> >         *)
> >                 export FSCK_OPTIONS="-n"
> >                 ;;
> > diff --git a/common/rc b/common/rc
> > index d5bb1fee..a843e9f6 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -158,6 +158,12 @@ case "$FSTYP" in
> >      ubifs)
> >         [ "$UBIUPDATEVOL_PROG" = "" ] && _fatal "ubiupdatevol not found"
> >         ;;
> > +    hfsplus)
> > +       [ "$MKFS_HFSPLUS_PROG" = "" ] && _fatal "mkfs.hfsplus not found"
> > +       mkfs.hfsplus -X |& grep "invalid" &&
> > +               _fatal "A patched version of mkfs.hfsplus is required:" \
> > +                      "https://github.com/eafer/hfsprogs-linux.git"
> 
> Why fatal and not just notrun the _scratch_mkfs_sized tests if no -X support?
> Must all tests run with mkfs -X and fsck -X??

I always use -X on mkfs so the tests will complain if the size of the
device is not a whole number of blocks.  Otherwise the tests would later
fail on fsck.

Maybe I should check for -X support when setting the FSCK_OPTIONS, and in
_scratch_mkfs()?  Then _fatal would not be necessary.

> 
> > +       ;;
> >  esac
> >
> >  if [ ! -z "$REPORT_LIST" ]; then
> > @@ -746,6 +752,10 @@ _scratch_mkfs()
> >                 mkfs_cmd="yes | $MKFS_PROG -t $FSTYP --"
> >                 mkfs_filter="grep -v -e ^mkfs\.ocfs2"
> >                 ;;
> > +       hfsplus)
> > +               mkfs_cmd="yes | $MKFS_PROG -t $FSTYP -- -X"
> 
> -X here seems like a typo?

This is so the tests will complain if the device size is not a multiple
of the block size.  I think I explained it in the patched man page for
mkfs.hfsplus, but it may be better to put a comment here.

Thanks,
Ernest

> 
> Thanks,
> Amir.